Add an (optional) identification block in the bitcode

[mehdi_amini]

Summary

Processing bitcode from a different LLVM version can lead to unexpected behavior. The LLVM project guarantees autoupdating bitcode from a previous minor revision for the same major, but can't make any promise when reading bitcode generated from a either a non-released LLVM, a vendor toolchain, or a "future" LLVM release.
This patch aims at being more user-friendly and allows a bitcode produce to emit an optional block at the beginning of the bitcode that will contains an opaque string intended to describe the bitcode producer information.
The bitcode reader will dump this information alongside any error it reports.

[mehdi_amini]

Display the current version along with the producer version on error.

[chandlerc]

I wonder if this should have two components, one for the epoch and one for the string? I'm worried about the fact that you're using the only "unused" ID here, and it does seem nice to not use two IDs for this kind of data.

[vsk]

Is it possible to add a test for this, perhaps by forcing bad bitcode into llvm-dis?

$ llvm-as -disable-verify -o - | llvm-dis
define void @f() {
  %1 = alloca i32
  %1 = %1
  ret void
}
llvm-as: <stdin>:3:8: error: expected instruction opcode

Seems like we could have a check-line here for the message BitcodeReader::error emits.

[vsk]

Whoops, I think that amounts to just "echo '' | llvm-dis".

[mehdi_amini]

The problem is to be able to generate some invalid bitcode that *still contains the identification block*.

I tried with some valid bitcode: echo "randomstring" >> /tmp/bitcode.bc ;
But it is not enough to trigger an error with: llvm-dis /tmp/bitcode.bc

—
Mehdi

[mehdi_amini]

Add the "epoch" field along with the producer string.

[chandlerc]

Most of my comments are pretty high-level. I'd imagine Duncan is in a much better position to check the actual bitcode side of it. =]

include/llvm/Bitcode/LLVMBitCodes.h

37–39

Stale comment?

Also "Can be used to provide better error messages when we fail to parse a bitcode file." maybe?

55–60

Is there a better name here than "identification"? I'm not coming up with one... Any ideas Duncan?

62–67

I would start at 0 and define missing == 0. I would also document that property.

I think we should also spell out in a bit more detail that this is rev-ed with the major version of LLVM.

Nits:
Maybe "... which also accepts N-1."?
Use an anonymous enum rather than a macro?

lib/Bitcode/Reader/BitcodeReader.cpp

278–280

It also does basic Epoch enforcement...

3115

Can we provide a bit more detail here, much like you do above?

[mehdi_amini]

Update taking Chandler comments into account.

[mehdi_amini]

Change the epoch to encode major.minor.patch. The basic compatibility check becomes (for now):

!((Major < LLVM_VERSION_MAJOR && LLVM_VERSION_MINOR != 0) || Major > LLVM_VERSION_MAJOR || (Major == LLVM_VERSION_MAJOR && Minor > LLVM_VERSION_MAJOR))

[jroelofs]

lib/Bitcode/Reader/BitcodeReader.cpp

3120

Minor > LLVM_VERSION_MAJOR

should be:

Minor > LLVM_VERSION_MINOR

[mehdi_amini]

Fix bug spotted by Jonathan Roelofs

[jroelofs]

lib/Bitcode/Reader/BitcodeReader.cpp

3120

In irc we talked about how to test this... I'll capture those thoughts here:

9:06:40 PM - jokereph_: jroelofs: thks for spotting it
9:08:57 PM - jroelofs: jokereph_: testcases on that particular behavior is probably a good idea (regardless of which epoch kind you guys end up picking)
9:09:31 PM - jokereph_: jroelofs: any idea on how to write it?
9:10:16 PM - jokereph_: I guess I could write a c++ test for that, but it’s not easy to write a generic bitcode test since we want to test something that affects version X.0 only for instance
9:11:19 PM - jroelofs: what if you pull out the "here's what version I am" part of it, and feed that in to the check
9:12:33 PM - jroelofs: then the unitests test could poke in whatever values it wants for that, and have the check verify against poked-in values in the bitcode itself
9:12:41 PM - jokereph_: jroelofs, and writing a C++ unittests? Not a bitcode reader test right? I requires exposing this function though.
9:13:17 PM * jroelofs honestly isn't all that familiar with how the bitcode stuff works/is tested
9:13:30 PM - jokereph_: jroelofs: we only test coarse grain AFAIK
9:13:45 PM - jokereph_: i.e. llvm-dis for instance
9:14:09 PM - jokereph_: I may be able also to write a C++ unittest that forge a bitcode with a specific version and see what the reader says.
9:14:25 PM - jokereph_: In the end it seems quite complicated for what it is :(
9:14:47 PM - jroelofs: maybe split parseBitcodeVersion into a parser and a verifier
9:14:48 PM - jokereph_: I just tested the “major” manually

10:04:06 PM - jroelofs: jokereph: after thinking about it some more, I think the right thing to do testing-wise is to save off a bitcode file from each time the epoch changes, and then have a lit test that tries to load up each of these files. then check for the expected error message for each file
10:05:32 PM - jroelofs: jokereph: that way if the epoch format gets changed down the line, you've got testcases for all the old formats, and can check that the error code does the right thing for all of them

[ovyalov]

lib/Bitcode/Reader/BitcodeReader.cpp

3121

Android builder is failing because of unsupported std::to_string - http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-buildserver/builds/2835/steps/build%20android/logs/stdio
Is it possible to replace this call here with std::ostringstream ?

[mehdi_amini]

lib/Bitcode/Reader/BitcodeReader.cpp

3121

Sure, but do we know what makes it missing? The log says gcc 4.9.0, what is the C++ library used?
It seems we need to add some documentation here: http://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library
(unless I missed something there)

[jroelofs]

lib/Bitcode/Reader/BitcodeReader.cpp

3121

Twine(Major) would work here.

[ovyalov]

lib/Bitcode/Reader/BitcodeReader.cpp

3121

Android NDK uses own version of STL that has some missed pieces - for example, in LLDB we emulate to_string in such way:

https://github.com/llvm-mirror/lldb/blob/ce972b9acd5e10f709b1733c73319839a6a997b5/include/lldb/Host/android/Android.h

[mehdi_amini]

lib/Bitcode/Reader/BitcodeReader.cpp

3121

OK I didn't know about the custom STL (I knew that it used a custom libc).
Two actions seems appropriate to me here:

• submit a patch here: http://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library to require support for this STL.
• lldb to_string emulation could be moved in llvm itself. (There is already another place in llvm where to_string is used).

jroelofs: thanks for the suggestion, I didn't know Twine can do the to_string conversion implicitly. (and thanks ovyalov to have already implemented it).

[aizatsky]

lib/Bitcode/Writer/BitcodeWriter.cpp

2848

The revision that was actually submitted is different at least in this location:

https://github.com/llvm-mirror/llvm/commit/b738d340fa9958dbefe8d23a4d4659c2255b823a

The submitted version has a bug and a resulting memory leak.

[mehdi_amini]

I'll look into it today, thanks for the report.

[mehdi_amini]

lib/Bitcode/Writer/BitcodeWriter.cpp

2848

Fixed in r252110.

The committed diff was significantly different because this last diff was a proposal for another scheme for the versioning (Major/Minor/Patch instead of Epoch), the committed diff is based on the previous diff for this revision.

include/llvm/Bitcode/LLVMBitCodes.h

enum BlockIDs {
 
     // Module sub-block id's.
     PARAMATTR_BLOCK_ID,
     PARAMATTR_GROUP_BLOCK_ID,
 
     CONSTANTS_BLOCK_ID,
     FUNCTION_BLOCK_ID,
 
-    UNUSED_ID1,
+    // Block intended to contains a single string identifying the bitcode
+    // producer. Can be used to debug error while parsing the bitcode.
+    IDENTIFICATION_BLOCK_ID,
 
     VALUE_SYMTAB_BLOCK_ID,
     METADATA_BLOCK_ID,
     METADATA_ATTACHMENT_ID,
 
     TYPE_BLOCK_ID_NEW,
 
     USELIST_BLOCK_ID,
 
     MODULE_STRTAB_BLOCK_ID,
     FUNCTION_SUMMARY_BLOCK_ID,
 
     OPERAND_BUNDLE_TAGS_BLOCK_ID
   };
 
+  // Idenfitication block contains a string that describes the producer details,
+  // and an epoch that defines the auto-upgrade capability.
+  enum IdentificationCodes {
+    IDENTIFICATION_CODE_STRING = 1,    // IDENTIFICATION:      [strchr x N]
+    IDENTIFICATION_CODE_EPOCH  = 2,    // EPOCH:               [epoch#]
+  };
+
+// The epoch that defines the auto-upgrade compatibility for the bitcode.
+//
+// The reader accepts only bitcode with the same epoch, except for the X.0
+// release which accepts as well N-1.
+// FIXME: where to put this define?
+#define BITCODE_CURRENT_EPOCH 1
 
   /// MODULE blocks have a number of optional fields and subblocks.
   enum ModuleCodes {
     MODULE_CODE_VERSION     = 1,    // VERSION:     [version#]
     MODULE_CODE_TRIPLE      = 2,    // TRIPLE:      [strchr x N]
     MODULE_CODE_DATALAYOUT  = 3,    // DATALAYOUT:  [strchr x N]
     MODULE_CODE_ASM         = 4,    // ASM:         [strchr x N]
     MODULE_CODE_SECTIONNAME = 5,    // SECTIONNAME: [strchr x N]

lib/Bitcode/Reader/BitcodeReader.cpp

class BitcodeReader : public GVMaterializer {
   std::unique_ptr<BitstreamReader> StreamFile;
   BitstreamCursor Stream;
   // Next offset to start scanning for lazy parsing of function bodies.
   uint64_t NextUnreadBit = 0;
   // Last function offset found in the VST.
   uint64_t LastFunctionBlockBit = 0;
   bool SeenValueSymbolTable = false;
   unsigned VSTOffset = 0;
+  // Contains an arbitrary and optional string identifying the bitcode producer
+  std::string ProducerIdentification;
 
   std::vector<Type*> TypeList;
   BitcodeReaderValueList ValueList;
   BitcodeReaderMDValueList MDValueList;
   std::vector<Comdat *> ComdatList;
   SmallVector<Instruction *, 64> InstructionList;
 
   std::vector<std::pair<GlobalVariable*, unsigned> > GlobalInits;
public:
   static uint64_t decodeSignRotatedValue(uint64_t V);
 
   /// Materialize any deferred Metadata block.
   std::error_code materializeMetadata() override;
 
   void setStripDebugInfo() override;
 
 private:
+  /// Parse the "IDENTIFICATION_BLOCK_ID" block and populate the
+  /// ProducerIdentification data member.
+  std::error_code parseBitcodeVersion();
+
   std::vector<StructType *> IdentifiedStructTypes;
   StructType *createIdentifiedStructType(LLVMContext &Context, StringRef Name);
   StructType *createIdentifiedStructType(LLVMContext &Context);
 
   Type *getTypeByID(unsigned ID);
   Value *getFnValueByID(unsigned ID, Type *Ty,
                         OperatorConstraint OC = OC_None) {
     if (Ty && Ty->isMetadataTy())

 
 static std::error_code error(DiagnosticHandlerFunction DiagnosticHandler,
                              const Twine &Message) {
   return error(DiagnosticHandler,
                make_error_code(BitcodeError::CorruptedBitcode), Message);
 }
 
 std::error_code BitcodeReader::error(BitcodeError E, const Twine &Message) {
+  if (!ProducerIdentification.empty()) {
+    Twine MsgWithID = Message + " (Producer: '" + ProducerIdentification
+        + "' Reader: 'LLVM " + LLVM_VERSION_STRING "')";
+    return ::error(DiagnosticHandler, make_error_code(E), MsgWithID);
+  }
   return ::error(DiagnosticHandler, make_error_code(E), Message);
 }
 
 std::error_code BitcodeReader::error(const Twine &Message) {
+  if (!ProducerIdentification.empty()) {
+    Twine MsgWithID = Message + " (Producer: '" + ProducerIdentification
+        + "' Reader: 'LLVM " + LLVM_VERSION_STRING "')";
+    return ::error(DiagnosticHandler,
+                   make_error_code(BitcodeError::CorruptedBitcode), MsgWithID);
+  }
   return ::error(DiagnosticHandler,
                  make_error_code(BitcodeError::CorruptedBitcode), Message);
 }
 
 std::error_code BitcodeReader::error(BitcodeError E) {
   return ::error(DiagnosticHandler, make_error_code(E));
 }
 
switch (Entry.Kind) {
             if (std::error_code EC = rememberAndSkipFunctionBody()) return EC;
             NextUnreadBit = Stream.GetCurrentBitNo();
             return std::error_code();
         }
     }
   }
 }
 
+std::error_code BitcodeReader::parseBitcodeVersion() {
+  if (Stream.EnterSubBlock(bitc::IDENTIFICATION_BLOCK_ID))
+    return error("Invalid record");
+
+
+  // Read all the records.
+  SmallVector<uint64_t, 64> Record;
+  while (1) {
+    BitstreamEntry Entry = Stream.advance();
+
+    switch (Entry.Kind) {
+      default:
+      case BitstreamEntry::Error:
+        return error("Malformed block");
+      case BitstreamEntry::EndBlock:
+        return std::error_code();
+      case BitstreamEntry::Record:
+        // The interesting case.
+        break;
+    }
+
+    // Read a record.
+    Record.clear();
+    unsigned BitCode = Stream.readRecord(Entry.ID, Record);
+    switch (BitCode) {
+      default: // Default behavior: reject
+        return error("Invalid value");
+      case bitc::IDENTIFICATION_CODE_STRING: { // IDENTIFICATION:      [strchr x N]
+        convertToString(Record, 0, ProducerIdentification);
+        break;
+      }
+      case bitc::IDENTIFICATION_CODE_EPOCH: { // EPOCH:      [epoch#]
+        unsigned epoch = (unsigned)Record[0];
+        if (epoch != BITCODE_CURRENT_EPOCH) {
+          return error("Incompatible epoch");
+        }
+      }
+    }
+  }
+}
+
 std::error_code BitcodeReader::parseModule(uint64_t ResumeBit,
                                            bool ShouldLazyLoadMetadata) {
   if (ResumeBit)
     Stream.JumpToBit(ResumeBit);
   else if (Stream.EnterSubBlock(bitc::MODULE_BLOCK_ID))
     return error("Invalid record");
 
   SmallVector<uint64_t, 64> Record;
while (1) {
     }
 
     BitstreamEntry Entry =
       Stream.advance(BitstreamCursor::AF_DontAutoprocessAbbrevs);
 
     if (Entry.Kind != BitstreamEntry::SubBlock)
       return error("Malformed block");
 
+    if (Entry.ID == bitc::IDENTIFICATION_BLOCK_ID) {
+      parseBitcodeVersion();
+      continue;
+    }
+
     if (Entry.ID == bitc::MODULE_BLOCK_ID)
       return parseModule(0, ShouldLazyLoadMetadata);
 
     if (Stream.SkipBlock())
       return error("Invalid record");
   }
 }
 

lib/Bitcode/Writer/BitcodeWriter.cpp

for (auto &FI : FII.getValue()) {
       Stream.EmitRecord(bitc::FS_CODE_COMBINED_ENTRY, NameVals, FSAbbrev);
       NameVals.clear();
     }
   }
 
   Stream.ExitBlock();
 }
 
+// Create the "IDENTIFICATION_BLOCK_ID" containing a single string with the
+// current llvm version.
+static void WriteIdentificationBlock(const Module *M, BitstreamWriter &Stream) {
+  Stream.EnterSubblock(bitc::IDENTIFICATION_BLOCK_ID, 5);
+  WriteStringRecord(bitc::IDENTIFICATION_CODE_STRING, "LLVM " LLVM_VERSION_STRING, 0, Stream);
+  SmallVector<unsigned, 1> Vals = { BITCODE_CURRENT_EPOCH };
+  Stream.EmitRecord(bitc::IDENTIFICATION_CODE_EPOCH, Vals);
+  Stream.ExitBlock();
+}
+
 /// WriteModule - Emit the specified module to the bitstream.
 static void WriteModule(const Module *M, BitstreamWriter &Stream,
                         bool ShouldPreserveUseListOrder,
                         uint64_t BitcodeStartBit, bool EmitFunctionSummary) {
   Stream.EnterSubblock(bitc::MODULE_BLOCK_ID, 3);
 
   SmallVector<unsigned, 1> Vals;
   unsigned CurVersion = 1;
   Vals.push_back(CurVersion);
   Stream.EmitRecord(bitc::MODULE_CODE_VERSION, Vals);
 
   // Analyze the module, enumerating globals, functions, etc.
   ValueEnumerator VE(*M, ShouldPreserveUseListOrder);
 
// Emit the module into the buffer.
     // saved at the start for the darwin header above. The reader stream
     // will start at the bitcode, and we need the offset of the VST
     // to line up.
     uint64_t BitcodeStartBit = Stream.GetCurrentBitNo();
 
     // Emit the file header.
     WriteBitcodeHeader(Stream);
 
+    WriteIdentificationBlock(M, Stream);
+
     // Emit the module.
     WriteModule(M, Stream, ShouldPreserveUseListOrder, BitcodeStartBit,
                 EmitFunctionSummary);
   }
 
   if (TT.isOSDarwin())
     EmitDarwinBCHeaderAndTrailer(Buffer, TT);
 

test/Bitcode/identification.ll

+; Check that a block "IDENTIFICATION_BLOCK_ID" is emitted.
+;RUN: llvm-as < %s | llvm-bcanalyzer -dump | FileCheck %s
+;CHECK: <IDENTIFICATION_BLOCK_ID
+;CHECK-NEXT: <STRING
+;CHECK-NEXT: <EPOCH
+;CHECK-NEXT: </IDENTIFICATION_BLOCK_ID

tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp

static const char *GetBlockName(unsigned BlockID,
   switch (BlockID) {
   default:                             return nullptr;
   case bitc::MODULE_BLOCK_ID:          return "MODULE_BLOCK";
   case bitc::PARAMATTR_BLOCK_ID:       return "PARAMATTR_BLOCK";
   case bitc::PARAMATTR_GROUP_BLOCK_ID: return "PARAMATTR_GROUP_BLOCK_ID";
   case bitc::TYPE_BLOCK_ID_NEW:        return "TYPE_BLOCK_ID";
   case bitc::CONSTANTS_BLOCK_ID:       return "CONSTANTS_BLOCK";
   case bitc::FUNCTION_BLOCK_ID:        return "FUNCTION_BLOCK";
+  case bitc::IDENTIFICATION_BLOCK_ID:
+    return "IDENTIFICATION_BLOCK_ID";
   case bitc::VALUE_SYMTAB_BLOCK_ID:    return "VALUE_SYMTAB";
   case bitc::METADATA_BLOCK_ID:        return "METADATA_BLOCK";
   case bitc::METADATA_ATTACHMENT_ID:   return "METADATA_ATTACHMENT_BLOCK";
   case bitc::USELIST_BLOCK_ID:         return "USELIST_BLOCK_ID";
   case bitc::FUNCTION_SUMMARY_BLOCK_ID:
                                        return "FUNCTION_SUMMARY_BLOCK";
   case bitc::MODULE_STRTAB_BLOCK_ID:   return "MODULE_STRTAB_BLOCK";
   }
default: return nullptr;
       STRINGIFY_CODE(MODULE_CODE, DEPLIB) // FIXME: Remove in 4.0
       STRINGIFY_CODE(MODULE_CODE, GLOBALVAR)
       STRINGIFY_CODE(MODULE_CODE, FUNCTION)
       STRINGIFY_CODE(MODULE_CODE, ALIAS)
       STRINGIFY_CODE(MODULE_CODE, PURGEVALS)
       STRINGIFY_CODE(MODULE_CODE, GCNAME)
       STRINGIFY_CODE(MODULE_CODE, VSTOFFSET)
     }
+  case bitc::IDENTIFICATION_BLOCK_ID:
+    switch (CodeID) {
+      default: return nullptr;
+        STRINGIFY_CODE(IDENTIFICATION_CODE, STRING)
+        STRINGIFY_CODE(IDENTIFICATION_CODE, EPOCH)
+    }
   case bitc::PARAMATTR_BLOCK_ID:
     switch (CodeID) {
     default: return nullptr;
     // FIXME: Should these be different?
     case bitc::PARAMATTR_CODE_ENTRY_OLD: return "ENTRY";
     case bitc::PARAMATTR_CODE_ENTRY:     return "ENTRY";
     case bitc::PARAMATTR_GRP_CODE_ENTRY: return "ENTRY";
     }