cg_llvm: Overhaul FFI for creation of LLVM range attributes #145846
Conversation
|
r? @SparrowLii rustbot has assigned @SparrowLii. Use |
rustbot
added
A-LLVM
|
Implementation of LLVM's Notice that |
|
Wouldn't it make more sense to assert that the number of bits is <= 128? Your new implementation (in a hypothetical world where WrappingRange can represent more than 128 bits) would also be wrong, just in a different way. |
Zalathar
force-pushed
the
llvm-range-attr
branch
from
240af05 to
5eb5fee
Compare
|
I suppose that's true. It would no longer be UB, but it would still be a bug. I'm still not fond of delegating to |
My thinking here was that we ideally wouldn't need LLVMRustCreateRangeAttribute at all, and directly use LLVMCreateConstantRangeAttribute instead. The only reason we don't do this right now is to pass |
|
Hmm, I see. I had mostly not considered moving towards LLVM-C for attribute kinds, because the documentation implies that looking up attribute kinds via the C API is even less stable than just using the C++ API, which made it seem pretty unattractive. But I don't know how accurate that is in reality. |
|
I'll prepare a narrower PR that just fixes the soundness hole with an assert (and some docs), which will leave the rest of the overhaul as a separate matter. @rustbot author |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
cg_llvm: Assert that LLVM range-attribute values don't exceed 128 bits The underlying implementation of `LLVMCreateConstantRangeAttribute` assumes that each of `LowerWords` and `UpperWords` points to enough u64 values to define an integer of the specified bit-length, and will encounter UB if that is not the case. Our safe wrapper function always passes pointers to `[u64; 2]` arrays, regardless of the bit-length specified. That's fine in practice, because scalar primitives never exceed 128 bits, but it is technically a soundness hole in a safe function. We can close the soundness hole by explicitly asserting `size_bits <= 128`. This is effectively just a stricter version of the existing check that the value must be small enough to fit in `c_uint`. --- This is a narrower version of the fix in rust-lang#145846.
Rollup merge of #145867 - Zalathar:range-attr, r=nikic cg_llvm: Assert that LLVM range-attribute values don't exceed 128 bits The underlying implementation of `LLVMCreateConstantRangeAttribute` assumes that each of `LowerWords` and `UpperWords` points to enough u64 values to define an integer of the specified bit-length, and will encounter UB if that is not the case. Our safe wrapper function always passes pointers to `[u64; 2]` arrays, regardless of the bit-length specified. That's fine in practice, because scalar primitives never exceed 128 bits, but it is technically a soundness hole in a safe function. We can close the soundness hole by explicitly asserting `size_bits <= 128`. This is effectively just a stricter version of the existing check that the value must be small enough to fit in `c_uint`. --- This is a narrower version of the fix in #145846.
|
☔ The latest upstream changes (presumably #145886) made this pull request unmergeable. Please resolve the merge conflicts. |
rustbot
removed
the
S-waiting-on-author
The existing implementation of
LLVMRustCreateRangeAttributehas a theoretical soundness bug.It delegates to
LLVMCreateConstantRangeAttribute, which dividesNumBitsby 64 to determine how long theLowerWords/UpperWordsslices are. Therefore, ifNumBits > 128, the code will think that the slices have three or more elements, even though the Rust-side code only supplies two valid elements.(I assume that this can't actually happen in practice, but because the
CreateRangeAttributewrapper function is safe, this still isn't ideal.)This PR therefore avoids
LLVMCreateConstantRangeAttributeentirely, and instead passes each range endpoint as two separateu64values, which can be reassembled without relying onNumBits.