[Bitcode] Ensure DIArgList in bitcode has no null or forward metadata refs

This patch fixes an issue in which ConstantAsMetadata arguments to a
DIArglist, as well as the Constant values referenced by that metadata,
would not be always be emitted correctly into bitcode. This patch fixes
this issue firstly by searching for ConstantAsMetadata in DIArgLists
(previously we would only search for them when directly wrapped in
MetadataAsValue), and secondly by enumerating all of a DIArgList's
arguments directly prior to enumerating the DIArgList itself.

This patch also adds a number of asserts, and no longer treats the
arguments to a DIArgList as optional fields when reading/writing to
bitcode.

Differential Revision: https://reviews.llvm.org/D100572

Author: SLTozer

llvm/lib/Bitcode/Reader/MetadataLoader.cpp

@@ -2078,8 +2078,15 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata(
   case bitc::METADATA_ARG_LIST: {
     SmallVector<ValueAsMetadata *, 4> Elts;
     Elts.reserve(Record.size());
-    for (uint64_t Elt : Record)
-      Elts.push_back(dyn_cast_or_null<ValueAsMetadata>(getMDOrNull(Elt)));
+    for (uint64_t Elt : Record) {
+      Metadata *MD = getMD(Elt);
+      if (isa<MDNode>(MD) && cast<MDNode>(MD)->isTemporary())
+        return error(
+            "Invalid record: DIArgList should not contain forward refs");
+      if (!isa<ValueAsMetadata>(MD))
+        return error("Invalid record");
+      Elts.push_back(cast<ValueAsMetadata>(MD));
+    }
 
     MetadataList.assignValue(DIArgList::get(Context, Elts), NextMetadataNo);
     NextMetadataNo++;

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

@@ -1876,7 +1876,7 @@ void ModuleBitcodeWriter::writeDIArgList(const DIArgList *N,
                                          unsigned Abbrev) {
   Record.reserve(N->getArgs().size());
   for (ValueAsMetadata *MD : N->getArgs())
-    Record.push_back(VE.getMetadataOrNullID(MD));
+    Record.push_back(VE.getMetadataID(MD));
 
   Stream.EmitRecord(bitc::METADATA_ARG_LIST, Record, Abbrev);
   Record.clear();

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

@@ -78,16 +78,6 @@ struct OrderMap {
 
 } // end anonymous namespace
 
-/// Look for a value that might be wrapped as metadata, e.g. a value in a
-/// metadata operand. Returns nullptr for a non-wrapped input value if
-/// OnlyWrapped is true, or it returns the input value as-is if false.
-static const Value *skipMetadataWrapper(const Value *V, bool OnlyWrapped) {
-  if (const auto *MAV = dyn_cast<MetadataAsValue>(V))
-    if (const auto *VAM = dyn_cast<ValueAsMetadata>(MAV->getMetadata()))
-      return VAM->getValue();
-  return OnlyWrapped ? nullptr : V;
-}
-
 static void orderValue(const Value *V, OrderMap &OM) {
   if (OM.lookup(V).first)
     return;
@@ -139,16 +129,25 @@ static OrderMap orderModule(const Module &M) {
   // these before global values, as these will be read before setting the
   // global values' initializers. The latter matters for constants which have
   // uses towards other constants that are used as initializers.
+  auto orderConstantValue = [&OM](const Value *V) {
+    if ((isa<Constant>(V) && !isa<GlobalValue>(V)) || isa<InlineAsm>(V))
+      orderValue(V, OM);
+  };
   for (const Function &F : M) {
     if (F.isDeclaration())
       continue;
     for (const BasicBlock &BB : F)
       for (const Instruction &I : BB)
         for (const Value *V : I.operands()) {
-          if (const Value *Op = skipMetadataWrapper(V, true)) {
-            if ((isa<Constant>(*Op) && !isa<GlobalValue>(*Op)) ||
-                isa<InlineAsm>(*Op))
-              orderValue(Op, OM);
+          if (const auto *MAV = dyn_cast<MetadataAsValue>(V)) {
+            if (const auto *VAM =
+                    dyn_cast<ValueAsMetadata>(MAV->getMetadata())) {
+              orderConstantValue(VAM->getValue());
+            } else if (const auto *AL =
+                           dyn_cast<DIArgList>(MAV->getMetadata())) {
+              for (const auto *VAM : AL->getArgs())
+                orderConstantValue(VAM->getValue());
+            }
           }
         }
   }
@@ -448,10 +447,17 @@ ValueEnumerator::ValueEnumerator(const Module &M,
             continue;
           }
 
-          // Local metadata is enumerated during function-incorporation.
-          if (isa<LocalAsMetadata>(MD->getMetadata()) ||
-              isa<DIArgList>(MD->getMetadata()))
+          // Local metadata is enumerated during function-incorporation, but
+          // any ConstantAsMetadata arguments in a DIArgList should be examined
+          // now.
+          if (isa<LocalAsMetadata>(MD->getMetadata()))
+            continue;
+          if (auto *AL = dyn_cast<DIArgList>(MD->getMetadata())) {
+            for (auto *VAM : AL->getArgs())
+              if (isa<ConstantAsMetadata>(VAM))
+                EnumerateMetadata(&F, VAM);
             continue;
+          }
 
           EnumerateMetadata(&F, MD->getMetadata());
         }
@@ -620,6 +626,11 @@ void ValueEnumerator::EnumerateFunctionLocalMetadata(
   EnumerateFunctionLocalMetadata(getMetadataFunctionID(&F), Local);
 }
 
+void ValueEnumerator::EnumerateFunctionLocalListMetadata(
+    const Function &F, const DIArgList *ArgList) {
+  EnumerateFunctionLocalListMetadata(getMetadataFunctionID(&F), ArgList);
+}
+
 void ValueEnumerator::dropFunctionFromMetadata(
     MetadataMapType::value_type &FirstMD) {
   SmallVector<const MDNode *, 64> Worklist;
@@ -730,7 +741,7 @@ const MDNode *ValueEnumerator::enumerateMetadataImpl(unsigned F, const Metadata
   return nullptr;
 }
 
-/// EnumerateFunctionLocalMetadataa - Incorporate function-local metadata
+/// EnumerateFunctionLocalMetadata - Incorporate function-local metadata
 /// information reachable from the metadata.
 void ValueEnumerator::EnumerateFunctionLocalMetadata(
     unsigned F, const LocalAsMetadata *Local) {
@@ -750,6 +761,39 @@ void ValueEnumerator::EnumerateFunctionLocalMetadata(
   EnumerateValue(Local->getValue());
 }
 
+/// EnumerateFunctionLocalListMetadata - Incorporate function-local metadata
+/// information reachable from the metadata.
+void ValueEnumerator::EnumerateFunctionLocalListMetadata(
+    unsigned F, const DIArgList *ArgList) {
+  assert(F && "Expected a function");
+
+  // Check to see if it's already in!
+  MDIndex &Index = MetadataMap[ArgList];
+  if (Index.ID) {
+    assert(Index.F == F && "Expected the same function");
+    return;
+  }
+
+  for (ValueAsMetadata *VAM : ArgList->getArgs()) {
+    if (isa<LocalAsMetadata>(VAM)) {
+      assert(MetadataMap.count(VAM) &&
+             "LocalAsMetadata should be enumerated before DIArgList");
+      assert(MetadataMap[VAM].F == F &&
+             "Expected LocalAsMetadata in the same function");
+    } else {
+      assert(isa<ConstantAsMetadata>(VAM) &&
+             "Expected LocalAsMetadata or ConstantAsMetadata");
+      assert(ValueMap.count(VAM->getValue()) &&
+             "Constant should be enumerated beforeDIArgList");
+      EnumerateMetadata(F, VAM);
+    }
+  }
+
+  MDs.push_back(ArgList);
+  Index.F = F;
+  Index.ID = MDs.size();
+}
+
 static unsigned getMetadataTypeOrder(const Metadata *MD) {
   // Strings are emitted in bulk and must come first.
   if (isa<MDString>(MD))
@@ -1072,7 +1116,7 @@ void ValueEnumerator::incorporateFunction(const Function &F) {
   // DIArgList entries must come after function-local metadata, as it is not
   // possible to forward-reference them.
   for (const DIArgList *ArgList : ArgListMDVector)
-    EnumerateMetadata(&F, ArgList);
+    EnumerateFunctionLocalListMetadata(F, ArgList);
 }
 
 void ValueEnumerator::purgeFunction() {

llvm/lib/Bitcode/Writer/ValueEnumerator.h

@@ -27,6 +27,7 @@ namespace llvm {
 
 class BasicBlock;
 class Comdat;
+class DIArgList;
 class Function;
 class Instruction;
 class LocalAsMetadata;
@@ -286,6 +287,9 @@ class ValueEnumerator {
   void EnumerateFunctionLocalMetadata(const Function &F,
                                       const LocalAsMetadata *Local);
   void EnumerateFunctionLocalMetadata(unsigned F, const LocalAsMetadata *Local);
+  void EnumerateFunctionLocalListMetadata(const Function &F,
+                                          const DIArgList *ArgList);
+  void EnumerateFunctionLocalListMetadata(unsigned F, const DIArgList *Arglist);
   void EnumerateNamedMDNode(const NamedMDNode *NMD);
   void EnumerateValue(const Value *V);
   void EnumerateType(Type *T);

llvm/test/DebugInfo/Generic/debug_value_list.ll

@@ -8,17 +8,17 @@ target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16
 target triple = "x86_64-pc-windows-msvc19.16.27034"
 
 ; CHECK-COUNT-3: llvm.dbg.value(
-; CHECK-SAME: metadata !DIArgList(i32 %a, i32 %b)
+; CHECK-SAME: metadata !DIArgList(i32 %a, i32 %b, i32 5)
 ; CHECK-SAME: metadata !16,
-; CHECK-SAME: metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus)
+; CHECK-SAME: metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus, DW_OP_LLVM_arg, 2, DW_OP_plus)
 define dso_local i32 @"?foo@@YAHHH@Z"(i32 %a, i32 %b) local_unnamed_addr !dbg !8 {
 entry:
   call void @llvm.dbg.value(metadata !DIArgList(i32 %b), metadata !14, metadata !DIExpression(DW_OP_LLVM_arg, 0)), !dbg !17
   call void @llvm.dbg.value(metadata !DIArgList(i32 %a), metadata !15, metadata !DIExpression(DW_OP_LLVM_arg, 0)), !dbg !17
   call void @llvm.dbg.value(
-    metadata !DIArgList(i32 %a, i32 %b),
+    metadata !DIArgList(i32 %a, i32 %b, i32 5),
     metadata !16,
-    metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus)), !dbg !17
+    metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus, DW_OP_LLVM_arg, 2, DW_OP_plus)), !dbg !17
   %mul = mul nsw i32 %b, %a, !dbg !18
   ret i32 %mul, !dbg !18
 }