Page MenuHomePhabricator

[Verifier] Require same signature for intrinsic calls
ClosedPublic

Authored by nikic on Jul 14 2021, 1:33 PM.

Details

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.

Diff Detail

Event Timeline

nikic created this revision.Jul 14 2021, 1:33 PM
nikic requested review of this revision.Jul 14 2021, 1:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2021, 1:33 PM

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 added a comment.Jul 14 2021, 1:58 PM

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 accepted this revision.Jul 14 2021, 2:19 PM
aeubanks added a subscriber: aeubanks.

lgtm

This revision is now accepted and ready to land.Jul 14 2021, 2:19 PM

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 accepted this revision.Jul 14 2021, 3:02 PM

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

nikic added a comment.Jul 14 2021, 3:13 PM

@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 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.

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 added a comment.Jul 15 2021, 9:36 AM

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.

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.

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.

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.

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

nikic updated this revision to Diff 359080.Jul 15 2021, 11:59 AM

Limit check to getIntrinsicID() for now.

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

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

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...

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...

@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.

@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 added a subscriber: pete.Jul 15 2021, 1:04 PM

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

nikic updated this revision to Diff 359134.Jul 15 2021, 2:23 PM
nikic edited the summary of this revision. (Show Details)

Restore previous implementation.

aeubanks accepted this revision.Jul 15 2021, 2:25 PM

still lgtm

dexonsmith accepted this revision.Jul 15 2021, 3:51 PM

still lgtm

LGTM still too.

This revision was landed with ongoing or failed builds.Jul 16 2021, 9:33 AM
This revision was automatically updated to reflect the committed changes.