Add CFI information to __rust_probestack #305

da-x · 2019-07-23T07:24:07Z

In order for GDB and other stack unwinders to correctly backtrace a stack overflow, it needs CFI information in __rust_probestack.

This turns the following backtrace,

>> bt
 #0  0x0000555555576f73 in __rust_probestack () at /cargo/registry/src/github.com-1ecc6299db9ec823/compiler_builtins-0.1.14/src/probestack.rs:55
Backtrace stopped: Cannot access memory at address 0x7fffff7fedf0

Into this:

>>> bt
 #0  0x0000555555574e47 in __rust_probestack ()
 #1  0x00005555555595ba in test::main ()
 #2  0x00005555555594f3 in std::rt::lang_start::{{closure}} ()
 #3  0x0000555555561ae3 in std::panicking::try::do_call ()
 #4  0x000055555556595a in __rust_maybe_catch_panic ()
 #5  0x000055555555af9b in std::rt::lang_start_internal ()
 #6  0x00005555555594d5 in std::rt::lang_start ()
 #7  0x000055555555977b in main ()

Fixes #304.

da-x · 2019-07-23T07:25:17Z

I've done some work to test this with a patched rustc, having compiler_builtins = { path = "src/libcompiler_builtins" }, and it seems to work.

In order for GDB to correctly backtrace a stack overflow, it needs CFI information in __rust_probestack. This turns the following backtrace, ``` >> bt #0 0x0000555555576f73 in __rust_probestack () at /cargo/registry/src/github.com-1ecc6299db9ec823/compiler_builtins-0.1.14/src/probestack.rs:55 Backtrace stopped: Cannot access memory at address 0x7fffff7fedf0 ``` To this: ``` >>> bt #0 0x0000555555574e47 in __rust_probestack () rust-lang#1 0x00005555555595ba in test::main () rust-lang#2 0x00005555555594f3 in std::rt::lang_start::{{closure}} () rust-lang#3 0x0000555555561ae3 in std::panicking::try::do_call () rust-lang#4 0x000055555556595a in __rust_maybe_catch_panic () rust-lang#5 0x000055555555af9b in std::rt::lang_start_internal () rust-lang#6 0x00005555555594d5 in std::rt::lang_start () rust-lang#7 0x000055555555977b in main () ```

bjorn3 · 2019-07-23T08:12:59Z

src/probestack_x86_64.S

+    movq    %rsp, %rbp
+    .cfi_def_cfa_register 6
+    mov    %rax,%r11
+    // duplicate %rax as we're clobbering %r11


Please move this comment one line up. In is about the mov above.

alexcrichton · 2019-07-23T14:26:42Z

build.rs

@@ -28,6 +28,22 @@ fn main() {
        println!("cargo:rustc-cfg=feature=\"mem\"");
    }

+    let target_os = env::var("CARGO_CFG_TARGET_OS").unwrap();
+    if target_os != "windows" {


I think it's probably fine to just check the target local variable like the rest of this function

alexcrichton · 2019-07-23T14:27:12Z

build.rs

+    if target_os != "windows" {
+        let target_arch = env::var("CARGO_CFG_TARGET_ARCH").unwrap();
+        if target_arch == "x86_64" {
+            let cfg = &mut cc::Build::new();


This compilation will want to be in the c module as well, and if possible it'd be great to use the same compilation support that all the other C intrinsics use below.

Indeed, it makes sense to refactor that way.

alexcrichton · 2019-07-23T14:27:42Z

src/probestack.rs

-#[naked]
-#[no_mangle]
-#[cfg(all(target_arch = "x86_64", not(feature = "mangled-names")))]
-pub unsafe extern "C" fn __rust_probestack() {


I don't think we'll want to delete these because a C compiler isn't always available and we'll still want definitions of these functions.

Actually, we wouldn't really need C because we are compiling assembly files. Passing '.S' files to a C compiler is just wrapping around an assembler. Can we rely solely on our bundled-in LLVM for it somehow?

Nah unfortunately we don't ship an assembler that uses LLVM, so we're left basically looking for cc. You're right that we otherwise just need as, but that's not actually that much easier to acquire than a C compiler most of the time.

alexcrichton · 2019-07-23T14:29:27Z

src/probestack_x86.S

+.globl  __rust_probestack
+.type   __rust_probestack, @function
+__rust_probestack:
+    .cfi_startproc


To confirm, are we certain that this can't be done with LLVM's inline assembly? For example does updating ebp/rbp work for gdb/backtraces? Does LLVM inline assembly not support the .cfi_* directives?

(in general it'll just be easier if we can stick to inline assembly I think)

I've tried adding the CFI directives both in the original inline assembly, also via global_asm!, and both cases they did not result in a binary containing the directives, as observed via dwarfdump. Only a separate compilation worked. I haven't tested yet whether gdb picks on the ebp/rbp usage without the directives.

gdb is very picky about what kind of prelude it accepts. Even placing a rex prefix before the push breaks it: https://github.com/bjorn3/rustc_codegen_cranelift/issues/146#issuecomment-449474527. Because of this, I wouldnt be suprised if using a different register as base pointer would confuse gdb.

I think we can use the standard base register prelude here, and if that works I think we probably want to stick with that since we should be able to continue using inline assembly.

@da-x want to test on your end with the base pointer management added but still using inline assembly and seeing if it works?

It works as expected. Please consider #306 and close this PR.

alexcrichton · 2019-07-24T14:04:27Z

Indeed I'm gonna close this in favor of #306, thanks for investigating!

bors · 2019-07-24T14:04:28Z

☔ The latest upstream changes (presumably #306) made this pull request unmergeable. Please resolve the merge conflicts.

da-x force-pushed the cfi-information branch 4 times, most recently from a7e6876 to e67619c Compare July 23, 2019 07:42

da-x force-pushed the cfi-information branch from e67619c to e32930d Compare July 23, 2019 07:50

bjorn3 reviewed Jul 23, 2019

View reviewed changes

da-x added 2 commits July 23, 2019 11:55

probestack_x86_64.S: move a comment line

ce93768

src/probestack_x86.S: opcode fixups

1afd66a

alexcrichton reviewed Jul 23, 2019

View reviewed changes

alexcrichton closed this Jul 24, 2019

tmandry mentioned this pull request Nov 14, 2019

CFI directives missing on __rust_probestack #325

Closed

tmandry mentioned this pull request Dec 5, 2019

Add control flow information to __rust_probestack #328

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CFI information to __rust_probestack #305

Add CFI information to __rust_probestack #305

da-x commented Jul 23, 2019

da-x commented Jul 23, 2019

bjorn3 Jul 23, 2019

alexcrichton Jul 23, 2019

alexcrichton Jul 23, 2019

da-x Jul 23, 2019

alexcrichton Jul 23, 2019

da-x Jul 23, 2019

alexcrichton Jul 23, 2019

alexcrichton Jul 23, 2019

da-x Jul 23, 2019

bjorn3 Jul 23, 2019 •

edited

Loading

alexcrichton Jul 23, 2019

da-x Jul 24, 2019

alexcrichton commented Jul 24, 2019

bors commented Jul 24, 2019

Add CFI information to __rust_probestack #305

Add CFI information to __rust_probestack #305

Conversation

da-x commented Jul 23, 2019

da-x commented Jul 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn3 Jul 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jul 24, 2019

bors commented Jul 24, 2019

bjorn3 Jul 23, 2019 •

edited

Loading