[Verifier] Require same signature for intrinsic calls

[nikic]

Summary

As suggested on D105733, this adds a verifier rule that calls to intrinsics must match the signature of the intrinsic.

Without opaque pointers this is automatically enforced for all calls, because the pointer types need to match. If the signatures don't match, a pointer bitcast has to be inserted. For intrinsics in particular, such bitcasts are not legal, because the address of intrinsics cannot be taken.

With opaque pointers, there are no more pointer bitcasts, so it's generally possible for the call and the callee signature to differ. However, for intrinsics we still want to enforce that the signature must match, the same as was done before through the address taken check.

We can't enforce this more generally for non-intrinsics, because calls with mismatched signature at the very least can legally occur in unreachable code, and might also be valid in some other cases, depending on how exactly the signatures differ.

[dexonsmith]

We probably can't enforce this in general due to unreachable (possibly de-indirected) calls, but I don't see a reason why we can't enforce this for intrinsics at least.

Can you explain in more detail why the verifier couldn't check this in unreachable (possibly de-indirected) calls? Why can't we require transforms to be type-correct here (by calling RAUW with a bitcast, or similar)?

[nikic]

In D106013#2878226, @dexonsmith wrote:

We probably can't enforce this in general due to unreachable (possibly de-indirected) calls, but I don't see a reason why we can't enforce this for intrinsics at least.

Can you explain in more detail why the verifier couldn't check this in unreachable (possibly de-indirected) calls? Why can't we require transforms to be type-correct here (by calling RAUW with a bitcast, or similar)?

The same reasoning as https://llvm.org/docs/FAQ.html#why-does-instcombine-simplifycfg-turn-a-call-to-a-function-with-a-mismatched-calling-convention-into-unreachable-why-not-make-the-verifier-reject-it applies here, or at least I would assume so.

[aeubanks]

lgtm

[dexonsmith]

In D106013#2878243, @nikic wrote:

In D106013#2878226, @dexonsmith wrote:

We probably can't enforce this in general due to unreachable (possibly de-indirected) calls, but I don't see a reason why we can't enforce this for intrinsics at least.

Can you explain in more detail why the verifier couldn't check this in unreachable (possibly de-indirected) calls? Why can't we require transforms to be type-correct here (by calling RAUW with a bitcast, or similar)?

The same reasoning as https://llvm.org/docs/FAQ.html#why-does-instcombine-simplifycfg-turn-a-call-to-a-function-with-a-mismatched-calling-convention-into-unreachable-why-not-make-the-verifier-reject-it applies here, or at least I would assume so.

Thanks for the link. The argument there is that we don't want all transforms to have to worry about calling conventions. (I guess we can't take the address of intrinsics, and that's why this verifier check is okay for them?)

We could choose to make a different decision for type-correctness of function calls.

My intuition is that it's easier for type-correctness than for calling conventions.

• Any code replacing one function with another is already calling RAUW with a bitcast to the new function type. (Doesn't catch calling conventions of course, but maintains FunctionType correctness.)
• Any existing code creating a new CallInst should similarly be creating the expected bitcasts.
• The inliner would need a patch. I spent some time looking at the code. The hard part (probably obvious to you) is that there won't be bitcasts at the call site, which means ValueMapper::mapValue() can't return the "right" result. You'd need to catch this up a level, in ValueMapper::remapInstruction, by checking for CallBase and adding a bitcast if there's a type mismatch. There's related code already in that function for when there's a TypeMapper, added in 348de69a3045e017d4d614213d28e69a6fdd4b73 to allow indirect calls to round-trip through bitcode and textual IR.

Are there other cases? I wonder if they're enumerable, or if it'll be a long tail... my intuition is the former, but I don't really know.

[dexonsmith]

(Regardless, this patch LGTM, certainly correct for intrinsics; probably better to continue this discussion at https://reviews.llvm.org/D105733.)

[nikic]

@dexonsmith I don't think upholding invariants during transforms is really the problem here. The problem is that something like this is legal C code:

extern void one_arg(unsigned);
int main() {
    if (0) {
        ((void(*)(void)) one_arg)();
    }
    return 0;
}

With opaque pointers (where there are no pointer bitcasts) this is going to lower to a call that has a different signature from the callee. We can't make that a verifier error.

[dexonsmith]

In D106013#2878499, @nikic wrote:

@dexonsmith I don't think upholding invariants during transforms is really the problem here. The problem is that something like this is legal C code:

extern void one_arg(unsigned);
int main() {
    if (0) {
        ((void(*)(void)) one_arg)();
    }
    return 0;
}

With opaque pointers (where there are no pointer bitcasts) this is going to lower to a call that has a different signature from the callee. We can't make that a verifier error.

Right, no pointer bitcasts at all -- I was somehow thinking there were still pointer-to-function bitcasts (just fewer of them, since pointers were collapsed in their parameter lists). Thanks for walking me through it.

[dexonsmith]

We probably can't enforce this in general due to unreachable (possibly de-indirected) calls, but I don't see a reason why we can't enforce this for intrinsics at least.

Might be good to refine this in the commit message to say "not correct in general because there are no bitcasts for pointers-to-functions" (or something like that), since the limitation doesn't really have much to do with unreachable code.

[nikic]

Looks like the new verifier rule triggers on some ObjCARC tests, for example:

Intrinsic called with incompatible signature
  %call21 = call signext i8 bitcast (i8* (i8*, i8*, ...)* @llvm.objc.msgSend to i8 (i8*, i8*)*)(i8* %call137, i8* %tmp19)
Intrinsic called with incompatible signature
  %call51 = call %struct._NSRange bitcast (i8* (i8*, i8*, ...)* @llvm.objc.msgSend to %struct._NSRange (i8*, i8*, i64, i64)*)(i8* %call137, i8* %tmp49, i64 0, i64 0)
Intrinsic called with incompatible signature
  %call66 = call signext i8 bitcast (i8* (i8*, i8*, ...)* @llvm.objc.msgSend to i8 (i8*, i8*, %1*)*)(i8* %call6110, i8* %tmp64, %1* bitcast (%struct.NSConstantString* @_unnamed_cfstring_44 to %1*))
Intrinsic called with incompatible signature
  %call84 = call signext i8 bitcast (i8* (i8*, i8*, ...)* @llvm.objc.msgSend to i8 (i8*, i8*)*)(i8* %filename.0.in, i8* %tmp82)

Not entirely sure what to do here. This code only works because llvm.objc.msgSend is not an actual registered intrinsic, which means that taking it's address is not prohibited by the verifier. For "real" intrinsics, the bitcast would already error.

So should I relax the check to just isIntrinsic() callees, or should I try to adjust those ObjCARC tests to drop the bitcast? It seems like the issue is with the input IR, not the transform, and check-clang passes, so it doesn't look like this IR is generated by the frontend.

[aeubanks]

isIntrinsic() looks like it's equivalent to getName().startswith("llvm.").

Is it expected that we can take the address of not-registered intrinsics? That seems wrong. I'd say we should disallow that, making it an easy decision to keep what we have and fix the test.

[nikic]

In D106013#2880590, @aeubanks wrote:

isIntrinsic() looks like it's equivalent to getName().startswith("llvm.").

Ah right. Apparently the relevant distinction is isIntrinsic() vs getIntrinsicID(). The former just checks for llvm., while the latter requires a registered intrinsic. The hasAddressTaken() check is currently only applied to the getIntrinsicID() case.

Is it expected that we can take the address of not-registered intrinsics? That seems wrong. I'd say we should disallow that, making it an easy decision to keep what we have and fix the test.

I'm inclined to agree. I'll give it a try.

[nikic]

Okay, this doesn't looks as simple as I thought. Taking the first call:

%call21 = call signext i8 bitcast (i8* (i8*, i8*, ...)* @llvm.objc.msgSend to i8 (i8*, i8*)*)(i8* %call137, i8* %tmp19)

What I missed before is that this actually changes the return type from i8* to i8. That's nasty. I could adjust for that using a ptrtoint + trunc, but that doesn't make this a straightforward test adjustment anymore. I'm not sure what's up with these @llvm.objc.msgSend intrinsics -- clang only uses the normal @objc_msgSend function.

[aeubanks]

@ahatanak, any idea what's up with @llvm.objc.msgSend()?

[nikic]

Limit check to getIntrinsicID() for now.

[dexonsmith]

In D106013#2881021, @aeubanks wrote:

@ahatanak, any idea what's up with @llvm.objc.msgSend()?

There's some context in https://reviews.llvm.org/D55348 / be4f5711073613115c036206db0d9a45fd0632ab.

In D106013#2881011, @nikic wrote:

I'm not sure what's up with these @llvm.objc.msgSend intrinsics -- clang only uses the normal @objc_msgSend function.

That doesn't seem right...

[dexonsmith]

Looks like the clang side was https://reviews.llvm.org/D55802 / 2cd3596b1a48a3ff007f58f933e77442caeec746.

But that didn't convert objc_msgSend, presumably because the optimizer just sees that as an opaque method. Not sure if an intrinsic is useful for that method...

[nikic]

In D106013#2881094, @dexonsmith wrote:

In D106013#2881021, @aeubanks wrote:

@ahatanak, any idea what's up with @llvm.objc.msgSend()?

There's some context in https://reviews.llvm.org/D55348 / be4f5711073613115c036206db0d9a45fd0632ab.

Seeing that, I wonder if this was some overzealous search and replace in tests? At least the ModuleHasARC() function doesn't contain llvm.objc.msgSend. And the other intrinsics introduced there are emitted by clang (or at least some I checked), but this one is not.

[dexonsmith]

@llvm.objc.msgSend is not listed at https://llvm.org/docs/LangRef.html#objective-c-arc-runtime-intrinsics -- I suspect this just crept into tests when the other runtime functions were converted to intrinsics.

[ahatanak]

Yes, it looks like @objc_msgSend in the tests was rewritten along with other ARC runtime functions.

[nikic]

Restore previous implementation.

[aeubanks]

still lgtm

[dexonsmith]

In D106013#2881537, @aeubanks wrote:

still lgtm

LGTM still too.

llvm/lib/IR/Verifier.cpp

Assert(Call.getArgOperand(i)->getType() == FTy->getParamType(i),
            "Call parameter type does not match function signature!",
            Call.getArgOperand(i), FTy->getParamType(i), Call);
 
   AttributeList Attrs = Call.getAttributes();
 
   Assert(verifyAttributeCount(Attrs, Call.arg_size()),
          "Attribute after last parameter!", Call);
 
-  bool IsIntrinsic = Call.getCalledFunction() &&
-                     Call.getCalledFunction()->isIntrinsic();
-
   Function *Callee =
       dyn_cast<Function>(Call.getCalledOperand()->stripPointerCasts());
+  bool IsIntrinsic = Callee && Callee->isIntrinsic();
+  if (IsIntrinsic)
+    Assert(Callee->getValueType() == FTy,
+           "Intrinsic called with incompatible signature", Call);
 
   if (Attrs.hasFnAttribute(Attribute::Speculatable)) {
     // Don't allow speculatable on call sites, unless the underlying function
     // declaration is also speculatable.
     Assert(Callee && Callee->isSpeculatable(),
            "speculatable attribute may not apply to call sites", Call);
   }
 

llvm/test/Verifier/force-opaque-ptr.ll

+; RUN: not opt -passes=verify -force-opaque-pointers -S < %s 2>&1 | FileCheck %s
+
+declare i32 @llvm.umax.i32(i32, i32)
+
+define void @intrinsic_signature_mismatch() {
+; CHECK: Intrinsic called with incompatible signature
+  call i32 @llvm.umax.i32(i32 0)
+  ret void
+}