[PATCH] D105313: [ValueTracking] Use call arguments for nonnull attribute check

Fri Jul 9 13:45:18 PDT 2021

nikic added a comment.

In D105313#2868000 <https://reviews.llvm.org/D105313#2868000>, @aeubanks wrote:

> are we going to allow mismatched argument types/num arguments for a direct call? it seems bad for usability and seems very easy to mess up frontends, or even simple handwritten IR tests
> I'm aware that there are issues with this, it's sort of like [1], but we should probably highlight this as part of the opaque pointer transition
>
> [1]: 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

Good question. I think we do need to allow it for similar reasons. Currently, you can call a function with a different signature by inserting a bitcast between the function type pointers. With opaque pointers we no longer have that bitcast to indicate the signature mismatch.

With that in mind, I think that this patch goes the wrong way about fixing the original problem (though I still think the change makes sense for other reasons, like using call-site attributes). Namely, what `getCalledFunction()` currently does is simply check whether `getCalledOperand()` is a function. However, with opaque pointers we should additionally check that the call function type matches the declaration function type. Then call can continue under the reasonable assumption that the call and the declaration match. Effectively this means that we're going to treat a call to a function with a different signature as an indirect, mostly non-analyzable call, the same as we did before. If we don't do this, we break lots of code, especially code handling intrinsics.

This doesn't address the issue of unintentional mistakes. I don't think we can really do anything about that -- it's a general drawback of the opaque pointer approach that you no longer have the sanity check between value types and pointer element types.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105313/new/

https://reviews.llvm.org/D105313