Skip to content

Commit

Permalink
WIP attempt to add coverage 0 of dead blocks
Browse files Browse the repository at this point in the history
...by saving dead counters/expressions.

Unfortunately, I'm still getting the same result, when trying to get
coverage from Fuchsia apps (at least those using fuchsia_async, but that
may not be the specific cause):

When running `llvm-cov` to get the coverage report, I get the well known
and still useless message:

```
Failed to load coverage: Malformed instrumentation profile data
```

I can change CoverageMappingReader.cpp to avoid failing and I will see
some coverage results (maybe most of the coverage results) but there is
missing coverage as well. I don't know why the function name is not
found.

The "hack" is, change this:

```c++
      if (Error Err = CFR->template getFuncName<Endian>(ProfileNames, FuncName))
        return Err;
      if (FuncName.empty())
        return make_error<InstrProfError>(instrprof_error::malformed);
      ++CovMapNumUsedRecords;
```

to this:

```c++
      if (Error Err = CFR->template getFuncName<Endian>(ProfileNames, FuncName))
        return Err;
      if (FuncName.empty())
        FuncName = "MissingFuncNameForCoverage";
      ++CovMapNumUsedRecords;
```

I did learn that the original implementation (replacing counters and
expressions with Zero/unreachable counters), so this PR addresses that
issue. I hoped that it would fix the problem, but it didn't.

Specifically regarding the LLVM coverage adjustment (improvement?) made
in this version, compared to the reverted PR (rust-lang#84797):

LLVM requires all counters with code regions be added to the coverage
map. The original dead block fix replaced the counters and expressions
with a Zero (unreachable) code region. This commit saves the original
counter or expression, without adding a statement to increment the
counter for the eliminated dead block.
  • Loading branch information
richkadel committed May 12, 2021
1 parent 4e4751b commit ebf1e00
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 32 deletions.
11 changes: 10 additions & 1 deletion compiler/rustc_codegen_ssa/src/coverageinfo/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ pub struct Expression {
/// only whitespace or comments). According to LLVM Code Coverage Mapping documentation, "A count
/// for a gap area is only used as the line execution count if there are no other regions on a
/// line."
#[derive(Debug)]
pub struct FunctionCoverage<'tcx> {
instance: Instance<'tcx>,
source_hash: u64,
Expand All @@ -51,7 +52,7 @@ impl<'tcx> FunctionCoverage<'tcx> {
fn create(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>, is_used: bool) -> Self {
let coverageinfo = tcx.coverageinfo(instance.def_id());
debug!(
"FunctionCoverage::new(instance={:?}) has coverageinfo={:?}. is_used={}",
"FunctionCoverage::create(instance={:?}) has coverageinfo={:?}. is_used={}",
instance, coverageinfo, is_used
);
Self {
Expand Down Expand Up @@ -113,6 +114,14 @@ impl<'tcx> FunctionCoverage<'tcx> {
expression_id, lhs, op, rhs, region
);
let expression_index = self.expression_index(u32::from(expression_id));
debug_assert!(
expression_index.as_usize() < self.expressions.len(),
"expression_index {} is out of range for expressions.len() = {}
for {:?}",
expression_index.as_usize(),
self.expressions.len(),
self,
);
if let Some(previous_expression) = self.expressions[expression_index].replace(Expression {
lhs,
op,
Expand Down
36 changes: 23 additions & 13 deletions compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,28 +20,38 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {

let Coverage { kind, code_region } = coverage;
match kind {
CoverageKind::Counter { function_source_hash, id } => {
CoverageKind::Counter { function_source_hash, id, is_dead } => {
if bx.set_function_source_hash(instance, function_source_hash) {
// If `set_function_source_hash()` returned true, the coverage map is enabled,
// so continue adding the counter.
if let Some(code_region) = code_region {
// Note: Some counters do not have code regions, but may still be referenced
// from expressions. In that case, don't add the counter to the coverage map,
// but do inject the counter intrinsic.
// from expressions. In that case, don't add the counter to the coverage
// map, but do inject the counter intrinsic.
if is_dead {
debug!(
"Adding coverage counter for dead code region: instance={:?},
function_source_hash={:?}, id={:?}, code_region={:?}",
instance, function_source_hash, id, code_region
);
}
bx.add_coverage_counter(instance, id, code_region);
}

let coverageinfo = bx.tcx().coverageinfo(instance.def_id());
if !is_dead {
let coverageinfo = bx.tcx().coverageinfo(instance.def_id());

let fn_name = bx.get_pgo_func_name_var(instance);
let hash = bx.const_u64(function_source_hash);
let num_counters = bx.const_u32(coverageinfo.num_counters);
let index = bx.const_u32(id.zero_based_index());
debug!(
"codegen intrinsic instrprof.increment(fn_name={:?}, hash={:?}, num_counters={:?}, index={:?})",
fn_name, hash, num_counters, index,
);
bx.instrprof_increment(fn_name, hash, num_counters, index);
let fn_name = bx.get_pgo_func_name_var(instance);
let hash = bx.const_u64(function_source_hash);
let num_counters = bx.const_u32(coverageinfo.num_counters);
let index = bx.const_u32(id.zero_based_index());
debug!(
"codegen intrinsic instrprof.increment(fn_name={:?}, hash={:?},
num_counters={:?}, index={:?})",
fn_name, hash, num_counters, index,
);
bx.instrprof_increment(fn_name, hash, num_counters, index);
}
}
}
CoverageKind::Expression { id, lhs, op, rhs } => {
Expand Down
25 changes: 20 additions & 5 deletions compiler/rustc_middle/src/mir/coverage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,16 +102,25 @@ impl From<InjectedExpressionId> for ExpressionOperandId {

#[derive(Clone, PartialEq, TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable)]
pub enum CoverageKind {
Counter {
function_source_hash: u64,
id: CounterValueReference,
},
/// A live `Counter` will codegen statements to increment the counter when
/// its block is executed at runtime. A dead counter will not codegen
/// anything, but will still be added to the coverage map, so it can still
/// be referenced by expressions, and its code region will show up as
/// uncovered.
Counter { function_source_hash: u64, id: CounterValueReference, is_dead: bool },

/// A counter whose value can be computed when generating a coverage report.
/// This statement does not codegen anything, but the expression and code
/// region are added to the coverage map.
Expression {
id: InjectedExpressionId,
lhs: ExpressionOperandId,
op: Op,
rhs: ExpressionOperandId,
},

/// A code region that is never executed, and is never part of an
/// expression.
Unreachable,
}

Expand All @@ -134,7 +143,13 @@ impl Debug for CoverageKind {
fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result {
use CoverageKind::*;
match self {
Counter { id, .. } => write!(fmt, "Counter({:?})", id.index()),
Counter { id, is_dead, .. } => {
if *is_dead {
write!(fmt, "Counter({:?}/dead)", id.index())
} else {
write!(fmt, "Counter({:?})", id.index())
}
}
Expression { id, lhs, op, rhs } => write!(
fmt,
"Expression({:?}) = {} {} {}",
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_mir/src/transform/coverage/counters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ impl CoverageCounters {
let counter = CoverageKind::Counter {
function_source_hash: self.function_source_hash,
id: self.next_counter(),
is_dead: false,
};
if self.debug_counters.is_enabled() {
self.debug_counters.add_counter(&counter, (debug_block_label_fn)());
Expand Down
24 changes: 11 additions & 13 deletions compiler/rustc_mir/src/transform/simplify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,24 +329,22 @@ fn save_unreachable_coverage(
// report `0` executions for the uncovered code regions.
let mut dropped_coverage = Vec::new();
for dead_block in first_dead_block..basic_blocks.len() {
for statement in basic_blocks[BasicBlock::new(dead_block)].statements.iter() {
if let StatementKind::Coverage(coverage) = &statement.kind {
if let Some(code_region) = &coverage.code_region {
dropped_coverage.push((statement.source_info, code_region.clone()));
}
for statement in basic_blocks[BasicBlock::new(dead_block)].statements.drain(..) {
if let StatementKind::Coverage(box coverage) = statement.kind {
dropped_coverage.push((statement.source_info, coverage));
}
}
}
for (source_info, code_region) in dropped_coverage {
basic_blocks[START_BLOCK].statements.push(Statement {
source_info,
kind: StatementKind::Coverage(box Coverage {
kind: CoverageKind::Unreachable,
code_region: Some(code_region),
}),
})
for (source_info, mut coverage) in dropped_coverage {
if let CoverageKind::Counter { ref mut is_dead, .. } = coverage.kind {
*is_dead = true;
}
basic_blocks[START_BLOCK]
.statements
.push(Statement { source_info, kind: StatementKind::Coverage(box coverage) })
}
}

pub struct SimplifyLocals;

impl<'tcx> MirPass<'tcx> for SimplifyLocals {
Expand Down

0 comments on commit ebf1e00

Please sign in to comment.