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

[Jordan Rose via Phabricator via llvm-commits]

jordan_rose added inline comments.


================
Comment at: unittests/Bitcode/BitstreamWriterTest.cpp:75-182
+TEST(BitstreamWriterTest, trapOnSmallAbbrev_CodeLength2) {
+  SmallString<64> Buffer;
+  BitstreamWriter W(Buffer);
+  W.EnterSubblock(bitc::FIRST_APPLICATION_BLOCKID, /*CodeLen*/2);
+  auto Abbrev = std::make_shared<BitCodeAbbrev>();
+  Abbrev->Add(BitCodeAbbrevOp(42U));
+  unsigned AbbrevCode = W.EmitAbbrev(std::move(Abbrev));
----------------
lebedev.ri wrote:
> This looks rather monstrous, with a lot of duplication.
> I was suggesting to just do:
> ```
> TEST(BitstreamWriterTest, trapOnSmallAbbrevUsingBlockInfo_CodeLength3_Ok) {
>   SmallString<64> Buffer;
>   BitstreamWriter W(Buffer);
>   W.EnterBlockInfoBlock();
>   auto Abbrev = std::make_shared<BitCodeAbbrev>();
>   Abbrev->Add(BitCodeAbbrevOp(2U);
>   (void)W.EmitBlockInfoAbbrev(bitc::FIRST_APPLICATION_BLOCKID,
>                               std::move(Abbrev));
>   W.ExitBlock();
>   W.EnterSubblock(bitc::FIRST_APPLICATION_BLOCKID, /*CodeLen*/3);
>   W.EmitRecordWithAbbrev(Abbrev, makeArrayRef(2U));
>   W.ExitBlock();
> }
> 
> TEST(BitstreamWriterTest, trapOnSmallAbbrevUsingBlockInfo_CodeLength3_LastOk) {
>   SmallString<64> Buffer;
>   BitstreamWriter W(Buffer);
>   W.EnterBlockInfoBlock();
>   auto Abbrev = std::make_shared<BitCodeAbbrev>();
>   Abbrev->Add(BitCodeAbbrevOp(3U);
>   (void)W.EmitBlockInfoAbbrev(bitc::FIRST_APPLICATION_BLOCKID,
>                               std::move(Abbrev));
>   W.ExitBlock();
>   W.EnterSubblock(bitc::FIRST_APPLICATION_BLOCKID, /*CodeLen*/3);
>   W.EmitRecordWithAbbrev(Abbrev, makeArrayRef(3U));
>   W.ExitBlock();
> }
> 
> TEST(BitstreamWriterTest, trapOnSmallAbbrevUsingBlockInfo_CodeLength3_NotOk) {
>   SmallString<64> Buffer;
>   BitstreamWriter W(Buffer);
>   W.EnterBlockInfoBlock();
>   auto Abbrev = std::make_shared<BitCodeAbbrev>();
>   Abbrev->Add(BitCodeAbbrevOp(4U);
>   (void)W.EmitBlockInfoAbbrev(bitc::FIRST_APPLICATION_BLOCKID,
>                               std::move(Abbrev));
>   W.ExitBlock();
>   W.EnterSubblock(bitc::FIRST_APPLICATION_BLOCKID, /*CodeLen*/3);
>   W.EmitRecordWithAbbrev(Abbrev, makeArrayRef(4U));
>   W.ExitBlock();
> 
> TEST(BitstreamWriterTest, trapOnSmallAbbrevUsingBlockInfo_CodeLength3_NoOverFlow) {
>   SmallString<64> Buffer;
>   BitstreamWriter W(Buffer);
>   W.EnterBlockInfoBlock();
>   auto Abbrev = std::make_shared<BitCodeAbbrev>();
>   Abbrev->Add(BitCodeAbbrevOp(4294967295U); // i32 -1 
>   (void)W.EmitBlockInfoAbbrev(bitc::FIRST_APPLICATION_BLOCKID,
>                               std::move(Abbrev));
>   W.ExitBlock();
>   W.EnterSubblock(bitc::FIRST_APPLICATION_BLOCKID, /*CodeLen*/32);
>   W.EmitRecordWithAbbrev(Abbrev, makeArrayRef(4294967295U));
>   W.ExitBlock();
> }
> ```
The values in the abbreviation have nothing to do with the abbreviation codes themselves; in order to get to code 8 you have to actually register 5 abbreviations. (0-3 are reserved.) I could probably pull out some of this into helper functions but I didn't feel like it was going to be super important.


Repository:
  rL LLVM

https://reviews.llvm.org/D38833