-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Clean the LLVM wrapper #135502
base: master
Are you sure you want to change the base?
Clean the LLVM wrapper #135502
Conversation
pub enum UnnamedAddr { | ||
No, | ||
Local, | ||
Global, |
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 enums that are passed across FFI to C/C++ code, we shouldn't delete Rust-side variants that are unused.
Doing so just makes it harder to verify that the Rust-side enum and the C-side enum have the same layout, which is essential for soundness.
Default, | ||
Gnu, |
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 is another case where removing unused enum variants is a net negative. It just makes auditing harder.
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 this case, it was removed from both the Rust and C++ side: would you rather that I add it back to both?
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.
Yeah, please add it back to both.
(Our LLVMRustDebugNameTableKind
enum is basically just a substitute for LLVM's DICompileUnit::DebugNameTableKind
. It's needed to work around the fact that we can't reliably pass LLVM's C++ enums across FFI, because the LLVM C++ API isn't stable, so upgrading LLVM would potentially introduce silent UB if the enums change incompatibly.)
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.
Done
(There's more review I want to do here, but unfortunately GitHub has decided that trying to add a review comment should now hang the browser for several seconds, so the review interface is more-or-less unusable right now.) |
I've tried some of this cleanup in #127274, so some review comments from it can be used here as well. |
Good call:
|
Yes, removing the tail call stuff is fine, as you said it is a trivial revert. |
r? @Zalathar as you're already reviewing this PR 🤔 feel free to reassign |
☔ The latest upstream changes (presumably #135335) made this pull request unmergeable. Please resolve the merge conflicts. |
This change sets the visibility for LLVM wrapper FFI items to
pub(crate)
, which allows Rust's dead code lints to flag unused parts of the LLVM wrapper.