Prepare call/invoke for opaque pointers #87743
Conversation
|
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @nikic |
| fn llvm_type(&self, cx: &CodegenCx<'ll, 'tcx>) -> &'ll Type { | ||
| let args_capacity: usize = self.args.iter().map(|arg| | ||
| fn llvm_type(&self, cx: &CodegenCx<'ll, 'tcx>, decl: bool) -> &'ll Type { | ||
| // Ignore extra args when calling C variadic functions. |
There was a problem hiding this comment.
This comment is very slightly confusing. I think it would be somewhat more comprehensible if it said
| // Ignore extra args when calling C variadic functions. | |
| // Ignore variadic args when calling C variadic functions. |
Additionally, this says "calling", but the ignoring only happens if decl is true…
There was a problem hiding this comment.
Finally another thing I find confusing here is that we tell LLVM the function as variadic regardless of decl, so I'm not exactly sure what purpose this serves? Is it intended to remove the VaList style argument?
There was a problem hiding this comment.
Say you have fn(i32, i64, ...), and you call it like fn(i32, i64, bool, f32), I think the last two argument types need to be removed to make sure that the function signature at the call site matches the declaration. Maybe always using &self.args[..self.fixed_count] would work too though?
There was a problem hiding this comment.
So, this variadic thing was the last bit I struggled with, and I'm happy to explore better ways to represent it.
The specific example that tripped when building std was libc::fcntl, which looks like this:
pub fn fcntl(fd: ::c_int, cmd: ::c_int, ...) -> ::c_int;That turns into LLVM fn(i32, i32, ...) -> i32. The call site for set_cloexec in std passes three args, and the FnAbi::llvm_type from there gave me fn(i32, i32, i32, ...) -> i32 -- having that "extra" arg when its FnAbi was constructed, but still represented as variadic. However, that tripped an assertion:
rustc: [...]/rust/src/llvm-project/llvm/lib/Bitcode/Writer/ValueEnumerator.h:167: unsigned int llvm::ValueEnumerator::getTypeID(llvm::Type*) const: Assertion `I != TypeMap.end() && "Type not in ValueEnumerator!"' failed.
So I used fixed_count to try and get the original declaration type, which seems to work -- giving the same signature as element_type(val_ty(fn_ptr)) would if we didn't care about being opaque.
Maybe always using
&self.args[..self.fixed_count]would work too though?
It at least needs to check for c_variadic, because I know that the #[track_caller] argument for &Location is also past the fixed_count. I can try it without considering the new decl parameter though, or outright removing that.
There was a problem hiding this comment.
It at least needs to check for c_variadic, because I know that the #[track_caller] argument for &Location is also past the fixed_count.
That seems bad. At least on riscv that will cause the #[track_caller] argument to be handled like a vararg during the abi calculation.
There was a problem hiding this comment.
At least on riscv that will cause the
#[track_caller]argument to be handled like a vararg during the abi calculation.
I looked at that, but it's only called by adjust_for_cabi, which isn't used for Rust/intrinsic ABIs.
There was a problem hiding this comment.
All tests passed locally without using decl, even with LLVM assertions, so I've removed it.
Rather than relying on `getPointerElementType()` from LLVM function pointers, we now pass the function type explicitly when building `call` or `invoke` instructions.
We can apply the `c_variadic` fix all the time, rather than trying to distinguish between declarations and any other use.
|
AFAICS the remaining calls to |
|
@bors r+ |
|
📌 Commit a292390 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
| fn fn_ptr_backend_type(&self, fn_abi: &FnAbi<'tcx, Ty<'tcx>>) -> &'ll Type { | ||
| fn_abi.ptr_to_llvm_type(self) | ||
| } |
There was a problem hiding this comment.
I wonder if this method should have this default (or be inlined, but right now there's two callers):
self.type_ptr_to_ext(cx.data_layout().instruction_address_space)(also type_ptr_to_ext should probably be renamed to type_ptr_to_in_addrspace or something)
Rather than relying on
getPointerElementType()from LLVM functionpointers, we now pass the function type explicitly when building
callor
invokeinstructions.