Skip to content

first stage of implementing LLVM code coverage #73011

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Jun 19, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
added test, Operand::const_from_scalar, require_lang_item, & comments
Addresses feedback from @oli-obk (Thanks!)
  • Loading branch information
richkadel committed Jun 15, 2020
commit 20aba8f634c13fa2bb1b043b51a074769dc06f66
28 changes: 28 additions & 0 deletions src/librustc_middle/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ use rustc_macros::HashStable;
use rustc_serialize::{Decodable, Encodable};
use rustc_span::symbol::Symbol;
use rustc_span::{Span, DUMMY_SP};
use rustc_target::abi;
use rustc_target::asm::InlineAsmRegOrRegClass;
use std::borrow::Cow;
use std::fmt::{self, Debug, Display, Formatter, Write};
Expand Down Expand Up @@ -2218,6 +2219,33 @@ impl<'tcx> Operand<'tcx> {
})
}

/// Convenience helper to make a literal-like constant from a given scalar value.
/// Since this is used to synthesize MIR, assumes `user_ty` is None.
pub fn const_from_scalar(
tcx: TyCtxt<'tcx>,
ty: Ty<'tcx>,
val: Scalar,
span: Span,
) -> Operand<'tcx> {
debug_assert!({
let param_env_and_ty = ty::ParamEnv::empty().and(ty);
let type_size = tcx
.layout_of(param_env_and_ty)
.unwrap_or_else(|e| panic!("could not compute layout for {:?}: {:?}", ty, e))
.size;
let scalar_size = abi::Size::from_bytes(match val {
Scalar::Raw { size, .. } => size,
_ => panic!("Invalid scalar type {:?}", val),
});
scalar_size == type_size
});
Operand::Constant(box Constant {
span,
user_ty: None,
literal: ty::Const::from_scalar(tcx, val, ty),
})
}

pub fn to_copy(&self) -> Self {
match *self {
Operand::Copy(_) | Operand::Constant(_) => self.clone(),
Expand Down
1 change: 1 addition & 0 deletions src/librustc_mir/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
);
self.copy_op(self.operand_index(args[0], index)?, dest)?;
}
// FIXME(#73156): Handle source code coverage in const eval
sym::count_code_region => (),
Copy link
Member

@RalfJung RalfJung Jun 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please always ping @rust-lang/wg-const-eval for new intrinsics in CTFE.
It is not great when I realize by mere accident that we have a new intrinsic here. It's just a stub this time, so no big deal, but who knows what it might be next time. ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, note that as per this comment, T-lang should have been involved as well:

//! Note: any changes to the constness of intrinsics should be discussed with the language team.
//! This includes changes in the stability of the constness.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amusingly this intrinsic is not marked as const at all. The static checks preventing non-const intrinsics from being invoked happen before mir optimizations. Since instrumentation is a MIR transformation... this isn't caught. Maybe instrumentation should happen before const checking and such?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, so this intrinsic is not called by users, but the call is inserted by an instrumentation pass later? Interesting. That reduces my T-lang concern.

Maybe instrumentation should happen before const checking and such?

Maybe -- I am not sure to what extend it would be useful to check the instrumented code as if it was user-written.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I write

const fn a() -> u8 {
    42
}

const A: u8 = a();

fn main() {
    println!("{}", A);
}

Then currently a will contribute towards coverage, yet it is not marked as executed. I think the count_code_region intrinsic should also mark code as executed during CTFE.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bjorn3 that is a separate issue though from the other discussion here? Namely, it is #73156.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't see that issue, thanks.

Copy link
Member

@wesleywiser wesleywiser Jun 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that @RalfJung! That was my fault; I didn't realize we should be pinging that group. I will try to keep that in mind in the future.

Do you think we still need to get the lang team involved? This intrinsic is used by an unstable -Z compiler flag which generates code coverage data using LLVM features. The compiler team already signed off on this approach via an MCP.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt they will have any objections, but sending T-lang an "FYI this is what we did here" would still be a good idea I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for flagging the issue @RalfJung ...

Note also, the next PR increment is under review, and adds some additional Rust intrinsics that I believe we need for coverage map generation.

See this related note:

#73684 (comment)

Since the MIR analysis is minimal at the moment (injecting at the function level only) I'm not generating calls to these intrinsics yet, but when I do, I suspect I may need to add these new intrinsics to src/librustc_mir/interpret/intrinsics.rs as well, similarly stubbed out.

_ => return Ok(false),
}
Expand Down
52 changes: 15 additions & 37 deletions src/librustc_mir/transform/instrument_coverage.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,17 @@
use crate::transform::{MirPass, MirSource};
use crate::util::patch::MirPatch;
use rustc_hir::lang_items;
use rustc_middle::mir::interpret::Scalar;
use rustc_middle::mir::*;
use rustc_middle::ty;
use rustc_middle::ty::Ty;
use rustc_middle::ty::TyCtxt;
use rustc_span::def_id::DefId;
use rustc_span::Span;
use rustc_target::abi;

/// Inserts call to count_code_region() as a placeholder to be replaced during code generation with
/// the intrinsic llvm.instrprof.increment.
pub struct InstrumentCoverage;

/**
* Inserts call to count_code_region() as a placeholder to be replaced during code generation with
* the intrinsic llvm.instrprof.increment.
*/

impl<'tcx> MirPass<'tcx> for InstrumentCoverage {
fn run_pass(&self, tcx: TyCtxt<'tcx>, src: MirSource<'tcx>, body: &mut Body<'tcx>) {
if tcx.sess.opts.debugging_opts.instrument_coverage {
Expand All @@ -34,10 +30,17 @@ const INIT_FUNCTION_COUNTER: u32 = 0;
pub fn instrument_coverage<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
let span = body.span.shrink_to_lo();

let count_code_region_fn =
function_handle(tcx, span, tcx.lang_items().count_code_region_fn().unwrap());
let counter_index =
const_int_operand(tcx, span, tcx.types.u32, Scalar::from_u32(INIT_FUNCTION_COUNTER));
let count_code_region_fn = function_handle(
tcx,
tcx.require_lang_item(lang_items::CountCodeRegionFnLangItem, None),
span,
);
let counter_index = Operand::const_from_scalar(
tcx,
tcx.types.u32,
Scalar::from_u32(INIT_FUNCTION_COUNTER),
span,
);

let mut patch = MirPatch::new(body);

Expand Down Expand Up @@ -68,38 +71,13 @@ pub fn instrument_coverage<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
body.basic_blocks_mut().swap(next_block, new_block);
}

fn function_handle<'tcx>(tcx: TyCtxt<'tcx>, span: Span, fn_def_id: DefId) -> Operand<'tcx> {
fn function_handle<'tcx>(tcx: TyCtxt<'tcx>, fn_def_id: DefId, span: Span) -> Operand<'tcx> {
let ret_ty = tcx.fn_sig(fn_def_id).output();
let ret_ty = ret_ty.no_bound_vars().unwrap();
let substs = tcx.mk_substs(::std::iter::once(ty::subst::GenericArg::from(ret_ty)));
Operand::function_handle(tcx, fn_def_id, substs, span)
}

fn const_int_operand<'tcx>(
tcx: TyCtxt<'tcx>,
span: Span,
ty: Ty<'tcx>,
val: Scalar,
) -> Operand<'tcx> {
debug_assert!({
let param_env_and_ty = ty::ParamEnv::empty().and(ty);
let type_size = tcx
.layout_of(param_env_and_ty)
.unwrap_or_else(|e| panic!("could not compute layout for {:?}: {:?}", ty, e))
.size;
let scalar_size = abi::Size::from_bytes(match val {
Scalar::Raw { size, .. } => size,
_ => panic!("Invalid scalar type {:?}", val),
});
scalar_size == type_size
});
Operand::Constant(box Constant {
span,
user_ty: None,
literal: ty::Const::from_scalar(tcx, val, ty),
})
}

fn placeholder_block<'tcx>(source_info: SourceInfo) -> BasicBlockData<'tcx> {
BasicBlockData {
statements: vec![],
Expand Down
19 changes: 19 additions & 0 deletions src/test/mir-opt/instrument_coverage.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Test that the initial version of Rust coverage injects count_code_region() placeholder calls,
// at the top of each function. The placeholders are later converted into LLVM instrprof.increment
// intrinsics, during codegen.

// compile-flags: -Zinstrument-coverage
// EMIT_MIR rustc.main.InstrumentCoverage.diff
// EMIT_MIR rustc.bar.InstrumentCoverage.diff
fn main() {
loop {
if bar() {
break;
}
}
}

#[inline(never)]
fn bar() -> bool {
true
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
- // MIR for `bar` before InstrumentCoverage
+ // MIR for `bar` after InstrumentCoverage

fn bar() -> bool {
let mut _0: bool; // return place in scope 0 at $DIR/instrument_coverage.rs:17:13: 17:17
+ let mut _1: (); // in scope 0 at $DIR/instrument_coverage.rs:17:1: 19:2

bb0: {
+ StorageLive(_1); // scope 0 at $DIR/instrument_coverage.rs:17:1: 19:2
+ _1 = const std::intrinsics::count_code_region(const 0u32) -> bb2; // scope 0 at $DIR/instrument_coverage.rs:17:1: 19:2
+ // ty::Const
+ // + ty: unsafe extern "rust-intrinsic" fn(u32) {std::intrinsics::count_code_region}
+ // + val: Value(Scalar(<ZST>))
+ // mir::Constant
+ // + span: $DIR/instrument_coverage.rs:17:1: 17:1
+ // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(u32) {std::intrinsics::count_code_region}, val: Value(Scalar(<ZST>)) }
+ // ty::Const
+ // + ty: u32
+ // + val: Value(Scalar(0x00000000))
+ // mir::Constant
+ // + span: $DIR/instrument_coverage.rs:17:1: 17:1
+ // + literal: Const { ty: u32, val: Value(Scalar(0x00000000)) }
+ }
+
+ bb1 (cleanup): {
+ resume; // scope 0 at $DIR/instrument_coverage.rs:17:1: 19:2
+ }
+
+ bb2: {
+ StorageDead(_1); // scope 0 at $DIR/instrument_coverage.rs:18:5: 18:9
_0 = const true; // scope 0 at $DIR/instrument_coverage.rs:18:5: 18:9
// ty::Const
// + ty: bool
// + val: Value(Scalar(0x01))
// mir::Constant
// + span: $DIR/instrument_coverage.rs:18:5: 18:9
// + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
return; // scope 0 at $DIR/instrument_coverage.rs:19:2: 19:2
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
- // MIR for `main` before InstrumentCoverage
+ // MIR for `main` after InstrumentCoverage

fn main() -> () {
let mut _0: (); // return place in scope 0 at $DIR/instrument_coverage.rs:8:11: 8:11
let mut _1: (); // in scope 0 at $DIR/instrument_coverage.rs:8:1: 14:2
let mut _2: bool; // in scope 0 at $DIR/instrument_coverage.rs:10:12: 10:17
let mut _3: !; // in scope 0 at $DIR/instrument_coverage.rs:10:18: 12:10
+ let mut _4: (); // in scope 0 at $DIR/instrument_coverage.rs:8:1: 14:2

bb0: {
- falseUnwind -> [real: bb1, cleanup: bb6]; // scope 0 at $DIR/instrument_coverage.rs:9:5: 13:6
+ StorageLive(_4); // scope 0 at $DIR/instrument_coverage.rs:8:1: 14:2
+ _4 = const std::intrinsics::count_code_region(const 0u32) -> bb7; // scope 0 at $DIR/instrument_coverage.rs:8:1: 14:2
+ // ty::Const
+ // + ty: unsafe extern "rust-intrinsic" fn(u32) {std::intrinsics::count_code_region}
+ // + val: Value(Scalar(<ZST>))
+ // mir::Constant
+ // + span: $DIR/instrument_coverage.rs:8:1: 8:1
+ // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(u32) {std::intrinsics::count_code_region}, val: Value(Scalar(<ZST>)) }
+ // ty::Const
+ // + ty: u32
+ // + val: Value(Scalar(0x00000000))
+ // mir::Constant
+ // + span: $DIR/instrument_coverage.rs:8:1: 8:1
+ // + literal: Const { ty: u32, val: Value(Scalar(0x00000000)) }
}

bb1: {
StorageLive(_2); // scope 0 at $DIR/instrument_coverage.rs:10:12: 10:17
_2 = const bar() -> [return: bb2, unwind: bb6]; // scope 0 at $DIR/instrument_coverage.rs:10:12: 10:17
// ty::Const
// + ty: fn() -> bool {bar}
// + val: Value(Scalar(<ZST>))
// mir::Constant
// + span: $DIR/instrument_coverage.rs:10:12: 10:15
// + literal: Const { ty: fn() -> bool {bar}, val: Value(Scalar(<ZST>)) }
}

bb2: {
FakeRead(ForMatchedPlace, _2); // scope 0 at $DIR/instrument_coverage.rs:10:12: 10:17
switchInt(_2) -> [false: bb4, otherwise: bb3]; // scope 0 at $DIR/instrument_coverage.rs:10:9: 12:10
}

bb3: {
falseEdges -> [real: bb5, imaginary: bb4]; // scope 0 at $DIR/instrument_coverage.rs:10:9: 12:10
}

bb4: {
_1 = const (); // scope 0 at $DIR/instrument_coverage.rs:10:9: 12:10
// ty::Const
// + ty: ()
// + val: Value(Scalar(<ZST>))
// mir::Constant
// + span: $DIR/instrument_coverage.rs:10:9: 12:10
// + literal: Const { ty: (), val: Value(Scalar(<ZST>)) }
StorageDead(_2); // scope 0 at $DIR/instrument_coverage.rs:13:5: 13:6
goto -> bb0; // scope 0 at $DIR/instrument_coverage.rs:9:5: 13:6
}

bb5: {
_0 = const (); // scope 0 at $DIR/instrument_coverage.rs:11:13: 11:18
// ty::Const
// + ty: ()
// + val: Value(Scalar(<ZST>))
// mir::Constant
// + span: $DIR/instrument_coverage.rs:11:13: 11:18
// + literal: Const { ty: (), val: Value(Scalar(<ZST>)) }
StorageDead(_2); // scope 0 at $DIR/instrument_coverage.rs:13:5: 13:6
return; // scope 0 at $DIR/instrument_coverage.rs:14:2: 14:2
}

bb6 (cleanup): {
resume; // scope 0 at $DIR/instrument_coverage.rs:8:1: 14:2
+ }
+
+ bb7: {
+ StorageDead(_4); // scope 0 at $DIR/instrument_coverage.rs:9:5: 13:6
+ falseUnwind -> [real: bb1, cleanup: bb6]; // scope 0 at $DIR/instrument_coverage.rs:9:5: 13:6
}
}