-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Replace some LLVMRust wrappers with calls to the LLVM C API #132167
Conversation
For historical context, these wrappers were added by #36200 and #38117. There appears to have been a fear of the enums going out of sync with LLVM, but the actual desyncs appear to have been self-inflicted, due to people modifying the Rust-side enum in ways that reflect the unstable C++ enum rather than the stable C enum. |
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, this looks like a very nice cleanup. I have some minor suggestions, otherwise this LGTM.
0fd8f64
to
3e51f6f
Compare
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. You can r=me after PR CI is green.
🟩 |
Replace some LLVMRust wrappers with calls to the LLVM C API This PR removes the LLVMRust wrapper functions for getting/setting linkage and visibility, and replaces them with direct calls to the corresponding functions in LLVM's C API. To make this convenient and sound, two pieces of supporting code have also been added: - A simple proc-macro that derives `TryFrom<u32>` for fieldless enums - A wrapper type for C enum values returned by LLVM functions, to ensure soundness if LLVM returns an enum value we don't know about In a few places, the use of safe wrapper functions means that an `unsafe` block is no longer needed, so the affected code has changed its indentation level.
3e51f6f
to
2f06853
Compare
Replace some LLVMRust wrappers with calls to the LLVM C API This PR removes the LLVMRust wrapper functions for getting/setting linkage and visibility, and replaces them with direct calls to the corresponding functions in LLVM's C API. To make this convenient and sound, two pieces of supporting code have also been added: - A simple proc-macro that derives `TryFrom<u32>` for fieldless enums - A wrapper type for C enum values returned by LLVM functions, to ensure soundness if LLVM returns an enum value we don't know about In a few places, the use of safe wrapper functions means that an `unsafe` block is no longer needed, so the affected code has changed its indentation level.
Rollup of 4 pull requests Successful merges: - rust-lang#132124 (coverage: Consolidate creation of covmap/covfun records) - rust-lang#132167 (Replace some LLVMRust wrappers with calls to the LLVM C API) - rust-lang#132169 (Deny calls to non-`#[const_trait]` methods in MIR constck) - rust-lang#132174 (x86 target features: make pclmulqdq imply sse2) r? `@ghost` `@rustbot` modify labels: rollup
|
|
2f06853
to
12fff8b
Compare
This comment has been minimized.
This comment has been minimized.
Looks like I am not the first person to have this problem with Given that there's precedent in |
12fff8b
to
d976ca8
Compare
Workaround for the |
This is fine, I'll revisit how bootstrap sets that lint level... it seems somewhat suspicious. @bors r+ rollup=iffy |
☀️ Test successful - checks-actions |
Finished benchmarking commit (be33e4f): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 2.7%, secondary -2.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 1.5%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 782.934s -> 782.771s (-0.02%) |
This PR removes the LLVMRust wrapper functions for getting/setting linkage and visibility, and replaces them with direct calls to the corresponding functions in LLVM's C API.
To make this convenient and sound, two pieces of supporting code have also been added:
TryFrom<u32>
for fieldless enumsIn a few places, the use of safe wrapper functions means that an
unsafe
block is no longer needed, so the affected code has changed its indentation level.