[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 a comment.

See inline notes.
Also, the edge case with `CurCodeSize` == 32, if that is possible, should be added.



================
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();
----------------
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");
```


================
Comment at: unittests/Bitcode/BitstreamWriterTest.cpp:65
+  auto Abbrev = std::make_shared<BitCodeAbbrev>();
+  Abbrev->Add(BitCodeAbbrevOp(42U));
+  unsigned AbbrevCode = W.EmitAbbrev(std::move(Abbrev));
----------------
I'd expect to see three tests.
One last valid one, the invalid one, and the next after the first invalid one.
So `3U`, `4U`, `5U`.


Repository:
  rL LLVM

https://reviews.llvm.org/D38833