[PATCH] D28083: Add an index for Module Metadata record in the bitcode

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

In https://reviews.llvm.org/D28083#631731, @tejohnson wrote:

> I guess you don't have any BitcodeReader changes since the unknown records are simply ignored?


Yes!

> I think it would be useful to add a BitcodeReader change though to parse the index offset record and sanity check against the offset of the index record when it is later encountered.

Good idea, will do!



================
Comment at: llvm/include/llvm/Bitcode/BitstreamWriter.h:282
       if (Op.getEncodingData())
-        Emit((unsigned)V, (unsigned)Op.getEncodingData());
+        Emit64((unsigned)V, (unsigned)Op.getEncodingData());
       break;
----------------
tejohnson wrote:
> tejohnson wrote:
> > mehdi_amini wrote:
> > > tejohnson wrote:
> > > > This change seems unnecessary
> > > The offset that we back patch is emitted as a 64 bits fixed size, which wasn't possible without this change.
> > But you are casting V to unsigned aka unsigned int, which is 32 bits. It seems like you don't want to cast V at all if you change to Emit64, since Emit64 takes a uint64_t, and in order to pass in a 64-bit val the uintty of V should already be uint64_t. This is similar to how V is not cast when passed to EmitVBR64 below (which takes a uint64_t value).
> > 
> > Actually - I think you are getting lucky here because BackpatchWord also takes a 32-bit unsigned value. So you are never currently writing a 64-bit value for the offset.
> > 
> > For the VST offset we simply emit the number of 32-bit words since blocks are guaranteed to be 32-bit aligned. I guess you can't do that here since the index is not at the start of the block. Alternatively, emit into its own block, then you can emit the word offset.
> > Alternatively, emit into its own block, then you can emit the word offset.
> 
> Thinking more about this, it should be lower overhead to emit in the original block and just emit properly as a 64-bit value. I think that is just invoking Emit64 without the cast on V, and adding a new backpatch interface that takes a 64-bit unsigned and splits into two halves as Emit64 does in the >32 bit case.
> Thinking more about this, it should be lower overhead to emit in the original block and just emit properly as a 64-bit value. I think that is just invoking Emit64 without the cast on V, and adding a new backpatch interface that takes a 64-bit unsigned and splits into two halves as Emit64 does in the >32 bit case.

Yes will do. Thanks for catching, it solved the assertion but would still have been broken on large files...






https://reviews.llvm.org/D28083