[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

88–89

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

88–89

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

 
 /// 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())
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 *Value : ArgList->getArgs()) {
+    if (isa<LocalAsMetadata>(Value)) {
+      assert(MetadataMap.count(Value) &&
+             "LocalAsMetadata should be enumerated before DIArgList");
+      assert(MetadataMap[Value].F == F &&
+             "Expected LocalAsMetadata in the same function");
+    } else {
+      assert(isa<ConstantAsMetadata>(Value) &&
+             "Expected LocalAsMetadata or ConstantAsMetadata");
+      EnumerateMetadata(F, Value);
+    }
+  }
+
+  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]);