[PATCH] D13666: Add an (optional) identification block in the bitcode

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

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. =]


================
Comment at: include/llvm/Bitcode/LLVMBitCodes.h:37-39
@@ -36,3 +36,5 @@
 
-    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,
 
----------------
Stale comment?

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

================
Comment at: include/llvm/Bitcode/LLVMBitCodes.h:55-60
@@ -52,1 +54,8 @@
 
+  // 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#]
+  };
+
----------------
Is there a better name here than "identification"? I'm not coming up with one... Any ideas Duncan?

================
Comment at: include/llvm/Bitcode/LLVMBitCodes.h:62-67
@@ -53,1 +61,8 @@
+
+// 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
 
----------------
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?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:278-280
@@ -275,1 +277,5 @@
 private:
+  /// Parse the "IDENTIFICATION_BLOCK_ID" block and populate the
+  /// ProducerIdentification data member.
+  std::error_code parseBitcodeVersion();
+
----------------
It also does basic Epoch enforcement...

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3115
@@ +3114,3 @@
+        if (epoch != BITCODE_CURRENT_EPOCH) {
+          return error("Incompatible epoch");
+        }
----------------
Can we provide a bit more detail here, much like you do above?


http://reviews.llvm.org/D13666