Rustc adds line-number information for unhittable panic handlers

[bossmc]

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...?

[bossmc]

Simple repro in https://github.com/bossmc/kcov-rust-issue.

Reproduce with cargo kcov (from https://github.com/kennytm/cargo-kcov). Note that the closing brace is marked as uncovered unless the second test is enabled.

[pnkfelix]

This still reproduces.

I want to dig in with a little private investigation to understand the scenarios where the cleanup path is marked as unreached, and to know how one should be expected to avoid such marking in cases like this (e.g. should we require a #[no_panic] attribute on the function signature of fn get_age to signal that tools can assume it doesn't panic?)

I also want to understand the scenarios where people want to know about code paths that were silently injected by the compiler. I do believe there are cases where people want to know about that, but I also think the majority of programmers are trying to avoid actively thinking about cleanup paths, and for the most part we want to encourage that...

[bossmc]

There are two "markings" at play here:

1. The compiler marking addresses with debug information (mainly intended for consumption by gdb/lldb/etc.
2. KCov marking lines of source code as hit/unhit

Which are you asking about? Are you exploring if Rustc can detect the unhittable landing pads and not emit debug information for them? Or if kcov can tell if a given debug info line corresponds to a landing pad and therefore not report it?

[pnkfelix]

I'm asking about the expected/desired experience for an end-developer. I expect some people are going to want to test the control-flow paths that correspond to panics for certain library routines. But I also expect some set of people to treat some panic paths as something they do not attempt to test (because, for example, they are truly unreachable -- as is the case in the situation outlined in this issue).

What is the right way to serve both groups? Is it to ask people to add certain attributes to their source code (and then rustc incorporates those attributes into the metadata that it generates for kcov)? Or should we expect the source code in both of these scenarios to remain unchanged?

(To be clear: @bossmc 's original description included bullet points that explore this question with ideas of paths to follow. I just want to start from a higher level: To ask what the ideal experience should be in the two scenarios I outlined, and then try to work backwards from there.)

[bjorn3]

Is this also an issue when using -C instrument-coverage?

[bossmc]

Although the motivation for investigating this issue was coverage (specifically kcov-based coverage) the underlying issue is that a debug rust binary will contain debug line information for unhittable (and unwritten) code, which may be potentially confusing for other tools that use dwarf information to navigate binaries.

Looking specifically at the coverage question, instrument-coverage does not report if cleanup blocks were entered or not, this is a more consistent position than kcov-based coverage, though as @pnkfelix asks, it's not clear if coverage should count (hittable) cleanup blocks (treating them as branches in the processing flow).

Confirming that instrument-coverage ignores landing pads

Analysing my example crate gives:

$ rustc -C instrument-coverage --emit llvm-ir src/lib.rs
$ cat lib.ll
...
; lib::make_a::<lib::main::get_age>
; Function Attrs: nonlazybind uwtable
define internal void @_RINvCs1a1nQsqUXE0_3lib6make_aNvNvB2_4main7get_ageEB2_(ptr sret([32 x i8]) align 8 %_0) unnamed_addr #0 personality ptr @rust_eh_personality {
start:
  %0 = alloca [16 x i8], align 8
  %_2 = alloca [24 x i8], align 8
  %f = alloca [0 x i8], align 1
  ; *** HERE ***
  %1 = atomicrmw add ptr @__profc__RINvCs1a1nQsqUXE0_3lib6make_aNvNvB2_4main7get_ageEB2_, i64 1 monotonic, align 8
; invoke <str as alloc::string::ToString>::to_string
  invoke void @_RNvXsw_NtCs7h7qHWfF5BF_5alloc6stringeNtB5_8ToString9to_stringCs1a1nQsqUXE0_3lib(ptr sret([24 x i8]) align 8 %_2, ptr align 1 @alloc_61704a7a910a0689d4cb1341b8da12d1, i64 5)
          to label %bb1 unwind label %cleanup

bb5:                                              ; preds = %bb4, %cleanup
  %2 = load ptr, ptr %0, align 8
  %3 = getelementptr inbounds i8, ptr %0, i64 8
  %4 = load i32, ptr %3, align 8
  %5 = insertvalue { ptr, i32 } poison, ptr %2, 0
  %6 = insertvalue { ptr, i32 } %5, i32 %4, 1
  resume { ptr, i32 } %6

cleanup:                                          ; preds = %start
  %7 = landingpad { ptr, i32 }
          cleanup
  %8 = extractvalue { ptr, i32 } %7, 0
  %9 = extractvalue { ptr, i32 } %7, 1
  store ptr %8, ptr %0, align 8
  %10 = getelementptr inbounds i8, ptr %0, i64 8
  store i32 %9, ptr %10, align 8
  br label %bb5

bb1:                                              ; preds = %start
; invoke <lib::main::get_age as core::ops::function::Fn<()>>::call
  %_4 = invoke i32 @_RNvYNvNvCs1a1nQsqUXE0_3lib4main7get_ageINtNtNtCs649FzoMgw5V_4core3ops8function2FnuE4callB6_(ptr align 1 %f)
          to label %bb2 unwind label %cleanup1

bb4:                                              ; preds = %cleanup1
; invoke core::ptr::drop_in_place::<alloc::string::String>
  invoke void @_RINvNtCs649FzoMgw5V_4core3ptr13drop_in_placeNtNtCs7h7qHWfF5BF_5alloc6string6StringECs1a1nQsqUXE0_3lib(ptr align 8 %_2) #15
          to label %bb5 unwind label %terminate

cleanup1:                                         ; preds = %bb1
  %11 = landingpad { ptr, i32 }
          cleanup
  %12 = extractvalue { ptr, i32 } %11, 0
  %13 = extractvalue { ptr, i32 } %11, 1
  store ptr %12, ptr %0, align 8
  %14 = getelementptr inbounds i8, ptr %0, i64 8
  store i32 %13, ptr %14, align 8
  br label %bb4

bb2:                                              ; preds = %bb1
  call void @llvm.memcpy.p0.p0.i64(ptr align 8 %_0, ptr align 8 %_2, i64 24, i1 false)
  %15 = getelementptr inbounds i8, ptr %_0, i64 24
  store i32 %_4, ptr %15, align 8
  ret void

terminate:                                        ; preds = %bb4
  %16 = landingpad { ptr, i32 }
          filter [0 x ptr] zeroinitializer
  %17 = extractvalue { ptr, i32 } %16, 0
  %18 = extractvalue { ptr, i32 } %16, 1
; call core::panicking::panic_in_cleanup
  call void @_ZN4core9panicking16panic_in_cleanup17h7e5529b9cf989fd4E() #16
  unreachable
}

The only line that is counted for instrument-coverage-based coverage is the one marked *** HERE ***, neither the landing pads (nor the continuations) are individually instrumented so the coverage map and tooling can't say anything about whether they were hit or not.