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

[Duncan P. N. Exon Smith via llvm-commits]

> On 2015-Oct-14, at 13:49, Chandler Carruth <chandlerc at gmail.com> wrote:
> 
> 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. =]

Bitcode side LGTM.

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