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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Linker/IRMover.cpp:1554
   /// it has no effect on definitions in the same module.
-  if (auto *F = dyn_cast<Function>(&GV)) {
-    if (!F->isIntrinsic())
-      F->removeFnAttr(llvm::Attribute::NoCallback);
+  if (DstF) {
+    if (!DstF->isIntrinsic())
----------------
How about just do an early return if !DstF, since your changes below also rely on DstF being non-null.


================
Comment at: llvm/lib/Linker/IRMover.cpp:1568
+  // If `F` was a declaration in its source module but is (already) a
+  // definition in the destination module, then the signatures of the two
+  // functions may have diverged (even if they were originally idential) due to
----------------
I think this is backwards? My understanding (also from the code and the test case) is that the issue occurs if F is a declaration in the dest module but was a definition in the source module.


================
Comment at: llvm/lib/Linker/IRMover.cpp:1561
       for (Instruction &I : BB)
         if (CallInst *CI = dyn_cast<CallInst>(&I))
           CI->removeFnAttr(Attribute::NoCallback);
----------------
TimNN wrote:
> arsenm wrote:
> > Drive by comment, but is this brokenly not checking for CallBase?
> I'm not familiary enough with this code / LLVM to answer this. cc @gulfem who added this in D137360.
> 
> Anyway, marking this comment as "done" because if this needs to be fixed, it shouldn't be part of this change.
Thanks for noting that, it is a limitation with the earlier patch that I missed. I will copy your comment over there.


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139209