[PATCH] D38833: BitstreamWriter: Better assertion when a block's abbrev code size is too small

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: include/llvm/Bitcode/BitstreamWriter.h:303
     assert(AbbrevNo < CurAbbrevs.size() && "Invalid abbrev #!");
+    assert(Abbrev < (1U << CurCodeSize) && "Block code length is too small");
     const BitCodeAbbrev *Abbv = CurAbbrevs[AbbrevNo].get();
----------------
jordan_rose wrote:
> lebedev.ri wrote:
> > jordan_rose wrote:
> > > lebedev.ri wrote:
> > > > What is the expected range of `CurCodeSize` values?
> > > > This has UB if `CurCodeSize` == `32`.
> > > > It would be much safer to just use the variant with subtraction:
> > > > ```
> > > > assert(Abbrev <= (~(~0U >> (32-CurCodeSize))) && "Block code length is too small");
> > > > ```
> > > Good point. I don't think anyone's trying to use 32-bit record codes, but the [spec](http://llvm.org/docs/BitCodeFormat.html) doesn't seem to impose a limit. I'll switch it.
> > I think you want to move this assert to before the first use of `Abbrev` (before line 301)
> It shouldn't matter in this case. The first use (and the above assertion) is for indexing into a write-time data structure; the second is for the actual representation in the bitstream.
Point being, if you don't move it, you won't be able to add a test for 32-bit `CurCodeSize` overflow.


Repository:
  rL LLVM

https://reviews.llvm.org/D38833