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

[StephenTozer]

Summary

This solves an issue seen in a Chromium build, which resulted from incorrect bitcode read/write handling for DIArgList. The specific cause of the issue is that previously, when writing DIArgList to bitcode, there was no guarantee that all of its arguments had been written to bitcode beforehand. DIArgList is strict about its arguments, and will only accept ValueAsMetadata*, so forward MD references (temporary MDNode*s) are not valid when constructing a DIArgList object. It also meant that if a ConstantAsMetadata was only referenced in a DIArgList, it would not be written at all, resulting in a null record.

This issue was further obfuscated by the fact that DIArgList was not being stringent enough when reading and writing bitcode records; although DIArgList does not support null values for its arguments, it was allowing the argument records to be null.

This has been fixed by adding a new method to the ValueEnumerator to handle DIArgList that enumerates any ConstantAsMetadata arguments and contains a number of DIArgList-specific assertions. The read/write code has also been modified to appropriately assert that its arguments are non-null.

[StephenTozer]

In the previous version of the patch, the actual Constant values used by the ConstantAsMetadata arguments to DIArgList were not being enumerated until the function that used them was incorporated, which resulted in incorrect bitcode output (all constant values in the bitcode are expected to appear at the module level).

[jmorse]

LGTM, I enjoy the minimal extension of a test to improve coverage too.

I'm a little twitchy about whether this will affect performance in what's an important path for LTO builds: on balance, I don't think it will to any significant extent.

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

80–81

I worry about putting some extra loops and bigger return value in what seems to be a hot path (touches all Values once). On the other hand, this should be trivially inlined and DIArgLists won't be common.

[StephenTozer]

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

80–81

I agree actually; I folded it this way for simplicity's sake, but we can really handle ValueAsMetadata and DIArgList as separate cases. Even with inlining, I worry that the use of SmallVector here might obstruct potential optimizations for the non-DIArgList case. It might only have a small impact, but it's easy to fix nonetheless.

[StephenTozer]

Reduce possible performance issues in constant value ordering.

[jmorse]

LGTM

llvm/lib/Bitcode/Reader/MetadataLoader.cpp

case bitc::METADATA_KIND: {
     // block with METADATA_BLOCK_ID.
     if (Error Err = parseMetadataKindRecord(Record))
       return Err;
     break;
   }
   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++;
     break;
   }
   }
   return Error::success();
 #undef GET_OR_DISTINCT

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

void ModuleBitcodeWriter::writeDIMacroFile(const DIMacroFile *N,
   Record.clear();
 }
 
 void ModuleBitcodeWriter::writeDIArgList(const DIArgList *N,
                                          SmallVectorImpl<uint64_t> &Record,
                                          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();
 }
 
 void ModuleBitcodeWriter::writeDIModule(const DIModule *N,
                                         SmallVectorImpl<uint64_t> &Record,
                                         unsigned Abbrev) {

llvm/lib/Bitcode/Writer/ValueEnumerator.h

 #include <cstdint>
 #include <utility>
 #include <vector>
 
 namespace llvm {
 
 class BasicBlock;
 class Comdat;
+class DIArgList;
 class Function;
 class Instruction;
 class LocalAsMetadata;
 class MDNode;
 class Metadata;
 class Module;
 class NamedMDNode;
 class raw_ostream;
private:
   ///{
   void EnumerateMetadata(const Function *F, const Metadata *MD);
   void EnumerateMetadata(unsigned F, const Metadata *MD);
   ///}
 
   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);
   void EnumerateOperandType(const Value *V);
   void EnumerateAttributes(AttributeList PAL);
 
   void EnumerateValueSymbolTable(const ValueSymbolTable &ST);
   void EnumerateNamedMetadata(const Module &M);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_BITCODE_WRITER_VALUEENUMERATOR_H

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

struct OrderMap {
   void index(const Value *V) {
     // Explicitly sequence get-size and insert-value operations to avoid UB.
     unsigned ID = IDs.size() + 1;
     IDs[V].first = ID;
   }
 };
 
 } // 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;
 
   if (const Constant *C = dyn_cast<Constant>(V)) {
     if (C->getNumOperands() && !isa<GlobalValue>(C)) {
       for (const Value *Op : C->operands())
         if (!isa<BasicBlock>(Op) && !isa<GlobalValue>(Op))
           orderValue(Op, OM);
for (const Use &U : F.operands())
         orderValue(U.get(), OM);
   }
 
   // As constants used in metadata operands are emitted as module-level
   // constants, we must order them before other operands. Also, we must order
   // 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());
+            }
           }
         }
   }
   OM.LastGlobalConstantID = OM.size();
 
   // Initializers of GlobalValues are processed in
   // BitcodeReader::ResolveGlobalAndAliasInits().  Match the order there rather
   // than ValueEnumerator, and match the code in predictValueUseListOrderImpl()
for (const BasicBlock &BB : F)
       for (const Instruction &I : BB) {
         for (const Use &Op : I.operands()) {
           auto *MD = dyn_cast<MetadataAsValue>(&Op);
           if (!MD) {
             EnumerateOperandType(Op);
             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());
         }
         if (auto *SVI = dyn_cast<ShuffleVectorInst>(&I))
           EnumerateType(SVI->getShuffleMaskForBitcode()->getType());
         EnumerateType(I.getType());
         if (const auto *Call = dyn_cast<CallBase>(&I))
           EnumerateAttributes(Call->getAttributes());
void ValueEnumerator::EnumerateMetadata(const Function *F, const Metadata *MD) {
   EnumerateMetadata(getMetadataFunctionID(F), MD);
 }
 
 void ValueEnumerator::EnumerateFunctionLocalMetadata(
     const Function &F, const LocalAsMetadata *Local) {
   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;
   auto push = [&Worklist](MetadataMapType::value_type &MD) {
     auto &Entry = MD.second;
 
     // Nothing to do if this metadata isn't tagged.
     if (!Entry.F)
const MDNode *ValueEnumerator::enumerateMetadataImpl(unsigned F, const Metadata *MD) {
 
   // Enumerate the constant, if any.
   if (auto *C = dyn_cast<ConstantAsMetadata>(MD))
     EnumerateValue(C->getValue());
 
   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) {
   assert(F && "Expected a function");
 
   // Check to see if it's already in!
   MDIndex &Index = MetadataMap[Local];
   if (Index.ID) {
     assert(Index.F == F && "Expected the same function");
     return;
   }
 
   MDs.push_back(Local);
   Index.F = F;
   Index.ID = MDs.size();
 
   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))
     return 0;
 
   // ConstantAsMetadata doesn't reference anything.  We may as well shuffle it
   // to the front since we can detect it.
   auto *N = dyn_cast<MDNode>(MD);
for (unsigned i = 0, e = FnLocalMDVector.size(); i != e; ++i) {
     // have a metadata operand that references a value that hasn't been seen.
     assert(ValueMap.count(FnLocalMDVector[i]->getValue()) &&
            "Missing value for metadata operand");
     EnumerateFunctionLocalMetadata(F, FnLocalMDVector[i]);
   }
   // 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() {
   /// Remove purged values from the ValueMap.
   for (unsigned i = NumModuleValues, e = Values.size(); i != e; ++i)
     ValueMap.erase(Values[i].first);
   for (unsigned i = NumModuleMDs, e = MDs.size(); i != e; ++i)
     MetadataMap.erase(MDs[i]);

llvm/test/DebugInfo/Generic/debug_value_list.ll

 ; RUN: opt -verify < %s | opt -verify -S | FileCheck %s
 
 ; Simple IR-BC-IR round-trip test for a @llvm.dbg.value that uses !DIArgList
 ; and DW_OP_LLVM_arg.
 
 source_filename = ".\\debug_value_list.cpp"
 target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 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
 }
 
 declare void @llvm.dbg.value(metadata, metadata, metadata)
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!3, !4, !5, !6}