Arguments need to have the proper ABI parameter attributes set.
Followup to D101806.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp | ||
|---|---|---|
| 845 | Can we do something like CB->setAttributes(CB->getDirectCallee()->getAttributes() that avoids duplicating the knowledge of the ABI attributes to the call site? We know at this point that the constructed call will be direct, nothing stops this code from looking at the callee. | |
Comment Actions
lgtm
| llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp | ||
|---|---|---|
| 892 | nit: on second thought, maybe it's simpler to use CallbackFunc rather than CB->getCalledFunction(). It seems silly to recompute that which is available in a local variable. | |
Comment Actions
use FunctionCallee's function
this is actually a bit more verbose since we have to cast FunctionCallee's callee to a Function
Comment Actions
Ew. I guess I was wrong, the getCalledFunction version was better. Sorry about that. :)
| llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp | ||
|---|---|---|
| 845 | Is there anything preventing us from doing this directly in IRBuilder? Is there some situation where we don't want to inherit attributes from the declaration? | |
| llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp | ||
|---|---|---|
| 845 | In the case of indirect calls, we don't have a callee to look at. It would be unintuitive to only sometimes copy attributes depending on whether the call is direct or not. We could create a separate IRBuilder::CreateDirectCall() which does that. | |
Comment Actions
I ran the verifier extension (D103412) on SystemZ/SPEC17, and got several failures. I reduced one as a test case:
declare dso_local i8* @strchr(i8*, i32) local_unnamed_addr #1
declare dso_local i8* @memchr(i8*, i32 signext, i64)
declare void @llvm.assume(i1 noundef)
@0 = private unnamed_addr constant [21 x i8] c"$&*(){}[]'\22;\\|?<>~`\0A\00", align 2
define i1 @Perl_do_exec3(i8* %arg, i32 %arg1, i32 %arg2) {
bb:
%i = call i8* @strchr(i8* getelementptr inbounds ([21 x i8], [21 x i8]* @0, i64 0, i64 0), i32 signext undef) #4
%i3 = icmp ne i8* %i, null
call void @llvm.assume(i1 %i3)
br label %bb5
bb5: ; preds = %bb
ret i1 undef
}bin/opt -march=z14 -S -o - -enable-new-pm=0 -instcombine
gives
argument to signext parameter requires signext attribute %memchr = call i8* @memchr(i8* noundef nonnull dereferenceable(21) getelementptr inbounds ([21 x i8], [21 x i8]* @0, i64 0, i64 0), i32 undef, i64 21) in function Perl_do_exec3 LLVM ERROR: Broken function found, compilation aborted!
I tried this patch, which helped me with the test case:
--- a/llvm/lib/IR/Instructions.cpp +++ b/llvm/lib/IR/Instructions.cpp @@ -501,6 +501,9 @@ void CallInst::init(FunctionType *FTy, Value *Func, ArrayRef<Value *> Args, (void)It; assert(It + 1 == op_end() && "Should add up!"); + if (Function *Fun = dyn_cast<Function>(Func)) + setAttributes(Fun->getAttributes()); + setName(NameStr); }
I saw the question as to why not do this in IRBuilder with the response that "It would be unintuitive to only sometimes copy attributes depending on whether the call is direct or not.", but there didn't seem to be some consensus on this. I would be one to agree that it seems very desirable to have this handled generally and not just hope that everyone will always remember this. It seems to me it could be done in the CallInst/InvokeInst constructors even per above. If somebody has the intent of *not* adding the attributes to the CallInst (don't understand why), there could be an option to not do so - like an argument 'bool CopyAttributes = true'.
I am new to this discussion so I may be missing something... I guess I don't fully understand why the declaration is no longer used for attributes - can the same function be called with/without e.g. a ZExt attribute on a parameter? I thought the callee expects this always per the ABI and that it would be an error to omit it..?
Comment Actions
I don't like the idea of having the IRBuilder look at the call target and implicitly setting attributes, but I am open to revisiting the backend change. Here's a rationale for going back to the old behavior:
- calling a function with the wrong function prototype is UB
- calling a function with mismatched ABI attributes is very much like prototype mismatch, so it's UB
- LLVM can generate whatever code it wants in cases of UB. So, if we have a direct callee, why not have the backend implicitly use the attributes from the callee?
I've forgotten why exactly we wanted to avoid looking at the callee's attributes.
Comment Actions
I was looking back to D101713 where I found some crashing tests due to mismatched ABI attributes, specifically ones with type parameters like byval. The solution I had come up with was D101806, which was to ignore caller ABI attributes. We could also fix those crashes by also looking at the caller indirect ABI attribute type parameters.
I still think it's more consistent to only look at the callee parameter ABI attributes. It's weird that indirect and direct calls could be different. For example, ABI lowering could be different if an indirect call is promoted to a direct call with some ABI attributes not present on the call instruction. But it's less work to insert random calls to functions (especially compiler-rt functions or library calls) and not have to worry about ABI attributes. At this point I'm willing to give up on D101806 and just go down the easier route of looking at the caller for the type parameters to byval/etc (and revert the changes I've made similar to this).
Comment Actions
OK, right, the issue we were trying to solve is that LLVM was crashing when byval (+inalloca, etc) types mismatched somehow.
It occurs to me that, in a world of typeless pointers, it is possible that the function type used by the call may not match the prototype of the callee. Consider something like this:
define dso_local void @pass_int() #0 {
entry:
call void bitcast (void (i32*)* @use_int to void (i32)*)(i32 0)
ret void
}
declare dso_local void @use_int(i32* byval(i32)) #1With opaque pointers, we lose the bitcast, so the call may appear to be direct, but the prototypes do not match. If the prototypes don't match, the attribute lists may not even correspond. In this case, the argument we are passing to a byval parameter isn't even a pointer. I don't know how the codegenerator would handle that.
We may need to teach CallBase::getCallee to do more than just dyn_cast, maybe it should check that the function prototypes match, and the call will be considered indirect otherwise. Is this a known issue, or something new?
If the prototypes do match, then I think it is OK to look at ABI attributes on the declaration of the callee. In the case of byval, LLVM would get the type from the declaration and use that. The caller would have only a typeless pointer argument anyway, which has no information about the underlying value to be passed.
The other option is that we compromise and allow codegen to look at the callee only for some kinds of ABI attributes.
Comment Actions
calling a function with mismatched ABI attributes is very much like prototype mismatch, so it's UB
Sounds right to me.
The other option is that we compromise and allow codegen to look at the callee only for some kinds of ABI attributes.
...just go down the easier route of looking at the caller for the type parameters to byval/etc
The sext/zext attributes are the ones of immediate concern and must never be missing, at least on SystemZ.
I have proposed a patch at https://reviews.llvm.org/D103992, that follows the route of trying to make sure that all calls everywhere have the proper attributes set. That is needed if you would prefer to avoid the checking of the prototype during isel. (On the other hand, I am not really saying that I think that is better than the old behaviour...)
Can we do something like CB->setAttributes(CB->getDirectCallee()->getAttributes() that avoids duplicating the knowledge of the ABI attributes to the call site? We know at this point that the constructed call will be direct, nothing stops this code from looking at the callee.