Fix the bitcode upgrade for DIGlobalVariable in a DIImportedEntity context.

[aprantl]

Summary

The bitcode upgrade for DIGlobalVariable unconditionally wrapped DIGlobalVariables in a DIGlobalVariableExpression. When a DIGlobalVariable is referenced by a DIImportedEntity, however, this is wrong. This patch fixes the bitcode upgrade by deferring the creation of DIGlobalVariableExpressions until we know the context of the DIGlobalVariable.

rdar://problem/30134279

[mehdi_amini]

lib/Bitcode/Reader/MetadataLoader.cpp

796

Should we have a single entry point "performDebugInfoUpgrade" instead of duplicating the calls at every exit point?

1466–1470

Does this need to be set to true when DGVE is correctly set below?

1474

Are we using C-style cast elsewhere in similar situation?

Also the logic seems quite complex to me (it was already, but getting worse) around the invariant that link Attach and Expr and what we end up assigning to NextMetadataNo, or attaching.

[aprantl]

Address review comments

[aprantl]

Fix whitespace.

[mehdi_amini]

LGTM. Thanks!

lib/Bitcode/Reader/MetadataLoader.cpp

class MetadataLoader::MetadataLoaderImpl {
   /// metadata.
   SmallDenseMap<Function *, DISubprogram *, 16> FunctionsWithSPs;
 
   // Map the bitcode's custom MDKind ID to the Module's MDKind ID.
   DenseMap<unsigned, unsigned> MDKindMap;
 
   bool StripTBAA = false;
   bool HasSeenOldLoopTags = false;
+  bool NeedUpgradeToDIGlobalVariableExpression = false;
 
   /// True if metadata is being parsed for a module being ThinLTO imported.
   bool IsImporting = false;
 
   Error parseOneMetadata(SmallVectorImpl<uint64_t> &Record, unsigned Code,
                          PlaceholderQueue &Placeholders, StringRef Blob,
                          unsigned &NextMetadataNo);
   Error parseMetadataStrings(ArrayRef<uint64_t> Record, StringRef Blob,
void upgradeCUSubprograms() {
     for (auto CU_SP : CUSubprograms)
       if (auto *SPs = dyn_cast_or_null<MDTuple>(CU_SP.second))
         for (auto &Op : SPs->operands())
           if (auto *SP = dyn_cast_or_null<MDNode>(Op))
             SP->replaceOperandWith(7, CU_SP.first);
     CUSubprograms.clear();
   }
 
+  /// Upgrade old-style bare DIGlobalVariables to DIGlobalVariableExpressions.
+  void upgradeCUVariables() {
+    if (!NeedUpgradeToDIGlobalVariableExpression)
+      return;
+
+    // Upgrade list of variables attached to the CUs.
+    if (NamedMDNode *CUNodes = TheModule.getNamedMetadata("llvm.dbg.cu"))
+      for (unsigned I = 0, E = CUNodes->getNumOperands(); I != E; ++I) {
+        auto *CU = cast<DICompileUnit>(CUNodes->getOperand(I));
+        if (auto *GVs = dyn_cast_or_null<MDTuple>(CU->getRawGlobalVariables()))
+          for (unsigned I = 0; I < GVs->getNumOperands(); I++)
+            if (auto *GV =
+                    dyn_cast_or_null<DIGlobalVariable>(GVs->getOperand(I))) {
+              auto *DGVE =
+                  DIGlobalVariableExpression::getDistinct(Context, GV, nullptr);
+              GVs->replaceOperandWith(I, DGVE);
+            }
+      }
+
+    // Upgrade variables attached to globals.
+    for (auto &GV : TheModule.globals()) {
+      SmallVector<MDNode *, 1> MDs, NewMDs;
+      GV.getMetadata(LLVMContext::MD_dbg, MDs);
+      GV.eraseMetadata(LLVMContext::MD_dbg);
+      for (auto *MD : MDs)
+        if (auto *DGV = dyn_cast_or_null<DIGlobalVariable>(MD)) {
+          auto *DGVE =
+              DIGlobalVariableExpression::getDistinct(Context, DGV, nullptr);
+          GV.addMetadata(LLVMContext::MD_dbg, *DGVE);
+        } else
+          GV.addMetadata(LLVMContext::MD_dbg, *MD);
+    }
+  }
+  void upgradeDebugInfo() {
+    upgradeCUSubprograms();
+    upgradeCUVariables();
+  }
+
 public:
   MetadataLoaderImpl(BitstreamCursor &Stream, Module &TheModule,
                      BitcodeReaderValueList &ValueList,
                      std::function<Type *(unsigned)> getTypeByID,
                      bool IsImporting)
       : MetadataList(TheModule.getContext()), ValueList(ValueList),
         Stream(Stream), Context(TheModule.getContext()), TheModule(TheModule),
         getTypeByID(std::move(getTypeByID)), IsImporting(IsImporting) {}
if (SuccessOrErr.get()) {
       // An index was successfully created and we will be able to load metadata
       // on-demand.
       MetadataList.resize(MDStringRef.size() +
                           GlobalMetadataBitPosIndex.size());
 
       // Reading the named metadata created forward references and/or
       // placeholders, that we flush here.
       resolveForwardRefsAndPlaceholders(Placeholders);
-      upgradeCUSubprograms();
+      upgradeDebugInfo();
       // Return at the beginning of the block, since it is easy to skip it
       // entirely from there.
       Stream.ReadBlockEnd(); // Pop the abbrev block context.
       Stream.JumpToBit(EntryPos);
       if (Stream.SkipBlock())
         return error("Invalid record");
       return Error::success();
     }
     // Couldn't load an index, fallback to loading all the block "old-style".
   }
 
   unsigned NextMetadataNo = MetadataList.size();
 
   // Read all the records.
   while (true) {
     BitstreamEntry Entry = Stream.advanceSkippingSubblocks();
 
     switch (Entry.Kind) {
     case BitstreamEntry::SubBlock: // Handled for us already.
     case BitstreamEntry::Error:
       return error("Malformed block");
     case BitstreamEntry::EndBlock:
       resolveForwardRefsAndPlaceholders(Placeholders);
-      upgradeCUSubprograms();
+      upgradeDebugInfo();
       return Error::success();
     case BitstreamEntry::Record:
       // The interesting case.
       break;
     }
 
     // Read a record.
     Record.clear();
     StringRef Blob;
if (Version == 1) {
       }
       DIGlobalVariable *DGV = GET_OR_DISTINCT(
           DIGlobalVariable,
           (Context, getMDOrNull(Record[1]), getMDString(Record[2]),
            getMDString(Record[3]), getMDOrNull(Record[4]), Record[5],
            getDITypeRefOrNull(Record[6]), Record[7], Record[8],
            getMDOrNull(Record[10]), AlignInBits));
 
-      auto *DGVE = DIGlobalVariableExpression::getDistinct(Context, DGV, Expr);
-      MetadataList.assignValue(DGVE, NextMetadataNo);
-      NextMetadataNo++;
+      DIGlobalVariableExpression *DGVE = nullptr;
+      if (Attach || Expr)
+        DGVE = DIGlobalVariableExpression::getDistinct(Context, DGV, Expr);
+      else
+        NeedUpgradeToDIGlobalVariableExpression = true;
       if (Attach)
         Attach->addDebugInfo(DGVE);
+
+      auto *MDNode = Expr ? cast<Metadata>(DGVE) : cast<Metadata>(DGV);
+      MetadataList.assignValue(MDNode, NextMetadataNo);
+      NextMetadataNo++;
     } else
       return error("Invalid record");
 
     break;
   }
   case bitc::METADATA_LOCAL_VAR: {
     // 10th field is for the obseleted 'inlinedAt:' field.
     if (Record.size() < 8 || Record.size() > 10)

test/Bitcode/DIGlobalVariableExpression.ll

 ; RUN: llvm-dis -o - %s.bc | FileCheck %s
 ; RUN: llvm-dis -o - %s.bc | llvm-as - | llvm-bcanalyzer -dump - | FileCheck %s --check-prefix=BC
 
 ; BC: GLOBAL_VAR_EXPR
 ; BC: GLOBAL_DECL_ATTACHMENT
 ; CHECK: @g = common global i32 0, align 4, !dbg ![[G:[0-9]+]]
 ; CHECK: @h = common global i32 0, align 4, !dbg ![[H:[0-9]+]]
 ; CHECK: ![[G]] = {{.*}}!DIGlobalVariableExpression(var: ![[GVAR:[0-9]+]], expr: ![[GEXPR:[0-9]+]])
 ; CHECK: ![[GVAR]] = distinct !DIGlobalVariable(name: "g",
+; CHECK: DICompileUnit({{.*}}, imports: ![[IMPORTS:[0-9]+]]
 ; CHECK: !DIGlobalVariableExpression(var: ![[CVAR:[0-9]+]], expr: ![[CEXPR:[0-9]+]])
 ; CHECK: ![[CVAR]] = distinct !DIGlobalVariable(name: "c",
 ; CHECK: ![[CEXPR]] = !DIExpression(DW_OP_constu, 23, DW_OP_stack_value)
-; CHECK: ![[H]] = {{.*}}!DIGlobalVariableExpression(var: ![[HVAR:[0-9]+]])
-; CHECK: ![[HVAR]] = distinct !DIGlobalVariable(name: "h",
+; CHECK: ![[HVAR:[0-9]+]] = distinct !DIGlobalVariable(name: "h",
+; CHECK: ![[IMPORTS]] = !{![[CIMPORT:[0-9]+]]}
+; CHECK: ![[CIMPORT]] = !DIImportedEntity({{.*}}entity: ![[HVAR]]
 ; CHECK: ![[GEXPR]] = !DIExpression(DW_OP_plus, 1)
+; CHECK: ![[H]] = {{.*}}!DIGlobalVariableExpression(var: ![[HVAR]])
+
 @g = common global i32 0, align 4, !dbg !0
 @h = common global i32 0, align 4, !dbg !11
 
 !llvm.dbg.cu = !{!1}
 !llvm.module.flags = !{!6, !7, !8}
 !llvm.ident = !{!9}
 
 !0 = distinct !DIGlobalVariable(name: "g", scope: !1, file: !2, line: 1, type: !5, isLocal: false, isDefinition: true, expr: !DIExpression(DW_OP_plus, 1))
-!1 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2, producer: "clang version 4.0.0 (trunk 286129) (llvm/trunk 286128)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !3, globals: !4)
+!1 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2, producer: "clang version 4.0.0 (trunk 286129) (llvm/trunk 286128)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, globals: !4, imports: !3)
 !2 = !DIFile(filename: "a.c", directory: "/")
-!3 = !{}
+!3 = !{!12}
 !4 = !{!0, !10, !11}
 !5 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
 !6 = !{i32 2, !"Dwarf Version", i32 4}
 !7 = !{i32 2, !"Debug Info Version", i32 3}
 !8 = !{i32 1, !"PIC Level", i32 2}
 !9 = !{!"clang version 4.0.0 (trunk 286129) (llvm/trunk 286128)"}
 !10 = distinct !DIGlobalVariable(name: "c", scope: !1, file: !2, line: 1, type: !5, isLocal: false, isDefinition: true, expr: !DIExpression(DW_OP_constu, 23, DW_OP_stack_value))
 !11 = distinct !DIGlobalVariable(name: "h", scope: !1, file: !2, line: 2, type: !5, isLocal: false, isDefinition: true)
+!12 = !DIImportedEntity(tag: DW_TAG_imported_declaration, line: 1, scope: !1, entity: !11)

test/Bitcode/dityperefs-3.8.ll

 ; CHECK-NEXT: !2 = !DICompositeType(tag: DW_TAG_structure_type, name: "T2", scope: !1{{.*}}, baseType: !1, vtableHolder: !1, identifier: "T2")
 ; CHECK-NEXT: !3 = !DIDerivedType(tag: DW_TAG_member, name: "M1", scope: !1{{.*}}, baseType: !2)
 ; CHECK-NEXT: !4 = !DISubroutineType(types: !5)
 ; CHECK-NEXT: !5 = !{!1, !2}
 ; CHECK-NEXT: !6 = !DISubprogram(scope: !1,{{.*}} containingType: !1{{[,)]}}
 ; CHECK-NEXT: !7 = !DILocalVariable(name: "V1", scope: !6, type: !2)
 ; CHECK-NEXT: !8 = !DIObjCProperty(name: "P1", type: !1)
 ; CHECK-NEXT: !9 = !DITemplateTypeParameter(type: !1)
-; CHECK-NEXT: !10 = distinct !DIGlobalVariableExpression(var: !11)
-; CHECK-NEXT: !11 = !DIGlobalVariable(name: "G",{{.*}} type: !1,
-; CHECK-NEXT: !12 = !DITemplateValueParameter(type: !1, value: i32* @G1)
-; CHECK-NEXT: !13 = !DIImportedEntity(tag: DW_TAG_imported_module, name: "T2", scope: !0, entity: !1)
-; CHECK-NEXT: !14 = !DICompositeType(tag: DW_TAG_structure_type, name: "T3", file: !0, elements: !15, identifier: "T3")
-; CHECK-NEXT: !15 = !{!16}
-; CHECK-NEXT: !16 = !DISubprogram(scope: !14,
-; CHECK-NEXT: !17 = !DIDerivedType(tag: DW_TAG_ptr_to_member_type,{{.*}} extraData: !14)
+; CHECK-NEXT: !10 = !DIGlobalVariable(name: "G",{{.*}} type: !1,
+; CHECK-NEXT: !11 = !DITemplateValueParameter(type: !1, value: i32* @G1)
+; CHECK-NEXT: !12 = !DIImportedEntity(tag: DW_TAG_imported_module, name: "T2", scope: !0, entity: !1)
+; CHECK-NEXT: !13 = !DICompositeType(tag: DW_TAG_structure_type, name: "T3", file: !0, elements: !14, identifier: "T3")
+; CHECK-NEXT: !14 = !{!15}
+; CHECK-NEXT: !15 = !DISubprogram(scope: !13,
+; CHECK-NEXT: !16 = !DIDerivedType(tag: DW_TAG_ptr_to_member_type,{{.*}} extraData: !13)
 
 !0 = !DIFile(filename: "path/to/file", directory: "/path/to/dir")
 !1 = !DICompositeType(tag: DW_TAG_structure_type, name: "T1", file: !0, identifier: "T1")
 !2 = !DICompositeType(tag: DW_TAG_structure_type, name: "T2", file: !0, scope: !"T1", baseType: !"T1", vtableHolder: !"T1", identifier: "T2")
 !3 = !DIDerivedType(tag: DW_TAG_member, name: "M1", file: !0, scope: !"T1", baseType: !"T2")
 !4 = !DISubroutineType(types: !5)
 !5 = !{!"T1", !"T2"}
 !6 = !DISubprogram(scope: !"T1", isDefinition: false, containingType: !"T1")