Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CFI information to __rust_probestack #305

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ test = false
core = { version = "1.0.0", optional = true, package = 'rustc-std-workspace-core' }

[build-dependencies]
cc = { optional = true, version = "1.0" }
cc = { version = "1.0" }

[dev-dependencies]
panic-handler = { path = 'crates/panic-handler' }
Expand All @@ -42,7 +42,7 @@ default = ["compiler-builtins"]

# Enable compilation of C code in compiler-rt, filling in some more optimized
# implementations and also filling in unimplemented intrinsics
c = ["cc"]
c = []

# Flag this library as the unstable compiler-builtins lib
compiler-builtins = []
Expand Down
16 changes: 16 additions & 0 deletions build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let target_arch = env::var("CARGO_CFG_TARGET_ARCH").unwrap();
if target_arch == "x86_64" {
let cfg = &mut cc::Build::new();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it makes sense to refactor that way.

cfg.file(&"src/probestack_x86_64.S");
cfg.compile("libcompiler-probestack.a");
}

if target_arch == "x86" {
let cfg = &mut cc::Build::new();
cfg.file(&"src/probestack_x86.S");
cfg.compile("libcompiler-probestack.a");
}
}

// NOTE we are going to assume that llvm-target, what determines our codegen option, matches the
// target triple. This is usually correct for our built-in targets but can break in presence of
// custom targets, which can have arbitrary names.
Expand Down
86 changes: 1 addition & 85 deletions src/probestack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,88 +41,4 @@
//! probes on any other architecture like ARM or PowerPC64. LLVM I'm sure would
//! be more than welcome to accept such a change!

#![cfg(not(windows))] // Windows already has builtins to do this

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Our goal here is to touch each page between %rsp+8 and %rsp+8-%rax,
// ensuring that if any pages are unmapped we'll make a page fault.
//
// The ABI here is that the stack frame size is located in `%eax`. Upon
// return we're not supposed to modify `%esp` or `%eax`.
asm!("
mov %rax,%r11 // duplicate %rax as we're clobbering %r11

// Main loop, taken in one page increments. We're decrementing rsp by
// a page each time until there's less than a page remaining. We're
// guaranteed that this function isn't called unless there's more than a
// page needed.
//
// Note that we're also testing against `8(%rsp)` to account for the 8
// bytes pushed on the stack orginally with our return address. Using
// `8(%rsp)` simulates us testing the stack pointer in the caller's
// context.

// It's usually called when %rax >= 0x1000, but that's not always true.
// Dynamic stack allocation, which is needed to implement unsized
// rvalues, triggers stackprobe even if %rax < 0x1000.
// Thus we have to check %r11 first to avoid segfault.
cmp $$0x1000,%r11
jna 3f
2:
sub $$0x1000,%rsp
test %rsp,8(%rsp)
sub $$0x1000,%r11
cmp $$0x1000,%r11
ja 2b

3:
// Finish up the last remaining stack space requested, getting the last
// bits out of r11
sub %r11,%rsp
test %rsp,8(%rsp)

// Restore the stack pointer to what it previously was when entering
// this function. The caller will readjust the stack pointer after we
// return.
add %rax,%rsp

ret
" ::: "memory" : "volatile");
::core::intrinsics::unreachable();
}

#[naked]
#[no_mangle]
#[cfg(all(target_arch = "x86", not(feature = "mangled-names")))]
pub unsafe extern "C" fn __rust_probestack() {
// This is the same as x86_64 above, only translated for 32-bit sizes. Note
// that on Unix we're expected to restore everything as it was, this
// function basically can't tamper with anything.
//
// The ABI here is the same as x86_64, except everything is 32-bits large.
asm!("
push %ecx
mov %eax,%ecx

cmp $$0x1000,%ecx
jna 3f
2:
sub $$0x1000,%esp
test %esp,8(%esp)
sub $$0x1000,%ecx
cmp $$0x1000,%ecx
ja 2b

3:
sub %ecx,%esp
test %esp,8(%esp)

add %eax,%esp
pop %ecx
ret
" ::: "memory" : "volatile");
::core::intrinsics::unreachable();
}
// The code here moved to probestack_*.S, to support debugger frame information.
38 changes: 38 additions & 0 deletions src/probestack_x86.S
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// This is the same as x86_64, only translated for 32-bit sizes. Note that on
// Unix we're expected to restore everything as it was, this function basically
// can't tamper with anything.
//
// The ABI here is the same as x86_64, except everything is 32-bits large.

.text
.globl __rust_probestack
.type __rust_probestack, @function
__rust_probestack:
.cfi_startproc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@bjorn3 bjorn3 Jul 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pushq %ebp
.cfi_def_cfa_offset 8
.cfi_offset 6, -8
movq %esp, %ebp
.cfi_def_cfa_register 6
push %ecx
mov %eax,%ecx

cmp $0x1000,%ecx
jna 3f
2:
sub $0x1000,%esp
test %esp,8(%esp)
sub $0x1000,%ecx
cmp $0x1000,%ecx
ja 2b

3:
sub %ecx,%esp
test %esp,8(%esp)

add %eax,%esp
pop %ecx
leave
.cfi_def_cfa 7, 8
ret
.cfi_endproc
57 changes: 57 additions & 0 deletions src/probestack_x86_64.S
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// Our goal here is to touch each page between %rsp+8 and %rsp+8-%rax,
// ensuring that if any pages are unmapped we'll make a page fault.
//
// The ABI here is that the stack frame size is located in `%eax`. Upon
// return we're not supposed to modify `%esp` or `%eax`.

.text
.p2align 4,,15
.globl __rust_probestack
.type __rust_probestack, @function
__rust_probestack:
.cfi_startproc
pushq %rbp
.cfi_def_cfa_offset 16
.cfi_offset 6, -16
movq %rsp, %rbp
.cfi_def_cfa_register 6
mov %rax,%r11
// duplicate %rax as we're clobbering %r11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Main loop, taken in one page increments. We're decrementing rsp by
// a page each time until there's less than a page remaining. We're
// guaranteed that this function isn't called unless there's more than a
// page needed.
//
// Note that we're also testing against `8(%rsp)` to account for the 8
// bytes pushed on the stack orginally with our return address. Using
// `8(%rsp)` simulates us testing the stack pointer in the caller's
// context.

// It's usually called when %rax >= 0x1000, but that's not always true.
// Dynamic stack allocation, which is needed to implement unsized
// rvalues, triggers stackprobe even if %rax < 0x1000.
// Thus we have to check %r11 first to avoid segfault.
cmp $0x1000,%r11
jna 3f
2:
sub $0x1000,%rsp
test %rsp,8(%rsp)
sub $0x1000,%r11
cmp $0x1000,%r11
ja 2b

3:
// Finish up the last remaining stack space requested, getting the last
// bits out of r11
sub %r11,%rsp
test %rsp,8(%rsp)

// Restore the stack pointer to what it previously was when entering
// this function. The caller will readjust the stack pointer after we
// return.
add %rax,%rsp
leave
.cfi_def_cfa 7, 8
ret
.cfi_endproc