Fix abbreviation handling in munged bitcode writer.

unittests/Bitcode/NaClMungeWriteErrorTests.cpp

Index: unittests/Bitcode/NaClMungeWriteErrorTests.cpp
diff --git a/unittests/Bitcode/NaClMungeWriteErrorTests.cpp b/unittests/Bitcode/NaClMungeWriteErrorTests.cpp
index f48e2801dea0d5e4d24a9b9e57e12e8d72aaf3a0..80fec5a3b9edb1a41e165e910b50c3af3efe7387 100644
--- a/unittests/Bitcode/NaClMungeWriteErrorTests.cpp
+++ b/unittests/Bitcode/NaClMungeWriteErrorTests.cpp
@@ -259,6 +259,143 @@ TEST(MyNaClMungerWriteErrorTests, DieOnWriteBadAbbreviationIndex) {
       ".*");
 }
 
+// Show that we check that the abbreviation actually applies to the
+// record associated with that abbreviation. Also shows that we repair
+// the problem by applying the default abbreviation instead.o

[jvoung (off chromium), 2015/05/20 22:40:10]

instead.o --> instead.

[Karl, 2015/05/21 18:22:08]

Done.

+TEST(NaClMungeWriteErrorsTests, TestMismatchedAbbreviation) {
+  // Create edits to:
+  // 1) Expand the number of abbreviation index bits for the block from 2 to 3.
+  // 2) Introduce the incorrect abbreviation for the return instruction.
+  //    i.e. [9] instead of [10].
+  // 3) Apply the bad abbreviation to record "ret"
+  const uint64_t FunctionEnterIndex = 7;
+  const uint64_t Edits[] {
+    FunctionEnterIndex, NaClMungedBitcode::Replace,
+        1, naclbitc::BLK_CODE_ENTER, naclbitc::FUNCTION_BLOCK_ID, 3, Terminator,
+    RetVoidIndex, NaClMungedBitcode::AddBefore,

[jvoung (off chromium), 2015/05/20 22:40:10]

I guess it is kind of cool/weird that you can define an abbrev in the middle of
a block.

[Karl, 2015/05/21 18:22:08]

It is.

+        2, naclbitc::BLK_CODE_DEFINE_ABBREV, 1, 1,
+        naclbitc::FUNC_CODE_INST_RET - 1, Terminator,
+    RetVoidIndex, NaClMungedBitcode::Replace,
+        4, naclbitc::FUNC_CODE_INST_RET, Terminator
+        };
+
+  NaClWriteMunger Munger(ARRAY_TERM(BitcodeRecords));
+  Munger.munge(ARRAY(Edits));
+  EXPECT_EQ(
+      "       1: [65535, 8, 2]\n"
+      "         1: [65535, 17, 3]\n"
+      "           3: [1, 2]\n"
+      "           3: [2]\n"
+      "           3: [21, 0, 0]\n"
+      "         0: [65534]\n"
+      "         3: [8, 1, 0, 0, 0]\n"
+      "         1: [65535, 12, 3]\n"     // Upped abbreviation index bits to 3
+      "           3: [1, 1]\n"
+      "           2: [65533, 1, 1, 9]\n" // added abbrev 4: [9]
+      "           4: [10]\n"             // "ret" with bad abbreviation.
+      "         0: [65534]\n"
+      "       0: [65534]\n",
+      stringify(Munger));
+  Munger.setTryToRecoverOnWrite(true);
+  EXPECT_TRUE(Munger.runTest());

[jvoung (off chromium), 2015/05/20 22:40:10]

Add a run without recovery too?

[Karl, 2015/05/21 18:22:08]

Done.

+  EXPECT_EQ(
+      "Error (Block 12): Abbreviation doesn't apply to record: 4: [10]\n"
+      "       1: [65535, 8, 2]\n"
+      "         1: [65535, 17, 3]\n"
+      "           3: [1, 2]\n"
+      "           3: [2]\n"
+      "           3: [21, 0, 0]\n"
+      "         0: [65534]\n"
+      "         3: [8, 1, 0, 0, 0]\n"
+      "         1: [65535, 12, 3]\n"
+      "           3: [1, 1]\n"
+      "           2: [65533, 1, 1, 9]\n"
+      "           3: [10]\n"              // Implicit repair here.
+      "         0: [65534]\n"
+      "       0: [65534]\n",
+      Munger.getTestResults());
+}
+
+// Show that we recognize when an abbreviation definition record is
+// malformed.  Also show that we repair the problem by removing the
+// definition.
+TEST(NaClMungeWriteErrorsTests, TestWritingMalformedAbbreviation) {
+  // Create edits to:
+  // 1) Expand the number of abbreviation index bits for the block from 2 to 3.
+  // 2) Leave out the "literal" operand encoding out.
+  const uint64_t FunctionEnterIndex = 7;
+  const uint64_t Edits[] {
+    FunctionEnterIndex, NaClMungedBitcode::Replace,   // Set Abbrev bits = 3
+        1, naclbitc::BLK_CODE_ENTER, naclbitc::FUNCTION_BLOCK_ID, 3, Terminator,
+    RetVoidIndex, NaClMungedBitcode::AddBefore,       // bad abbreviation!
+        2, naclbitc::BLK_CODE_DEFINE_ABBREV, 1, // 1,

[jvoung (off chromium), 2015/05/20 22:40:10]

How about expand on why the "// 1,"? E.g.,

// 1,

// 1, intentionally leave out "literal" operand

[Karl, 2015/05/21 18:22:08]

Done.

+        naclbitc::FUNC_CODE_INST_RET, Terminator,
+        };
+
+  NaClWriteMunger Munger(ARRAY_TERM(BitcodeRecords));
+  Munger.setTryToRecoverOnWrite(true);
+  EXPECT_TRUE(Munger.runTest(ARRAY(Edits)));
+  EXPECT_EQ(
+      "Error (Block 12): Error: Bad abbreviation operand encoding 10: "
+      "2: [65533, 1, 10]\n"
+      "       1: [65535, 8, 2]\n"
+      "         1: [65535, 17, 3]\n"
+      "           3: [1, 2]\n"
+      "           3: [2]\n"
+      "           3: [21, 0, 0]\n"
+      "         0: [65534]\n"
+      "         3: [8, 1, 0, 0, 0]\n"
+      "         1: [65535, 12, 3]\n"  // Note: not followed by abbreviation def.
+      "           3: [1, 1]\n"
+      "           3: [10]\n"
+      "         0: [65534]\n"
+      "       0: [65534]\n",
+      Munger.getTestResults());
+}
+
+// Show how we deal with additional abbreviations defined for a block,
+// once a bad abbreviation definition record is found. That is, we
+// remove all succeeding abbreviations definitions for that block. In
+// addition, any record refering to a remove abbreviation is changed
+// to use the default abbreviation.
+TEST(NaClMungedWriteErrorTests, TestRemovingAbbrevWithMultAbbrevs) {
+  NaClWriteMunger Munger(ARRAY_TERM(BitcodeRecords));
+  const uint64_t FunctionEnterIndex = 7;
+  const uint64_t Edits[] {
+    FunctionEnterIndex, NaClMungedBitcode::Replace,   // Set Abbrev bits = 3
+        1, naclbitc::BLK_CODE_ENTER, naclbitc::FUNCTION_BLOCK_ID, 3, Terminator,
+    RetVoidIndex, NaClMungedBitcode::AddBefore,  // bad abbreviation!
+        2, naclbitc::BLK_CODE_DEFINE_ABBREV, 1, // 1,
+        naclbitc::FUNC_CODE_INST_RET - 1, Terminator,
+    RetVoidIndex, NaClMungedBitcode::AddBefore,  // good abbreviation to ignore.
+        2, naclbitc::BLK_CODE_DEFINE_ABBREV, 1, 1,
+        naclbitc::FUNC_CODE_INST_RET, Terminator,
+    RetVoidIndex, NaClMungedBitcode::Replace,  // reference to good abreviation.
+        5, naclbitc::FUNC_CODE_INST_RET, Terminator
+        };
+
+  Munger.setTryToRecoverOnWrite(true);
+  EXPECT_TRUE(Munger.runTest(ARRAY(Edits)));
+  EXPECT_EQ(
+      "Error (Block 12): Error: Bad abbreviation operand encoding 9:"
+      " 2: [65533, 1, 9]\n"
+      "Error (Block 12): Ignoring abbreviation: 2: [65533, 1, 1, 10]\n"
+      "Error (Block 12): Uses illegal abbreviation index: 5: [10]\n"
+      "       1: [65535, 8, 2]\n"
+      "         1: [65535, 17, 3]\n"
+      "           3: [1, 2]\n"
+      "           3: [2]\n"
+      "           3: [21, 0, 0]\n"
+      "         0: [65534]\n"
+      "         3: [8, 1, 0, 0, 0]\n"
+      "         1: [65535, 12, 3]\n"
+      "           3: [1, 1]\n"
+      "           3: [10]\n"     // Abbreviation index 5 replaced with default.
+      "         0: [65534]\n"
+      "       0: [65534]\n",
+      Munger.getTestResults());
+}
+
 // Show that error recovery works when writing an illegal abbreviation
 // index. Show success by parsing fixed bitcode.
 TEST(NaClMungeWriteErrorTests, RecoverWhenParsingBadAbbrevIndex) {