[TransformUtils] Don't generate invalid llvm.dbg.cu

[ldrumm]

Summary

CloneFunctionInto generated an empty (zero operand; invalid) compile
unit in the destination module even in cases where neither the source
nor dest function's parents had them. This caused verifier failures.

This fix ensures that in the case there is no compile-unit to copy from,
an empty one is not created

[dblaikie]

what was invalid about the metadata? The llvm.dbg.cu list was empty? I wonder if we should just say that's valid. It seems pretty benign/reasonable - makes it easier to write code like this, or code that, say, stripped a CU out wouldn't have to worry about removing a potentially zero-length cu list. @aprantl @JDevlieghere ?

[ldrumm]

In D99334#2659955, @dblaikie wrote:

what was invalid about the metadata? The llvm.dbg.cu list was empty? I wonder if we should just say that's valid. It seems pretty benign/reasonable - makes it easier to write code like this, or code that, say, stripped a CU out wouldn't have to worry about removing a potentially zero-length cu list. @aprantl @JDevlieghere ?

I don't think that's a good idea. It complexifies every piece of user code around handling "llvm.dbg.cu" if the user can't assume it actually contains what it's supposed to. Doing so is a tradeoff of one if statement in CloneFunctionInto becoming an extra if statement around every use of "llvm.dbg.cu". That's not a good tradeoff in terms of reliability or ergonomics.

[ldrumm]

ping

[dblaikie]

In D99334#2661021, @ldrumm wrote:

In D99334#2659955, @dblaikie wrote:

what was invalid about the metadata? The llvm.dbg.cu list was empty? I wonder if we should just say that's valid. It seems pretty benign/reasonable - makes it easier to write code like this, or code that, say, stripped a CU out wouldn't have to worry about removing a potentially zero-length cu list. @aprantl @JDevlieghere ?

I don't think that's a good idea. It complexifies every piece of user code around handling "llvm.dbg.cu" if the user can't assume it actually contains what it's supposed to. Doing so is a tradeoff of one if statement in CloneFunctionInto becoming an extra if statement around every use of "llvm.dbg.cu". That's not a good tradeoff in terms of reliability or ergonomics.

Why would code handling llvm.dbg.cu need to add an if statement? I'd expect it to walk through whatever's in llvm.dbg.cu and if there's nothing it'd be fine with that?

[ldrumm]

Why would code handling llvm.dbg.cu need to add an if statement? I'd expect it to walk through whatever's in llvm.dbg.cu and if there's nothing it'd be fine with that?

Because 3 lines later we have auto *NMD = NewModule->getOrInsertNamedMetadata("llvm.dbg.cu");
In the case that the source module has no llvm.dbu.cu the target module ends up with on without operands. This is both illegal and a pain to deal with in user code.

[dblaikie]

In D99334#2701760, @ldrumm wrote:

Why would code handling llvm.dbg.cu need to add an if statement? I'd expect it to walk through whatever's in llvm.dbg.cu and if there's nothing it'd be fine with that?

Because 3 lines later we have auto *NMD = NewModule->getOrInsertNamedMetadata("llvm.dbg.cu");
In the case that the source module has no llvm.dbu.cu the target module ends up with on without operands. This is both illegal and a pain to deal with in user code.

I don't understand why it would be either illegal or a pain to deal with. (& I'm not sure it's illegal either - I modified an example IR file to have an empty llvm.dbg.cu list and it seemed to compile file (compared to other violations of IR debug info verification, which did result in an error/warning like this:

inlinable function call in a function with debug info must have a !dbg location
  call void @_Z2f1v()
warning: ignoring invalid debug info in test.ll
```)

[ldrumm]

In D99334#2702227, @dblaikie wrote:

In D99334#2701760, @ldrumm wrote:

Why would code handling llvm.dbg.cu need to add an if statement? I'd expect it to walk through whatever's in llvm.dbg.cu and if there's nothing it'd be fine with that?

Because 3 lines later we have auto *NMD = NewModule->getOrInsertNamedMetadata("llvm.dbg.cu");
In the case that the source module has no llvm.dbu.cu the target module ends up with on without operands. This is both illegal and a pain to deal with in user code.

I don't understand why it would be either illegal or a pain to deal with. (& I'm not sure it's illegal either - I modified an example IR file to have an empty llvm.dbg.cu list and it seemed to compile file (compared to other violations of IR debug info verification, which did result in an error/warning like this:

inlinable function call in a function with debug info must have a !dbg location
  call void @_Z2f1v()
warning: ignoring invalid debug info in test.ll
```)

1. The verifier says it's illegal. When I wrote this patch the regression test included fails without this change
2. getting an llvm.dbg.cu, and then having to check it's got the right number of operands so you don't crash every time is a pain. It's partly about ergonomics - and partly about making it easy for users to do the right thing.

If you're still unhappy with this if statement can you suggest how I solve this bug in a more suitable way?

[dblaikie]

In D99334#2702363, @ldrumm wrote:

In D99334#2702227, @dblaikie wrote:

In D99334#2701760, @ldrumm wrote:

Why would code handling llvm.dbg.cu need to add an if statement? I'd expect it to walk through whatever's in llvm.dbg.cu and if there's nothing it'd be fine with that?

Because 3 lines later we have auto *NMD = NewModule->getOrInsertNamedMetadata("llvm.dbg.cu");
In the case that the source module has no llvm.dbu.cu the target module ends up with on without operands. This is both illegal and a pain to deal with in user code.

I don't understand why it would be either illegal or a pain to deal with. (& I'm not sure it's illegal either - I modified an example IR file to have an empty llvm.dbg.cu list and it seemed to compile file (compared to other violations of IR debug info verification, which did result in an error/warning like this:

inlinable function call in a function with debug info must have a !dbg location
  call void @_Z2f1v()
warning: ignoring invalid debug info in test.ll
```)

1. The verifier says it's illegal.

Where does it say that? it doesn't look to me like this would fail with a zero-length llvm.dbg.cu: https://github.com/microsoft/llvm/blob/eddbfad05ce3a85732d8bff9f85ef63afc97ad7e/lib/IR/Verifier.cpp#L758 & as mentioned in my last message, my testing seemed to show that a zero-length llvm.dbg.cu didn't trigger a failure in the IR verifier, unlike some IR I knew to be invalid (missing dbg location on a call in a dbg-having function)

When I wrote this patch the regression test included fails without this change

The unit test? It specifically tests for llvm.dbg.cu being null at the end, and it seems that's what fails without the code change - but I'm suggesting that test is incorrect, and that it's OK for llvm.dbg.cu to be non-null (but empty) here.

2. getting an llvm.dbg.cu, and then having to check it's got the right number of operands so you don't crash every time is a pain.

I don't understand what code you're suggesting here - it looks to me like all the LLVM code that visits llvm.dbg.cu iterates over it not caring about the total number of elements - if there are zero, the loop iterates zero times (see the verifier code linked above), if it's 10, the loop iterates 10 times - either is totally fine.

If you're still unhappy with this if statement can you suggest how I solve this bug in a more suitable way?

I'm suggesting this patch is not necessary, and the behavior of the existing code doesn't seem to me to be buggy.

llvm/lib/Transforms/Utils/CloneFunction.cpp

#endif
   //
   // FIXME: This is making global / module-level changes, which doesn't seem
   // like the right encapsulation  Consider dropping the requirement to update
   // !llvm.dbg.cu (either obsoleting the node, or restricting it to
   // non-discardable compile units) instead of discovering compile units by
   // visiting the metadata attached to global values, which would allow this
   // code to be deleted. Alternatively, perhaps give responsibility for this
   // update to CloneFunctionInto's callers.
+  if (!OldFunc->getParent()->getNamedMetadata("llvm.dbg.cu"))
+    return;
+
   auto *NewModule = NewFunc->getParent();
   auto *NMD = NewModule->getOrInsertNamedMetadata("llvm.dbg.cu");
   // Avoid multiple insertions of the same DICompileUnit to NMD.
   SmallPtrSet<const void *, 8> Visited;
   for (auto *Operand : NMD->operands())
     Visited.insert(Operand);
   for (auto *Unit : DIFinder->compile_units()) {
     MDNode *MappedUnit =

llvm/unittests/Transforms/Utils/CloningTest.cpp

TEST(CloneFunction, CloneFunctionToDifferentModule) {
 
   EXPECT_FALSE(verifyModule(*ImplModule, &errs()));
   EXPECT_FALSE(verifyModule(*DeclModule, &errs()));
   // DICompileUnit !2 shall be cloned into DeclModule.
   EXPECT_TRUE(GetDICompileUnitCount(*DeclModule) == 1);
   EXPECT_FALSE(haveCompileUnitsInCommon(*ImplModule, *DeclModule));
 }
 
+TEST(CloneFunction, CloneFunctionToDifferentModuleNoCU) {
+  StringRef ImplAssembly = R"(
+    define void @foo() {
+      ret void
+    }
+  )";
+  StringRef DeclAssembly = R"(
+    declare void @foo()
+  )";
+
+  LLVMContext Context;
+  SMDiagnostic Error;
+
+  auto ImplModule = parseAssemblyString(ImplAssembly, Error, Context);
+  EXPECT_TRUE(ImplModule != nullptr);
+  EXPECT_TRUE(ImplModule->getNamedMetadata("llvm.dbg.cu") == nullptr);
+  auto *ImplFunction = ImplModule->getFunction("foo");
+  EXPECT_TRUE(ImplFunction != nullptr);
+
+  auto DeclModule = parseAssemblyString(DeclAssembly, Error, Context);
+  EXPECT_TRUE(DeclModule != nullptr);
+  // No DICompileUnits defined here.
+  EXPECT_TRUE(GetDICompileUnitCount(*DeclModule) == 0);
+  auto *DeclFunction = DeclModule->getFunction("foo");
+  EXPECT_TRUE(DeclFunction != nullptr);
+
+  EXPECT_FALSE(verifyModule(*ImplModule, &errs()));
+  EXPECT_FALSE(verifyModule(*DeclModule, &errs()));
+
+  ValueToValueMapTy VMap;
+  VMap[ImplFunction] = DeclFunction;
+  // No args to map
+  SmallVector<ReturnInst *, 8> Returns;
+  CloneFunctionInto(DeclFunction, ImplFunction, VMap,
+                    CloneFunctionChangeType::DifferentModule, Returns);
+
+  EXPECT_FALSE(verifyModule(*ImplModule, &errs()));
+  EXPECT_FALSE(verifyModule(*DeclModule, &errs()));
+  // DeclModule should not have a compile unit because it didn't start with one
+  // and nor did ImplModule
+  EXPECT_TRUE(DeclModule->getNamedMetadata("llvm.dbg.cu") == nullptr);
+}
+
 class CloneModule : public ::testing::Test {
 protected:
   void SetUp() override {
     SetupModule();
     CreateOldModule();
     CreateNewModule();
   }