[PATCH] D26719: Bitcode: Introduce initial multi-module reader API.

[Peter Collingbourne via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/include/llvm/Bitcode/BitcodeReader.h:48
+    uint64_t IdentificationBit;
+    uint64_t ModuleBit;
+
----------------
mehdi_amini wrote:
> Can you document these two fields?
Will do


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:850
+      ValueList(Context), MetadataList(Context) {
+  this->ProducerIdentification = ProducerIdentification;
+}
----------------
mehdi_amini wrote:
> Why not in the initializer list?
This field belongs to BitcodeReaderBase.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:6607
+    ProducerIdentification = *ProducerIdentificationOrErr;
+  }
+
----------------
mehdi_amini wrote:
> I think it is used only on error, isn't it?
> 
> We may be able to pass the `IdentificationBit` to the BitcodeReader and have it jump there to get the `ProducerIdentification` only if needed.
> 
> Might not worth the effort though.
We also need to check the epoch in the identification block.

I suppose we could have two functions, one that checks the epoch and the other that pulls the producer string, but again it doesn't seem worth the effort.


================
Comment at: llvm/test/Bitcode/invalid.test:38
 BAD-TYPE-TABLE-FORWARD-REF: Invalid TYPE table: Only named structs can be forward referenced
-BAD-BITWIDTH: Bitwidth for integer type out of range
+BAD-BITWIDTH: Malformed block
 BAD-ALIGN: Invalid alignment value
----------------
mehdi_amini wrote:
> I'm surprised by this change, is it really intended? I suspect that some of your change is losing coverage here.
> 
> (Same on most of the changes below)
Unfortunately some of our invalid bitcode files also have an invalid block size in addition to the problem under test, so we get an error trying to skip past the module in getBitcodeModuleList().

I'll see if I can restore coverage for these, but it may be some work as it will require binary patching of our test files.


https://reviews.llvm.org/D26719