[PATCH] D86957: [Bitstream] Use alignTo to make code more readable. NFC

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

In D86957#2250245 <https://reviews.llvm.org/D86957#2250245>, @stephan.yichao.zhao wrote:

> In D86957#2250225 <https://reviews.llvm.org/D86957#2250225>, @craig.topper wrote:
>
>> In D86957#2250193 <https://reviews.llvm.org/D86957#2250193>, @stephan.yichao.zhao wrote:
>>
>>> So this does both 64bit cast and alignment. Thank you.
>>>
>>> Hi Craig, in your case, will NumElts be actually larger than 2^32? NumElts is read from https://llvm.org/docs/BitCodeFormat.html#enter-subblock-encoding that defines the blocklen to be 32bit. So when it gets larger, the overflow can happen at the writer side (https://llvm.org/doxygen/BitstreamWriter_8h_source.html#l00384).
>>
>> My specific case was a blob for metadata strings that was ~1GB in size. The multipy by 8 to convert its size to bits was overflowing. I do worry that it might break again if the blob of metadata strings exceed 4GB.
>
> The case I fixed is similar. One way to address is to extend that blocklen field to 64bit. imo this does not introduce any back-compatibility issue because 32 is not a fixed width, but VBR.
>
> 1. when an old reader reads a bitcode written by a new writer, it works if blocklen is <= 2^32. Although it gets broken if blocklen is > 2^32, this case it does not work anyway.
> 2. when a new reader reads a bitcode written by an old writer, it works fine since blocklen is <= 2^32.
>
> So it is possible to extend it to 64bit.

The blocklen field in ENTER_SUBBLOCK isn't a VBR from what I could see. Its just a 32 bit value allowing a maximum block size of 16GB. There is a VBR6 to store the size of the blob. That one we could change to use uint64_t to allow blobs larger than 4GB, but we'd still be limited by the 16GB limit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86957/new/

https://reviews.llvm.org/D86957