Skip to content

Commit

Permalink
Fix -Z instrument-coverage on MSVC
Browse files Browse the repository at this point in the history
Found that -C link-dead-code (which was enabled automatically
under -Z instrument-coverage) was causing the linking error that
resulted in segmentation faults in coverage instrumented binaries. Link
dead code is now disabled under MSVC, allowing `-Z instrument-coverage`
to be enabled under MSVC for the first time.

More details are included in Issue #76038.

(This PR was broken out from PR #75828)
  • Loading branch information
richkadel committed Sep 1, 2020
1 parent 85fbf49 commit ddb054a
Show file tree
Hide file tree
Showing 6 changed files with 45 additions and 34 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/back/link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1668,7 +1668,7 @@ fn linker_with_args<'a, B: ArchiveBuilder<'a>>(
// FIXME: Order dependent, applies to the following objects. Where should it be placed?
// Try to strip as much out of the generated object by removing unused
// sections if possible. See more comments in linker.rs
if sess.opts.cg.link_dead_code != Some(true) {
if !sess.link_dead_code() {
let keep_metadata = crate_type == CrateType::Dylib;
cmd.gc_sections(keep_metadata);
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/mir/mono.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ impl<'tcx> MonoItem<'tcx> {
.debugging_opts
.inline_in_all_cgus
.unwrap_or_else(|| tcx.sess.opts.optimize != OptLevel::No)
&& tcx.sess.opts.cg.link_dead_code != Some(true);
&& !tcx.sess.link_dead_code();

match *self {
MonoItem::Fn(ref instance) => {
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir/src/monomorphize/partitioning/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ pub fn partition<'tcx>(

// Next we try to make as many symbols "internal" as possible, so LLVM has
// more freedom to optimize.
if tcx.sess.opts.cg.link_dead_code != Some(true) {
if !tcx.sess.link_dead_code() {
let _prof_timer = tcx.prof.generic_activity("cgu_partitioning_internalize_symbols");
partitioner.internalize_symbols(tcx, &mut post_inlining, inlining_map);
}
Expand Down Expand Up @@ -327,7 +327,7 @@ fn collect_and_partition_mono_items<'tcx>(
}
}
None => {
if tcx.sess.opts.cg.link_dead_code == Some(true) {
if tcx.sess.link_dead_code() {
MonoItemCollectionMode::Eager
} else {
MonoItemCollectionMode::Lazy
Expand Down
17 changes: 4 additions & 13 deletions compiler/rustc_session/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1718,20 +1718,11 @@ pub fn build_session_options(matches: &getopts::Matches) -> Options {
);
}

// `-Z instrument-coverage` implies:
// * `-Z symbol-mangling-version=v0` - to ensure consistent and reversible name mangling.
// Note, LLVM coverage tools can analyze coverage over multiple runs, including some
// changes to source code; so mangled names must be consistent across compilations.
// * `-C link-dead-code` - so unexecuted code is still counted as zero, rather than be
// optimized out. Note that instrumenting dead code can be explicitly disabled with:
// `-Z instrument-coverage -C link-dead-code=no`.
// `-Z instrument-coverage` implies `-Z symbol-mangling-version=v0` - to ensure consistent
// and reversible name mangling. Note, LLVM coverage tools can analyze coverage over
// multiple runs, including some changes to source code; so mangled names must be consistent
// across compilations.
debugging_opts.symbol_mangling_version = SymbolManglingVersion::V0;
if cg.link_dead_code == None {
// FIXME(richkadel): Investigate if the `instrument-coverage` implementation can
// inject ["zero counters"](https://llvm.org/docs/CoverageMappingFormat.html#counter)
// in the coverage map when "dead code" is removed, rather than forcing `link-dead-code`.
cg.link_dead_code = Some(true);
}
}

if !cg.embed_bitcode {
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_session/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -885,9 +885,9 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
"instrument the generated code to support LLVM source-based code coverage \
reports (note, the compiler build config must include `profiler = true`, \
and is mutually exclusive with `-C profile-generate`/`-C profile-use`); \
implies `-C link-dead-code` (unless explicitly disabled)` and \
`-Z symbol-mangling-version=v0`; and disables/overrides some optimization \
options (default: no)"),
implies `-C link-dead-code` (unless targeting MSVC, or explicitly disabled) \
and `-Z symbol-mangling-version=v0`; disables/overrides some Rust \
optimizations (default: no)"),
instrument_mcount: bool = (false, parse_bool, [TRACKED],
"insert function instrument code for mcount-based tracing (default: no)"),
keep_hygiene_data: bool = (false, parse_bool, [UNTRACKED],
Expand Down
48 changes: 34 additions & 14 deletions compiler/rustc_session/src/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1021,6 +1021,40 @@ impl Session {
|| self.opts.debugging_opts.sanitizer.intersects(SanitizerSet::ADDRESS | SanitizerSet::MEMORY)
}

pub fn link_dead_code(&self) -> bool {
match self.opts.cg.link_dead_code {
Some(explicitly_set) => explicitly_set,
None => {
self.opts.debugging_opts.instrument_coverage
&& !self.target.target.options.is_like_msvc
// Issue #76038: (rustc `-Clink-dead-code` causes MSVC linker to produce invalid
// binaries when LLVM InstrProf counters are enabled). As described by this issue,
// the "link dead code" option produces incorrect binaries when compiled and linked
// under MSVC. The resulting Rust programs typically crash with a segmentation
// fault, or produce an empty "*.profraw" file (profiling counter results normally
// generated during program exit).
//
// If not targeting MSVC, `-Z instrument-coverage` implies `-C link-dead-code`, so
// unexecuted code is still counted as zero, rather than be optimized out. Note that
// instrumenting dead code can be explicitly disabled with:
//
// `-Z instrument-coverage -C link-dead-code=no`.
//
// FIXME(richkadel): Investigate if `instrument-coverage` implementation can inject
// [zero counters](https://llvm.org/docs/CoverageMappingFormat.html#counter) in the
// coverage map when "dead code" is removed, rather than forcing `link-dead-code`.
// This may not be possible, however, if (as it seems to appear) the "dead code"
// that would otherwise not be linked is only identified as "dead" by the native
// linker. If that's the case, I believe it is too late for the Rust compiler to
// leverage any information it might be able to get from the linker regarding what
// code is dead, to be able to add those counters.
//
// On the other hand, if any Rust compiler passes are optimizing out dead code blocks
// we should inject "zero" counters for those code regions.
}
}
}

pub fn mark_attr_known(&self, attr: &Attribute) {
self.known_attrs.lock().mark(attr)
}
Expand Down Expand Up @@ -1428,20 +1462,6 @@ fn validate_commandline_args_with_session_available(sess: &Session) {
);
}

// FIXME(richkadel): See `src/test/run-make-fulldeps/instrument-coverage/Makefile`. After
// compiling with `-Zinstrument-coverage`, the resulting binary generates a segfault during
// the program's exit process (likely while attempting to generate the coverage stats in
// the "*.profraw" file). An investigation to resolve the problem on Windows is ongoing,
// but until this is resolved, the option is disabled on Windows, and the test is skipped
// when targeting `MSVC`.
if sess.opts.debugging_opts.instrument_coverage && sess.target.target.options.is_like_msvc {
sess.warn(
"Rust source-based code coverage instrumentation (with `-Z instrument-coverage`) \
is not yet supported on Windows when targeting MSVC. The resulting binaries will \
still be instrumented for experimentation purposes, but may not execute correctly.",
);
}

const ASAN_SUPPORTED_TARGETS: &[&str] = &[
"aarch64-fuchsia",
"aarch64-unknown-linux-gnu",
Expand Down

0 comments on commit ddb054a

Please sign in to comment.