[PATCH] D139209: [IRMover] Remove UB implying parameter attributes when necessary

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/lib/Linker/IRMover.cpp:1577-1579
+  if (DstF && SrcF && DstF->isDeclaration() && !SrcF->isDeclaration()) {
+    assert(DstF->arg_size() == SrcF->arg_size() &&
+           "Dst and Src should have the same signature.");
----------------
TimNN wrote:
> tejohnson wrote:
> > TimNN wrote:
> > > arsenm wrote:
> > > > TimNN wrote:
> > > > > arsenm wrote:
> > > > > > I thought we casted the function type at callsites for different signatures in different modules, so this wouldn't hold?
> > > > > @tejohnson do you have any input on this?
> > > > > 
> > > > > (I last looked at this code in-depth in early December, but don't remember seeing anything relevant. `ninja check-all` is also clean and in a brief test `IRMover` would happily import a function with a different signature without creating any bitcasts).
> > > > These days it would appear as a callsite with a different type from the callee, there's no explicit bitcast with opaque pointers
> > > Ack, then I guess the "brief test" wasn't really useful.
> > > 
> > > Anyway, if it is indeed expected that `SrcF` and `DstF` have different signatures here, I'd appreciate some guidance on how to best detect and handle that.
> > Sorry I'm not really sure. @arsenm do you mean that the arg_size could be different? Do you have an example of how/when that would happen?
> Friendly ping, @arsenm! Could you provide more details about your concern, or even better, a concrete example?
> 
I think what @arsenm is talking about is a separate thing that isn't relevant here. The callsite thing is on function calls (`CallBase::getCalledFunction()` returns `nullptr` if the call args and callee are mismatched), but this is about handling function declarations/definitions of the same function.

If there's any updating of callers of a function, that would be happening somewhere else, not in the code touched by this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139209