-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[rustc_public] Enhance PassMode API #159359
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
Open
celinval
wants to merge
3
commits into
rust-lang:main
Choose a base branch
from
celinval:feat/cast-passmode
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file has incorrect statements such as
See #132246 for more on why this is wrong.
I would honestly rather you not "enhance" anything here and set almost this entire file on fire here because it's just going to make me actually rewriting this harder. We could make more accurate information available in the future, but it's unclear why anyone would benefit from the current information because they lack the full context to interpret it.
View changes since the review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the context. Having to reverse engineer the compiler is not an easy task, so please do point out any inaccuracies. Most of the comments here I wrote based on the comments from rustc_target and from analyzing the generated FnAbi for different function signatures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dearly sympathize because my task of figuring out what the hell is going on involves reverse-engineering LLVM. 🙃
At some point I just sort of started assuming everything I am reading is probably wrong, unfortunately.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question here is mostly: What question is someone trying to answer that they think this information would be of use to them? If they're trying to do a basic ABI equality check then smuggling the data around opaquely with mostly no transformation and exposing a
PartialEqimpl that interrogates the innards seems more useful.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To elaborate: The
rustc_targetandrustc_abicrates were mostly written ages ago by people who are not easy to ask questions of anymore, in a single-backend context, where x86-64 was our only tier 1 target. It was then incrementally edited, including by people such as myself, in ways that were "good enough", and our unclear understandings documented as truth because no one was around anymore who completely knew exactly what was going on and we could usefully discuss things with. "cg_ssa" (rustc_codegen_ssa) was introduced as an incomplete abstraction layer over LLVMIR, and to the extent it has abstracted anything, it has made everything inrustc_abiandrustc_targetwronger.That is why asking questions about things below
FnAbiis probably not a good idea, unless it's about a fundamental likeSizeorAlign. Obviously those have to be correct. And we've... tried to makeLayoutData... vaguely intelligible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Rust ABI there are things that can't be represented as a valid C signature. For example there are cases where we use more return registers than the platform C ABI allows, but LLVM supports representing it anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the context. To summarize where I think we are: this PR adds information that is currently missing from
rustc_public— the structure ofPassMode::Cast, argument attributes, and corrected documentation. The data itself is not incorrect; it accurately reflects the compiler's calling convention decisions.Users may still need to do further processing depending on their use case (as
cg_ssadoes today), and the Rust ABI can produce things that don't map cleanly to a C signature. The data shouldn't be taken at face value as "this is exactly what happens on the wire" — but it is the correct input for tools that need to reason about argument layout and pass modes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these need to be compatible with the Rust ABI that the LLVM backend happens to use or is it only about C FFI and can they chose to use an arbitrary ABI for
extern "Rust"?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. From a usability perspective, I'd prefer that the API was complete in a way that allows users to interact with things like a pre-compiled rust std. The ideal goal is to enable users to build rich Rust tooling, not to tell users what they can or cannot do.
That said, from a maintenance point of view, I think it makes sense to start small if the cost is too high or impractical, and expand wherever we can once there's demand.
So being able to reason about C FFI interaction and implement an arbitrary ABI for
extern "Rust"is a reasonable first step.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For C FFI depending on how exactly your backend works, lowering the
Layoutrather thanFnAbimay work better. Especially if you are emitting to a C shaped IR with actual support for C types as arguments.FnAbiis designed for LLVM IR and works fine for Cranelift IR too given how similar Cranelift IR is to LLVM IR. It happens to work for gccjit, but I'm not sure if it is actually correct when cross-language LTO. I haven't checked, but I wouldn't be surprised if GCC keeps the full C types and expects other frontends to produce the exact same types, which usingFnAbidefinitively won't do. For LLVM shaped backends it does make sense to useFnAbi, but the function signature handling has to be fairly closely modeled after LLVM.