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 control flow information to __rust_probestack #328

Merged
merged 1 commit into from
Dec 6, 2019

Conversation

tmandry
Copy link
Member

@tmandry tmandry commented Dec 4, 2019

This adds control flow information (CFI) on x86 and x86_64 platforms, so that DWARF-based backtraces work when the stack probe fails.

The implementation builds on the work done in #305 and #306. It manages to keep the CFI in inline assembly by defining a "function within a function." This has been done elsewhere; see e.g. this LibFuzzer code.

src/probestack.rs Outdated Show resolved Hide resolved
src/probestack.rs Outdated Show resolved Hide resolved
src/probestack.rs Outdated Show resolved Hide resolved
src/probestack.rs Outdated Show resolved Hide resolved
src/probestack.rs Outdated Show resolved Hide resolved
src/probestack.rs Outdated Show resolved Hide resolved
@tmandry
Copy link
Member Author

tmandry commented Dec 5, 2019

Unfortunately the mach-o assembler doesn't support the directives I'm using to keep the assembly inline. I don't know how to express what I want to in that assembler (it doesn't like some of the assembly used in #305 either).

That means we'll have to split the implementations for Mac out :(

@tmandry
Copy link
Member Author

tmandry commented Dec 5, 2019

Actually, it seems #305's implementation did compile on apple except for the .type __rust_probestack, @function directive. I'm not sure what the implication would be of removing it, but if we can do that then it's probably best to go with that approach (which would require using external assembly files).

There doesn't seem to be any substitute for .pushsection/.popsection in Apple's assembler, and I don't know how to keep the assembly inline without those.

EDIT: I actually think there's still a way of doing it inline, so I'll play with that.

@tmandry tmandry force-pushed the probestack-cfi branch 6 times, most recently from 66b1486 to c7b64af Compare December 5, 2019 03:13
@tmandry
Copy link
Member Author

tmandry commented Dec 5, 2019

Hmm, so wrapping our inner implementation in .cfi_endproc/.cfi_startproc is required in debug builds, but fails in release builds. Not doing this succeeds in release builds, but fails in debug builds.

It seems like the enclosing .cfi_startproc/.cfi_endproc of __rust_probestack_wrapper gets "optimized out" in release builds. Not sure what to do about this yet.

.pushsection .text.__rust_probestack
.globl __rust_probestack
.type __rust_probestack, @function
__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 believe on Mach-O the linker would be allowed to move this symbol and following content to another location, as it uses symbols to determine the start and end of every function.

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 think that would be fine. __rust_probestack is the function that gets called, not __rust_probestack_wrapper.

@bjorn3
Copy link
Member

bjorn3 commented Dec 5, 2019

Maybe use global_asm! instead of inline asm?

@tmandry
Copy link
Member Author

tmandry commented Dec 5, 2019

Maybe use global_asm! instead of inline asm?

I didn't know about global_asm!, thanks! Hopefully that can get me out of this startproc/endproc mess.

@alexcrichton
Copy link
Member

Now that's an impressive set of green :)

I think it'd be good to hook this up into rust-lang/rust before merging though (via [patch] most likely). Would you be ok running the test suite locally with this merged into make sure it's all still working at least at a high level?

@tmandry tmandry changed the title [WIP] Add control flow information to __rust_probestack Add control flow information to __rust_probestack Dec 6, 2019
@tmandry
Copy link
Member Author

tmandry commented Dec 6, 2019

Done! Had to make one change, now all tests pass on both linux and mac.

@alexcrichton alexcrichton merged commit 2566aa6 into rust-lang:master Dec 6, 2019
@alexcrichton
Copy link
Member

👍

@tmandry tmandry deleted the probestack-cfi branch December 6, 2019 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants