Skip to content

Add -Zfunction-return={keep,thunk-extern} option #116892

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

Merged
merged 2 commits into from
Dec 1, 2023
Merged

Conversation

ojeda
Copy link
Contributor

@ojeda ojeda commented Oct 18, 2023

This is intended to be used for Linux kernel RETHUNK builds.

With this commit (optionally backported to Rust 1.73.0), plus a patched Linux kernel to pass the flag, I get a RETHUNK build with Rust enabled that is objtool-warning-free and is able to boot in QEMU and load a sample Rust kernel module.

Issue: #116853.

@rustbot
Copy link
Collaborator

rustbot commented Oct 18, 2023

r? @wesleywiser

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 18, 2023
@rust-log-analyzer

This comment has been minimized.

@ojeda
Copy link
Contributor Author

ojeda commented Oct 18, 2023

Note that LLVM ignores the attribute in non-x86, but I have not replicated Clang's check that this only works in x86 for the CLI flag for the moment (should it be part of the target spec?).

Also, I linked the feature request issue as the tracking one in the docs -- we can reuse it changing the OP message, I assume, but please let me know if it is best to create a new one.

@ojeda ojeda force-pushed the rethunk branch 2 times, most recently from 8df5004 to 8b6016a Compare October 19, 2023 22:25
Comment on lines +1710 to +1695
// FIXME: In principle, the inherited base LLVM target code model could be large,
// but this only checks whether we were passed one explicitly (like Clang does).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is a way to get the effective code model, it would be nice to use it.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That covers even less, no? i.e. code_model() covers codegen and target options.

@rust-log-analyzer

This comment has been minimized.

@ojeda
Copy link
Contributor Author

ojeda commented Oct 19, 2023

I added a folder but that still counts against the tidy limit in tests/ui. Should I move the folder to e.g. tests/ui/invalid-compile-flags or increase the limit by 1?

@ojeda
Copy link
Contributor Author

ojeda commented Oct 20, 2023

One more note: GCC and Clang give an error if one tries to use function-return at all, i.e. even with keep, on non-x86. However, in this PR keep is allowed for all architectures.

I did it like this because it seemed simpler (no Option) and more flexible, especially since (in the case of this flag) other architectures could support particular modes, e.g. s390 expolines (https://github.com/llvm/llvm-project/blob/49af6502c6dcb4a7f7520178bd14df396f78240c/clang/lib/Frontend/CompilerInvocation.cpp#L1942).

But perhaps consistency with GCC/Clang is better?

@ojeda
Copy link
Contributor Author

ojeda commented Oct 20, 2023

I added a folder but that still counts against the tidy limit in tests/ui. Should I move the folder to e.g. tests/ui/invalid-compile-flags or increase the limit by 1?

I moved them inside the other folder, and noticed one in invalid/ should be in that invalid-compile-flags/ folder: #116975.

@bors
Copy link
Collaborator

bors commented Oct 30, 2023

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

@ojeda
Copy link
Contributor Author

ojeda commented Oct 31, 2023

Resolved trivial formatting conflict.

@wesleywiser
Copy link
Member

Thanks for already getting the tracking issue set up @ojeda!

@bors r+

@ojeda ojeda deleted the rethunk branch December 1, 2023 11:59
@ojeda
Copy link
Contributor Author

ojeda commented Dec 1, 2023

@wesleywiser Thanks for the very quick review and have a nice holiday (I saw the other PR by chance when looking for this one :)

@wesleywiser
Copy link
Member

Thanks! ☺️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.