[PATCH] D101725: [Verifier] Check that calls' arguments match the callee's byval/inalloca/preallocated

Mon May 3 13:57:38 PDT 2021

dblaikie added a comment.

In D101725#2734397 <https://reviews.llvm.org/D101725#2734397>, @aeubanks wrote:

> In D101725#2732620 <https://reviews.llvm.org/D101725#2732620>, @dblaikie wrote:
>
>> In D101725#2732577 <https://reviews.llvm.org/D101725#2732577>, @aeubanks wrote:
>>
>>> I think the bitcode and textual representation both allow this issue.
>>> But based on the fact that so much of the checking for byval and related attributes is on the arguments rather than the parameters, IMO it's always been intended to have the attributes at least on the parameters. Users like Clang have always been properly emitting byval on the parameters (no check-clang tests regressed). This patch is just filling in a hole that should always have been filled in. But maybe someone else should chime in.
>>
>> The argument is that this was always invalid, but a bug that it wasn't detected, so it doesn't need to be upgraded when reading IR, it's OK for that to fail the verifier)
>>
>> Compared to an argument that it was valid, is now invalid, so needs to be upgraded to ensure bitcode compatibility?
>>
>> Do you think it'd be especially complicated to auto-upgrade?
>
> Yeah my argument is that it was always invalid.
>
> I'm sure it's possible to auto-upgrade bitcode, but that would have non-zero cost for every call we read from bitcode. IMO it's not worth it if this was always supposed to be invalid.
>
> Maybe we should ask llvm-dev for more opinions?

Yeah - if it's not possible to make that sort of IR from the APIs - if it'd require hand-crafted textual IR, or really weird/novel bitcode creation (please confirm whether/how hard it would be to have this in bitcode - if simple use of the IRBuilder/IR creation APIs could result in this problematic representation) I'd probably be OK calling it an "accepts invalid" bug and declaring it invalid.

Pending @rnk's concerns too.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101725