Make our DIFlags match LLVMDIFlags in the LLVM-C API
#135156
Conversation
rustbot
added
S-waiting-on-review
|
I've managed to convince myself that this way is better than what I was doing in #134009. Comparing all three values in a static assertion helps to guard against copy-paste errors, and we don't need to care about whether the C++ compiler is going to generate good or terrible code for the manual conversion. |
Zalathar
added
the
A-LLVM
| @@ -9,6 +9,8 @@ test = false | |||
| [dependencies] | |||
| # tidy-alphabetical-start | |||
| bitflags = "2.4.1" | |||
| # To avoid duplicate dependencies, this should match the version of gimli used | |||
| # by `rustc_codegen_ssa` via its `thorin-dwp` dependency. | |||
There was a problem hiding this comment.
This is off topic? (but harmless)
There was a problem hiding this comment.
Oh yeah, this was a follow-up to #135115 that I tacked on to this PR due to it being trivial, and vaguely related to debuginfo bindings.
| } | ||
|
|
||
| static_assert(eq(LLVMDIFlagZero, 0, DIFlags::FlagZero)); | ||
| static_assert(eq(LLVMDIFlagPrivate, 1, DIFlags::FlagPrivate)); |
There was a problem hiding this comment.
How about a good old C macro to make these less repetitive?
#define ASSERT_EQ_DI(Flag, Value) \
static_assert((LLVMDI##Flag == (Value)) && (DIFlags::Flag == (Value)));
ASSERT_EQ_DI(FlagZero, 0);
ASSERT_EQ_DI(FlagPrivate, 1);
// etc.I do see 1<<15 with different names below, but if that's the only one, then it could be stated explicitly. What do you think?
There was a problem hiding this comment.
At the time I remember choosing to prefer plain C++ code over C macros, but your example does look like an improvement, so maybe I'll give it a try.
Zalathar
force-pushed
the
debuginfo-flags
branch
from
9782e37 to
6278130
Compare
|
(Rebased with no changes; I'll try out the C macro and push that if it looks like an improvement.) |
Zalathar
force-pushed
the
debuginfo-flags
branch
from
6278130 to
d10bdaf
Compare
|
OK yeah, the C macro is a net improvement (diff). It's less greppable, which is unfortunate, but the upside is that each individual line is compact enough to actually skim/read. |
|
Looks good, thanks! @bors r+ |
bors
added
S-waiting-on-bors
…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#133372 (Refactor dyn-compatibility error and suggestions) - rust-lang#134396 (AIX: use align 8 for byval parameter) - rust-lang#135156 (Make our `DIFlags` match `LLVMDIFlags` in the LLVM-C API) - rust-lang#135816 (Use `structurally_normalize` instead of manual `normalizes-to` goals in alias relate errors) - rust-lang#135823 (make UI tests that use `--test` work on panic=abort targets) - rust-lang#135850 (Update the `wasm-component-ld` tool) - rust-lang#135858 (rustdoc: Finalize dyn compatibility renaming) - rust-lang#135866 (Don't pick `T: FnPtr` nested goals as the leaf goal in diagnostics for new solver) - rust-lang#135874 (Enforce that all spans are lowered in ast lowering) - rust-lang#135875 (Remove `Copy` bound from `enter_forall`) r? `@ghost` `@rustbot` modify labels: rollup
In order to be able to use a mixture of LLVM-C and C++ bindings for debuginfo, our Rust-side
DIFlagsneeds to have the same layout as LLVM-C'sLLVMDIFlags, and we also need to be able to convert it to theDIFlagsaccepted by LLVM's C++ API.Internally, LLVM converts between the two types with a simple cast. We can't necessarily rely on that always being true, and LLVM doesn't expose a conversion function, so we have two potential options:
As long as both types do remain the same under the hood (which seems likely), the static-assert-and-cast approach is easier and faster. If the static assertions ever start failing against some future version of LLVM, we'll have to switch over to the convert-each-subvalue approach, which is a bit more error-prone.
Extracted from #134009, though this PR ended up choosing the static-assert-and-cast approach over the convert-each-subvalue approach.