Transforms: Clone distinct nodes in metadata mapper unless RF_ReuseAndMutateDistinctMDs

[dexonsmith]

Summary

This is a follow up to 22a52dfddcefad4f275eb8ad1cc0e200074c2d8a and a
revert of df763188c9a1ecb1e7e5c4d4ea53a99fbb755903.

With this change, we only skip cloning distinct nodes in
MDNodeMapper::mapDistinct if RF_ReuseAndMutateDistinctMDs, dropping the
no-longer-needed local helper cloneOrBuildODR(). Skipping cloning in
other cases is unsound and breaks CloneModule, which is why the textual
IR for PR48841 didn't pass previously. This commit adds the test as:
Transforms/ThinLTOBitcodeWriter/cfi-debug-info-cloned-type-references-global-value.ll

Cloning less often exposed a hole in subprogram cloning in
CloneFunctionInto thanks to df763188c9a1ecb1e7e5c4d4ea53a99fbb755903's
test ThinLTO/X86/Inputs/dicompositetype-unique-alias.ll. If a function
has a subprogram attachment whose scope is a DICompositeType that
shouldn't be cloned, but it has no internal debug info pointing at that
type, that composite type was being cloned. This commit plugs that hole,
calling DebugInfoFinder::processSubprogram from CloneFunctionInto.

As hinted at in 22a52dfddcefad4f275eb8ad1cc0e200074c2d8a's commit
message, I think we need to formalize ownership of metadata a bit more
so that ValueMapper/CloneFunctionInto (and similar functions) can deal
with cloning (or not) metadata in a more generic, less fragile way.

This fixes PR48841.

Follow-up to: https://reviews.llvm.org/D96531
Revert of: https://reviews.llvm.org/D41669

[tejohnson]

Thanks. I am not very familiar with processSubprogram() or the implication of the other changes in CloneFunction.cpp, so will leave it to @aprantl or someone more familiar with the debug info details to review. I just have a small suggestion on the revert part.

llvm/lib/Transforms/Utils/ValueMapper.cpp

546

Why not just call MDNode::replaceWithDistinct(N.clone()) directly?

[dexonsmith]

Drop the one-line helper, cloneDistinct().

[dexonsmith]

llvm/lib/Transforms/Utils/ValueMapper.cpp

546

I had imagined this logic would get too busy, but actually this is fine. Thanks for the suggestion.

llvm/include/llvm/IR/DebugInfo.h

public:
   /// Process a single instruction and collect debug info anchors.
   void processInstruction(const Module &M, const Instruction &I);
 
   /// Process DbgVariableIntrinsic.
   void processVariable(const Module &M, const DbgVariableIntrinsic &DVI);
   /// Process debug info location.
   void processLocation(const Module &M, const DILocation *Loc);
 
+  /// Process subprogram.
+  void processSubprogram(DISubprogram *SP);
+
   /// Clear all lists.
   void reset();
 
 private:
   void InitializeTypeMap(const Module &M);
 
   void processCompileUnit(DICompileUnit *CU);
   void processScope(DIScope *Scope);
-  void processSubprogram(DISubprogram *SP);
   void processType(DIType *DT);
   bool addCompileUnit(DICompileUnit *CU);
   bool addGlobalVariable(DIGlobalVariableExpression *DIG);
   bool addScope(DIScope *Scope);
   bool addSubprogram(DISubprogram *SP);
   bool addType(DIType *DT);
 
 public:

llvm/lib/Transforms/Utils/CloneFunction.cpp

if (Changes < CloneFunctionChangeType::DifferentModule) {
     assert((NewFunc->getParent() == nullptr ||
             NewFunc->getParent() == OldFunc->getParent()) &&
            "Expected NewFunc to have the same parent, or no parent");
 
     // Need to find subprograms, types, and compile units.
     DIFinder.emplace();
 
     SPClonedWithinModule = OldFunc->getSubprogram();
+    if (SPClonedWithinModule)
+      DIFinder->processSubprogram(SPClonedWithinModule);
   } else {
     assert((NewFunc->getParent() == nullptr ||
             NewFunc->getParent() != OldFunc->getParent()) &&
            "Set SameModule to true if the new function is in the same module");
 
     if (Changes == CloneFunctionChangeType::DifferentModule) {
       assert(NewFunc->getParent() &&
              "Need parent of new function to maintain debug info invariants");
for (Function::const_iterator BI = OldFunc->begin(), BE = OldFunc->end();
     }
 
     // Note return instructions for the caller.
     if (ReturnInst *RI = dyn_cast<ReturnInst>(CBB->getTerminator()))
       Returns.push_back(RI);
   }
 
   if (Changes < CloneFunctionChangeType::DifferentModule &&
-      (SPClonedWithinModule || DIFinder->subprogram_count() > 0)) {
+      DIFinder->subprogram_count() > 0) {
     // Turn on module-level changes, since we need to clone (some of) the
     // debug info metadata.
     //
     // FIXME: Metadata effectively owned by a function should be made
     // local, and only that local metadata should be cloned.
     ModuleLevelChanges = true;
 
     auto mapToSelfIfNew = [&VMap](MDNode *N) {
       // Avoid clobbering an existing mapping.
       (void)VMap.MD().try_emplace(N, N);
     };
 
-    // Avoid cloning what the subprogram references.
-    if (SPClonedWithinModule) {
-      mapToSelfIfNew(SPClonedWithinModule->getUnit());
-      mapToSelfIfNew(SPClonedWithinModule->getType());
-      mapToSelfIfNew(SPClonedWithinModule->getFile());
-    }
-
-    // Avoid cloning other subprograms, compile units, and types.
+    // Avoid cloning types, compile units, and (other) subprograms.
     for (DISubprogram *ISP : DIFinder->subprograms())
       if (ISP != SPClonedWithinModule)
         mapToSelfIfNew(ISP);
 
     for (DICompileUnit *CU : DIFinder->compile_units())
       mapToSelfIfNew(CU);
 
     for (DIType *Type : DIFinder->types())
       mapToSelfIfNew(Type);
+  } else {
+    assert(!SPClonedWithinModule &&
+           "Subprogram should be in DIFinder->subprogram_count()...");
   }
 
   // Duplicate the metadata that is attached to the cloned function.
   // Subprograms/CUs/types that were already mapped to themselves won't be
   // duplicated.
   SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
   OldFunc->getAllMetadata(MDs);
   for (auto MD : MDs) {

llvm/lib/Transforms/Utils/ValueMapper.cpp

#endif
   }
 
   const MDNode &N = *cast<MDNode>(Op);
   if (N.isDistinct())
     return mapDistinctNode(N);
   return None;
 }
 
-static Metadata *cloneOrBuildODR(const MDNode &N) {
-  auto *CT = dyn_cast<DICompositeType>(&N);
-  // If ODR type uniquing is enabled, we would have uniqued composite types
-  // with identifiers during bitcode reading, so we can just use CT.
-  if (CT && CT->getContext().isODRUniquingDebugTypes() &&
-      CT->getIdentifier() != "")
-    return const_cast<DICompositeType *>(CT);
+static Metadata *cloneDistinct(const MDNode &N) {
   return MDNode::replaceWithDistinct(N.clone());
 }
 
 MDNode *MDNodeMapper::mapDistinctNode(const MDNode &N) {
   assert(N.isDistinct() && "Expected a distinct node");
   assert(!M.getVM().getMappedMD(&N) && "Expected an unmapped node");
   DistinctWorklist.push_back(
       cast<MDNode>((M.Flags & RF_ReuseAndMutateDistinctMDs)
                        ? M.mapToSelf(&N)
-                       : M.mapToMetadata(&N, cloneOrBuildODR(N))));
+                       : M.mapToMetadata(&N, cloneDistinct(N))));
   return DistinctWorklist.back();
 }
 
 static ConstantAsMetadata *wrapConstantAsMetadata(const ConstantAsMetadata &CMD,
                                                   Value *MappedV) {
   if (CMD.getValue() == MappedV)
     return const_cast<ConstantAsMetadata *>(&CMD);
   return MappedV ? ConstantAsMetadata::getConstant(MappedV) : nullptr;

llvm/test/Transforms/ThinLTOBitcodeWriter/cfi-debug-info-cloned-type-references-global-value.ll

+; RUN: opt -thinlto-bc -thinlto-split-lto-unit -o %t %s
+; RUN: llvm-modextract -b -n 0 -o - %t | llvm-dis | FileCheck %s
+
+; Crash test for CloneModule when there's a retained DICompositeType that
+; transitively references a global value.
+
+; CHECK: declare !type !{{[0-9]+}} !type !{{[0-9]+}} void @_Z1gIM1iKFivEEvT_(i64, i64)
+; CHECK: !llvm.dbg.cu
+; CHECK-DAG: distinct !DICompositeType({{.*}}, identifier: "_ZTS1oI1iiXadL_ZNKS0_5m_fn1EvEEE"
+; CHECK-DAG: distinct !DICompositeType({{.*}}, identifier: "_ZTS1i"
+; CHECK-DAG: !{i32 4, !"CFI Canonical Jump Tables", i32 0}
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@_ZN1i1pE = dso_local constant [1 x i8] zeroinitializer, align 1
+@_ZNK1i5m_fn1Ev = external global i32
+
+declare !type !17 !type !18 void @_Z1gIM1iKFivEEvT_(i64, i64)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!14, !15}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 12.0.0 ([e-mail]:llvm/llvm-project.git 51bf4c0e6d4cbc6dfa57857fc78003413cbeb17f)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, globals: !2, nameTableKind: None)
+!1 = !DIFile(filename: "<stdin>", directory: "/tmp")
+!2 = !{}
+!3 = !{!4}
+!4 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "o<i, int, &i::m_fn1>", file: !5, line: 22, size: 8, flags: DIFlagTypePassByValue | DIFlagNonTrivial, elements: !2, templateParams: !6, identifier: "_ZTS1oI1iiXadL_ZNKS0_5m_fn1EvEEE")
+!5 = !DIFile(filename: "t.ii", directory: "/tmp")
+!6 = !{!7}
+!7 = !DITemplateValueParameter(type: !8, value: i64 ptrtoint (i32* @_ZNK1i5m_fn1Ev to i64))
+!8 = !DIDerivedType(tag: DW_TAG_ptr_to_member_type, baseType: !9, size: 128, extraData: !13)
+!9 = !DISubroutineType(types: !10)
+!10 = !{!11, !12}
+!11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!12 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !11, size: 64, flags: DIFlagArtificial)
+!13 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "i", file: !5, line: 13, size: 8, flags: DIFlagTypePassByValue | DIFlagNonTrivial, elements: !2, identifier: "_ZTS1i")
+!14 = !{i32 2, !"Debug Info Version", i32 3}
+!15 = !{i32 4, !"CFI Canonical Jump Tables", i32 0}
+!16 = !{i64 ptrtoint (i32* @_ZNK1i5m_fn1Ev to i64)}
+!17 = !{i64 0, !"_ZTSFvM1iKFivEE"}
+!18 = !{i64 0, !"_ZTSFvM1iKFivEE.generalized"}