Repository: rust
Issue: Rustc adds line-number information for unhittable panic handlers
Labels: A-debuginfo,T-compiler

_First off, root issue I was investigating was kcov producing bad coverage information for Rust binaries.  I've worked out why kcov is producing the results it is and they're "correct" given what Rustc is doing.  I'm not sure where the fix (if any) needs to be made, but I'm starting with Rustc as kcov's strategy looks sound._

Consider the following code:

```
struct Person {
  name: String,
  age: u32,
}

fn get_age() -> u32 {
  42
}

fn create_bob() -> Person {
  Person {
    name: "Bob".to_string(),
    age: get_age(),
  }  // Uncovered
}

fn main() {
  let b = create_bob();
}
```

Kcov will mark the "Uncovered" line as unhit.  This is surprising, as that line was definitely passed during the execution of the program.  Lines can be omitted from kcov coverage (with `// KCOV_EXCL_LINE`) but this leads to hundreds of those markers scattered around the code, potentially hiding real coverage lapses/lowering maintainability of the codebase.

----

Kcov determines coverage by looking at the `.debug_lines` section of the binary, which contains a mapping from address in the binary to line of code.  It then sets a break point at every listed address before running the program.  Each time a break point is hit, kcov marks the associated line of code as hit, and clears the breakpoint (as hitting it again tells us nothing, and breakpoints are slow).  After the process completes, any lines remaining were not hit.

This means that kcov's "was this line hit" logic is pretty solid, assuming the `.debug_lines` section is accurate.

----

When Rust generates a block that calls any function after creating any binding to a type with a `Drop` implementation, it also generates a unwind-cleanup block to call that `drop()` in the event of the function `panic`-ing.  In the above case, the generated code is something like (assuming Rust had exceptions):

```
fn create_bob() -> Person {
  let name = "Bob".to_string();
  try {
    let age = get_age();
  } catch e {
    drop(name);
    throw e;
  }
  Person {
    name,
    age,
  }
}
```

This makes sense, if there is a panic (which might get handled up the stack somewhere) we should delete bindings that are memory-safe (yay Rust!) to free up memory that's not going to be accessible any more.  

----

Unfortunately:

* The cleanup code is associated with the "end of block" marker (the `}` line marked above) so unless `get_age` panics, this code won't be hit.
* The marked line is not associated with any other generated machine code, so nothing else will cause kcov to consider it hit
* `get_age` never panics so this cleanup code can't be hit (without changing the `get_age()` function at least)

In a release build, the cleanup handler is stripped because LLVM notices that there's no way for anything in the `try` block to panic, so the handler is not needed.  In a debug build, it doesn't do this optimisation and leaves genuinely unhittable code in the binary, but associates it with a line of code, causing many false positives in kcov's output (especially as kcov uses debug builds to prevent dead-code elimination removing untested code).

----

What to do differently?  I'm not sure, but here are some ideas:

* Associate the cleanup of a binding with the line that created the binding, rather than the end of the block.  Might be confusing if you are actually debugging a panic cleanup.
* Don't associate the cleanup code with any lines of code.  Definitely unhelpful in panic debugging.
* Enhance Kcov to ignore cleanup landing pads.  Isn't really fair, as in a language like C++ exceptions are fairly common and cleanup is important.  Also, landing pads are language-specific (see eh_personality and friends) so it's tricky to get this right.
* Somehow perform minimal dead-code elimination even in debug builds (or test builds more accurately) that only removes unnecessary cleanup code.  Not sure how plausible this is, nor how likely it is to create false positives.
* Something else...?