Implement a safer bitcode upgrade for DISubprogram

[aprantl]

Summary

The bitcode upgrade I added for DISubprogram in r266446 was based on the assumption that the CU node for the subprogram was already materialized by the time the DISubprogram is visited. This assumption may not hold true as future versions of LLVM may decide to write out bitcode in a different order. This patch corrects this by introducing a versioning bit next to the distinct flag to unambiguously differentiate the new from the old record layouts.

Note for people stabilizing LLVM out-of-tree: This patch introduces a bitcode incompatibility with llvm trunk revisions from r266446 — here. (But D19987 will ensure that it degrades gracefully).

[aprantl]

Renamed IsVersion3 to HasUnit. This is easier to read regardless.

[aprantl]

I get it. Independent flags don't really make sense in this context, and with the >2 check we can later add further bits *to the version number*.

[dexonsmith]

Sorry for the delay answering your questions above, but it looks like we reached consensus anyway :).

LGTM with one fixup.

[mehdi_amini]

Was this committed? (If yes please indicate revision and close)

[aprantl]

This was r268816.

lib/Bitcode/Reader/BitcodeReader.cpp

case bitc::METADATA_COMPILE_UNIT: {
         CUSubprograms.push_back({CU, SPs});
       break;
     }
     case bitc::METADATA_SUBPROGRAM: {
       if (Record.size() != 18 && Record.size() != 19)
         return error("Invalid record");
 
       IsDistinct =
-          Record[0] || Record[8]; // All definitions should be distinct.
+          (Record[0] & 1) || Record[8]; // All definitions should be distinct.
+      bool HasUnit = (Record[0] & (1 << 1)) == (1 << 1);
+      if (HasUnit && Record.size() != 19)
+        return error("Invalid record");
       // Version 1 has a Function as Record[15].
       // Version 2 has removed Record[15].
       // Version 3 has the Unit as Record[15].
       Metadata *CUorFn = getMDOrNull(Record[15]);
       unsigned Offset = Record.size() == 19 ? 1 : 0;
-      bool HasFn = Offset && dyn_cast_or_null<ConstantAsMetadata>(CUorFn);
-      bool HasCU = Offset && !HasFn;
+      bool HasFn = Offset && !HasUnit;
       DISubprogram *SP = GET_OR_DISTINCT(
           DISubprogram,
           (Context, getDITypeRefOrNull(Record[1]), getMDString(Record[2]),
            getMDString(Record[3]), getMDOrNull(Record[4]), Record[5],
            getMDOrNull(Record[6]), Record[7], Record[8], Record[9],
            getDITypeRefOrNull(Record[10]), Record[11], Record[12], Record[13],
-           Record[14], HasCU ? CUorFn : nullptr,
+           Record[14], HasUnit ? CUorFn : nullptr,
            getMDOrNull(Record[15 + Offset]), getMDOrNull(Record[16 + Offset]),
            getMDOrNull(Record[17 + Offset])));
       MetadataList.assignValue(SP, NextMetadataNo++);
 
       // Upgrade sp->function mapping to function->sp mapping.
       if (HasFn) {
-        if (auto *CMD = dyn_cast<ConstantAsMetadata>(CUorFn))
+        if (auto *CMD = dyn_cast_or_null<ConstantAsMetadata>(CUorFn))
           if (auto *F = dyn_cast<Function>(CMD->getValue())) {
             if (F->isMaterializable())
               // Defer until materialized; unmaterialized functions may not have
               // metadata.
               FunctionsWithSPs[F] = SP;
             else if (!F->empty())
               F->setSubprogram(SP);
           }

lib/Bitcode/Writer/BitcodeWriter.cpp

void ModuleBitcodeWriter::writeDICompileUnit(const DICompileUnit *N,
 
   Stream.EmitRecord(bitc::METADATA_COMPILE_UNIT, Record, Abbrev);
   Record.clear();
 }
 
 void ModuleBitcodeWriter::writeDISubprogram(const DISubprogram *N,
                                             SmallVectorImpl<uint64_t> &Record,
                                             unsigned Abbrev) {
-  Record.push_back(N->isDistinct());
+  uint64_t HasUnitFlag = 1 << 1;
+  Record.push_back(N->isDistinct() | HasUnitFlag);
   Record.push_back(VE.getMetadataOrNullID(N->getScope()));
   Record.push_back(VE.getMetadataOrNullID(N->getRawName()));
   Record.push_back(VE.getMetadataOrNullID(N->getRawLinkageName()));
   Record.push_back(VE.getMetadataOrNullID(N->getFile()));
   Record.push_back(N->getLine());
   Record.push_back(VE.getMetadataOrNullID(N->getType()));
   Record.push_back(N->isLocalToUnit());
   Record.push_back(N->isDefinition());