[PATCH] D32975: Make it illegal for two Functions to point to the same DISubprogram

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Looks pretty good to me



================
Comment at: lib/IR/DebugLoc.cpp:167
+  // Fix up debug variables to point to NewSP.
+  auto reparentVar = [&](DILocalVariable *Var) -> DILocalVariable * {
+    return DILocalVariable::getDistinct(
----------------
Probably not need to explicitly state the return type here, unless you particularly like/want to do so?


================
Comment at: lib/Transforms/Utils/CloneFunction.cpp:131
+        (MD.first == LLVMContext::MD_dbg && OldFunc->getParent() &&
+         OldFunc->getParent() == NewFunc->getParent());
+    if (MustCloneSP) {
----------------
Is this ever not true? I guess it could be if CloneFunction is used to clone from one Module to another? is that a thing that happens?


================
Comment at: lib/Transforms/Utils/CloneFunction.cpp:171-172
     if (BB.hasAddressTaken()) {
-      Constant *OldBBAddr = BlockAddress::get(const_cast<Function*>(OldFunc),
-                                              const_cast<BasicBlock*>(&BB));
+      Constant *OldBBAddr = BlockAddress::get(const_cast<Function *>(OldFunc),
+                                              const_cast<BasicBlock *>(&BB));
       VMap[OldBBAddr] = BlockAddress::get(NewFunc, CBB);
----------------
Unrelated formatting - probably revert that.


================
Comment at: unittests/Transforms/Utils/Cloning.cpp:364
 
-  EXPECT_TRUE(Sub == OldFunc->getSubprogram());
-  EXPECT_TRUE(Sub == NewFunc->getSubprogram());
+  EXPECT_FALSE(NewFunc->getSubprogram() == OldFunc->getSubprogram());
 }
----------------
EXPECT_NE ?


================
Comment at: unittests/Transforms/Utils/Cloning.cpp:391-392
       // But that they belong to different functions
-      auto *OldSubprogram = cast<DISubprogram>(OldDL.getScope());
-      auto *NewSubprogram = cast<DISubprogram>(NewDL.getScope());
+      auto *OldSubprogram = cast<DISubprogram>(OldDL.getInlinedAtScope());
+      auto *NewSubprogram = cast<DISubprogram>(NewDL.getInlinedAtScope());
       EXPECT_EQ(OldFunc->getSubprogram(), OldSubprogram);
----------------
I don't really understand why this change is here - could you explain it?

I guess the actual change you made to CloneFunction makes the new function look as though the old function was inlined into it? That's a great idea/way to do it! :) (though won't be what I need for the LTO+Fission issue - but perhaps I can use some of the pieces you've built/generalized/etc here)


https://reviews.llvm.org/D32975