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

Suppress debuginfo on naked function arguments #74105

Merged
merged 1 commit into from
Jul 30, 2020
Merged

Conversation

npmccallum
Copy link
Contributor

A function that has no prologue cannot be reasonably expected to support
debuginfo. In fact, the existing code (before this patch) would generate
invalid instructions that caused crashes. We can solve this easily by
just not emitting the debuginfo in this case.

Fixes #42779
cc #32408

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 6, 2020
@matthewjasper
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 8, 2020

📌 Commit 6b59cac has been approved by matthewjasper

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 8, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 8, 2020
Suppress debuginfo on naked function arguments

A function that has no prologue cannot be reasonably expected to support
debuginfo. In fact, the existing code (before this patch) would generate
invalid instructions that caused crashes. We can solve this easily by
just not emitting the debuginfo in this case.

Fixes rust-lang#42779
cc rust-lang#32408
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 8, 2020
Suppress debuginfo on naked function arguments

A function that has no prologue cannot be reasonably expected to support
debuginfo. In fact, the existing code (before this patch) would generate
invalid instructions that caused crashes. We can solve this easily by
just not emitting the debuginfo in this case.

Fixes rust-lang#42779
cc rust-lang#32408
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 10, 2020
Suppress debuginfo on naked function arguments

A function that has no prologue cannot be reasonably expected to support
debuginfo. In fact, the existing code (before this patch) would generate
invalid instructions that caused crashes. We can solve this easily by
just not emitting the debuginfo in this case.

Fixes rust-lang#42779
cc rust-lang#32408
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 10, 2020
Suppress debuginfo on naked function arguments

A function that has no prologue cannot be reasonably expected to support
debuginfo. In fact, the existing code (before this patch) would generate
invalid instructions that caused crashes. We can solve this easily by
just not emitting the debuginfo in this case.

Fixes rust-lang#42779
cc rust-lang#32408
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 10, 2020
Suppress debuginfo on naked function arguments

A function that has no prologue cannot be reasonably expected to support
debuginfo. In fact, the existing code (before this patch) would generate
invalid instructions that caused crashes. We can solve this easily by
just not emitting the debuginfo in this case.

Fixes rust-lang#42779
cc rust-lang#32408
Comment on lines +804 to +815
// Emit function argument debuginfo only for non-naked functions.
// See: https://github.com/rust-lang/rust/issues/42779
if naked {
continue;
}
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 this is a good idea, wouldn't be safer to just disable all debuginfo generation for #[naked] functions in codegen?

Copy link
Member

Choose a reason for hiding this comment

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

Also this PR doesn't add a mir-opt test showing the lack of MIR debuginfo (but which we might not want anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eddyb We don't want to suppress the debuginfo describing the calling contract of the function, just the internal function operations. The lldb can reference this information to show the contents of (for example) the first argument in %rdi. But if you think we need to implement this another way, I'm not particular.

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 10, 2020
Suppress debuginfo on naked function arguments

A function that has no prologue cannot be reasonably expected to support
debuginfo. In fact, the existing code (before this patch) would generate
invalid instructions that caused crashes. We can solve this easily by
just not emitting the debuginfo in this case.

Fixes rust-lang#42779
cc rust-lang#32408
@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 11, 2020
@npmccallum
Copy link
Contributor Author

@Manishearth Can you help me out? I'm not sure what to do here. I suspect there is some problem with the test. But I have no idea how to set up arm-android to test it.

@Manishearth
Copy link
Member

I'm sorry, I have no idea

@npmccallum
Copy link
Contributor Author

@Manishearth I spent ~6 hours today trying to get an arm-android environment running to debug the test, but I failed. The latest version of this patch is a practical solution: split debuginfo testing for naked functions into a separate test and ignore-android.

@Manishearth
Copy link
Member

I'm not the reviewer, you have to convince @matthewjasper or someone else on the compiler team that this is acceptable

@matthewjasper
Copy link
Contributor

The hang appears to happen in master as well, r=me with an issue opened and referenced in the test for this.

@npmccallum
Copy link
Contributor Author

@matthewjasper I'm not clear what you're asking for. Do you want me to open an issue about arm-android hangs, mention you in it and add a comment to this test with a link to the issue?

@matthewjasper
Copy link
Contributor

Yes

A function that has no prologue cannot be reasonably expected to support
debuginfo. In fact, the existing code (before this patch) would generate
invalid instructions that caused crashes. We can solve this easily by
just not emitting the debuginfo in this case.

Fixes rust-lang#42779
cc rust-lang#32408
@npmccallum
Copy link
Contributor Author

@matthewjasper Done. Thanks!

@npmccallum
Copy link
Contributor Author

Can someone please remove S-waiting-on-author? Thanks!

@matthewjasper
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 30, 2020

📌 Commit 2567074 has been approved by matthewjasper

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 30, 2020
@bors
Copy link
Contributor

bors commented Jul 30, 2020

⌛ Testing commit 2567074 with merge 1ce0cf0...

@bors
Copy link
Contributor

bors commented Jul 30, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: matthewjasper
Pushing 1ce0cf0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 30, 2020
@bors bors merged commit 1ce0cf0 into rust-lang:master Jul 30, 2020
tmandry added a commit to tmandry/rust that referenced this pull request Aug 14, 2020
Don't spill operands onto the stack in naked functions

Currently, the code spills operands onto the stack for the purpose of
debuginfo. However, naked functions can only contain an asm block. Therefore,
attempting to spill the operands on the stack is undefined behavior.

Fixes rust-lang#42779
cc rust-lang#32408

Note that this PR reverts rust-lang#74105 which ultimately didn't fix the problem.

cc @haraldh @Amanieu @matthewjasper
@bstrie bstrie added the A-naked Area: `#[naked]`, prologue and epilogue-free, functions, https://git.io/vAzzS label Feb 2, 2022
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-naked Area: `#[naked]`, prologue and epilogue-free, functions, https://git.io/vAzzS merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Naked functions with arguments generate a prologue
8 participants