Fix the NaCl bitstream reader to report fatal errors.

Fix the NaCl bitstream reader to report fatal errors.

The current code has several asserts on bitcode file read values. This
results in assert errors for badly user-defined input, causing the
application to crash. This CL Fixes this problem by providing an error
handling class (that can be overridden) to define how fatal errors
should be handled by the bitstream reader.

It also consolidates the printing of bit addresses into one place, and
then uses this to prefix (default) bitstream error messages.

It also fixes a bug where the NaClBitcodeReader/NaClBitcodeParser bit positions were
relative to the first bit after the header, rather than at
the beginning of the bitcode file.

This fixes pnacl-bcdis from core dumping on bitcode file specified in
bug 4002, since it has an illegal abbreviation index in the bitcode
file.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4002
R=jvoung@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=fad799848e661f81adfeca53660cd5b454928380

[Karl, 2015-02-19 20:47:09]

kschimpf@google.com changed reviewers:
+ jvoung@chromium.org

[jvoung (off chromium), 2015-02-20 00:12:42]

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitcodeParser.h (right):

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeParser.h:418: // Redirects bitstream reader
errors to corresponding parrser error
parrser -> parser

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeParser.h:444: ErrHandler(new
ErrorHandler(this, Cursor));
Sometimes multiple instances of NaClBitcodeParsers are created (to parse nested
blocks). I just want to make sure that doesn't end up allocating and
deallocating multiple error handlers for each level (that would seem wasteful).

Otherwise, I wonder if the ErrHandler field can just be the actual ErrorHandler
class so that it's allocated along with the parser (however that allocation
happens on the stack or heap), but you override the parser pointer.

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:218: /// This class handle
mistakes in the bitstream reader. Redirects
"handles mistakes"

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:224:
ErrorHandler(NaClBitstreamCursor &Cursor) : Cursor(Cursor) {}
explicit

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:299: // handler. Takes ownership
of the new handler and delete's it when
"delete's" -> "deletes"

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:302: ErrHandler =
std::move(NewHandler);
Hmm, I'm not too familiar with what exactly std::move will do to the old RHS
here.

Is this like ErrHandler.reset(NewHandler.release())?

Mainly, does ErrHandler delete the previous value it held before tracking the
new pointer w/ x = std::move(y)?

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/Analys...
File lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp (right):

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/Analys...
lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp:14: #include
"llvm/Support/Format.h"
No longer need Format.h?

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/Analys...
lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp:174: return
NaClBitstreamReader::getBitAddress(Bit, MinByteWidth);
Can this variant of RecordAddress be removed and
NaClBitstreamReader::getBitAddress() inlined, or is that too awkward?

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/Reader...
File lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp (right):

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:38: // the error occurred. This
allows old code to continue to work.
I'm not sure it's necessary to talk about "old code" -- at some point we won't
know what that version refers to?

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:109: llvm_unreachable("Not to be
used with literals!");
How come these are changed to unreachable, instead of a form of fatal error?

Pretty confident they will be unreachable (callers already check this)?

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:227: assert(NumOperands > 0 &&
"Too few operands for abbreviation!");
Should this assert be fatal too?

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClBitcodeMunge.cpp (right):

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMunge.cpp:289: // Fake writing out record
using default abbreviation pattern.
When is this sort of error recovery needed?

Can you just abort either way?

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMunge.cpp:292: uint32_t NumValues =
uint32_t(Values.size());
static_cast<uint32_t>()?

https://codereview.chromium.org/932953002/diff/100001/unittests/Bitcode/NaClO...
File unittests/Bitcode/NaClOtherParseTest.cpp (right):

https://codereview.chromium.org/932953002/diff/100001/unittests/Bitcode/NaClO...
unittests/Bitcode/NaClOtherParseTest.cpp:11: // Other parse tests not specific
to specific Bitcode constructs.
How about "NaClAbbrevErrorTests"?

What "Other" things do you imagine?

https://codereview.chromium.org/932953002/diff/100001/unittests/Bitcode/NaClO...
unittests/Bitcode/NaClOtherParseTest.cpp:43: const uint64_t ReplaceIndex = 3; //
Index for TYPE_COD_VOID;
TYPE_CODE_VOID

https://codereview.chromium.org/932953002/diff/100001/unittests/Bitcode/NaClO...
unittests/Bitcode/NaClOtherParseTest.cpp:93: } // end of anonamous namespace.
anonamous -> anonymous

https://codereview.chromium.org/932953002/diff/100001/unittests/Bitcode/NaClP...
File unittests/Bitcode/NaClParseInstsTest.cpp (right):

https://codereview.chromium.org/932953002/diff/100001/unittests/Bitcode/NaClP...
unittests/Bitcode/NaClParseInstsTest.cpp:58:
//EXPECT_FALSE(DumpMunger.runTestForAssembly("Nonexistant call arg"));
remove commented out version?

[Karl, 2015-02-20 21:41:18]

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitcodeParser.h (right):

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeParser.h:418: // Redirects bitstream reader
errors to corresponding parrser error
parrser -> parse

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeParser.h:444: ErrHandler(new
ErrorHandler(this, Cursor));
On 2015/02/20 00:12:42, jvoung wrote:
> Sometimes multiple instances of NaClBitcodeParsers are created (to parse
nested
> blocks). I just want to make sure that doesn't end up allocating and
> deallocating multiple error handlers for each level (that would seem
wasteful).
> 
> Otherwise, I wonder if the ErrHandler field can just be the actual
ErrorHandler
> class so that it's allocated along with the parser (however that allocation
> happens on the stack or heap), but you override the parser pointer.

Note that the comment states that this constructor is the "dummy" instance that
starts the parser. As such, it creates the error handler and installs it. The
constructor in the protected section (below) that is called by all nested
parsers, doesn't create an error handler.

Hence, we only generate one copy of the error handler. I did think of making it
a field of the parser. However, since we use the same base class for top-level
parsers and nested parsers, I did not do that. To add as a field, would create
multiple instances.

To add it as a field, it would require a refactoring of bitcode parsers (to
separate out top-level from nested).

In addition, I wanted to simplify changes by not having to figure out all
callers of a NaClBitstreamReader. For this reason, I used an owning model of the
error handler. The NaClBitstreamReader creates the default error handler, and
the caller replaces it if it wants to use its own.

In particular, the NaClBitstreamReader is built before it is passed the parsers
(a NaClBitstreamCursor is passed to the parser).

For these reasons, I thought that the solution I've written is a good
compromise, based on the different ways the bitstream reader is used.

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeParser.h:617:
ErrStream(EnclosingParser->ErrStream)
Note that this version (for nested parsers) doesn't create an error handler.

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:218: /// This class handle
mistakes in the bitstream reader. Redirects
On 2015/02/20 00:12:42, jvoung wrote:
> "handles mistakes"

Done.

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:224:
ErrorHandler(NaClBitstreamCursor &Cursor) : Cursor(Cursor) {}
On 2015/02/20 00:12:42, jvoung wrote:
> explicit

Done.

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:299: // handler. Takes ownership
of the new handler and delete's it when
On 2015/02/20 00:12:42, jvoung wrote:
> "delete's" -> "deletes"

Done.

https://codereview.chromium.org/932953002/diff/100001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:302: ErrHandler =
std::move(NewHandler);
On 2015/02/20 00:12:42, jvoung wrote:
> Hmm, I'm not too familiar with what exactly std::move will do to the old RHS
> here.
> 
> Is this like ErrHandler.reset(NewHandler.release())?
> 
> Mainly, does ErrHandler delete the previous value it held before tracking the
> new pointer w/ x = std::move(y)?

That is my understanding. I didn't know about this idiom until Jim pointed it
out. To return a unique_ptr (as a value) from a function, this is the way you do
it. If you look at unique_ptr, it implements MoveAssignable, which is used by
std::move.

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/Analys...
File lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp (right):

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/Analys...
lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp:14: #include
"llvm/Support/Format.h"
On 2015/02/20 00:12:42, jvoung wrote:
> No longer need Format.h?

Done.

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/Analys...
lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp:174: return
NaClBitstreamReader::getBitAddress(Bit, MinByteWidth);
On 2015/02/20 00:12:42, jvoung wrote:
> Can this variant of RecordAddress be removed and
> NaClBitstreamReader::getBitAddress() inlined, or is that too awkward?

Removed all instances of RecordAddress and ObjDumpAddress. I also removed
unnecessary fields (to be able to call these routines), as well as cleaning up
the bitstream cursor of the formatter, so that a start offset is no longer
needed (i.e. it points the the logical position within the file).

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/Reader...
File lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp (right):

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:38: // the error occurred. This
allows old code to continue to work.
On 2015/02/20 00:12:42, jvoung wrote:
> I'm not sure it's necessary to talk about "old code" -- at some point we won't
> know what that version refers to?

Removed comment.

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:109: llvm_unreachable("Not to be
used with literals!");
On 2015/02/20 00:12:42, jvoung wrote:
> How come these are changed to unreachable, instead of a form of fatal error?
> 
> Pretty confident they will be unreachable (callers already check this)?

These should be asserts (if they are reached, it is an error in the code). I
tried to follow previous advice (and changes made by JF) to use
llvm_unreachable, so that we still get an error if asserts are turned off. The
idea being that it is clearer that it should be unreachable. See NaClBitCodes.h
for examples of this.

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:227: assert(NumOperands > 0 &&
"Too few operands for abbreviation!");
On 2015/02/20 00:12:42, jvoung wrote:
> Should this assert be fatal too?

This is just a double check assertion. We already check if abbreviations are
valid after they are built. However, the code there doesn't check this. Fixing
the definition of NaClBitCodeAbbrev::isValid to check this and removing this
assertion.

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClBitcodeMunge.cpp (right):

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMunge.cpp:289: // Fake writing out record
using default abbreviation pattern.
On 2015/02/20 00:12:42, jvoung wrote:
> When is this sort of error recovery needed?
> 
> Can you just abort either way?

The purpose of the munger is to be able to introduce "errors" in bitcode files.
One of them is abbreviations. This code allows illegal abbreviations to be put
in the bitcode file, so that the reader can be tested and report the error.

Updated the comment to reflect this.

https://codereview.chromium.org/932953002/diff/100001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMunge.cpp:292: uint32_t NumValues =
uint32_t(Values.size());
On 2015/02/20 00:12:42, jvoung wrote:
> static_cast<uint32_t>()?

Done.

https://codereview.chromium.org/932953002/diff/100001/unittests/Bitcode/NaClO...
File unittests/Bitcode/NaClOtherParseTest.cpp (right):

https://codereview.chromium.org/932953002/diff/100001/unittests/Bitcode/NaClO...
unittests/Bitcode/NaClOtherParseTest.cpp:11: // Other parse tests not specific
to specific Bitcode constructs.
On 2015/02/20 00:12:42, jvoung wrote:
> How about "NaClAbbrevErrorTests"?
> 
> What "Other" things do you imagine?

Done.

https://codereview.chromium.org/932953002/diff/100001/unittests/Bitcode/NaClO...
unittests/Bitcode/NaClOtherParseTest.cpp:43: const uint64_t ReplaceIndex = 3; //
Index for TYPE_COD_VOID;
On 2015/02/20 00:12:42, jvoung wrote:
> TYPE_CODE_VOID

Done.

https://codereview.chromium.org/932953002/diff/100001/unittests/Bitcode/NaClO...
unittests/Bitcode/NaClOtherParseTest.cpp:93: } // end of anonamous namespace.
On 2015/02/20 00:12:42, jvoung wrote:
> anonamous -> anonymous

Done.

https://codereview.chromium.org/932953002/diff/100001/unittests/Bitcode/NaClP...
File unittests/Bitcode/NaClParseInstsTest.cpp (right):

https://codereview.chromium.org/932953002/diff/100001/unittests/Bitcode/NaClP...
unittests/Bitcode/NaClParseInstsTest.cpp:58:
//EXPECT_FALSE(DumpMunger.runTestForAssembly("Nonexistant call arg"));
On 2015/02/20 00:12:42, jvoung wrote:
> remove commented out version?

Done.

[jvoung (off chromium), 2015-02-23 18:58:56]

https://codereview.chromium.org/932953002/diff/140001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h (right):

https://codereview.chromium.org/932953002/diff/140001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h:227: /// errstream so that they can
be tested.
"errs()" stream

Is it more about being able to see the output for debugging or is it really
tested/checked by gtest?

https://codereview.chromium.org/932953002/diff/140001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitcodeParser.h (right):

https://codereview.chromium.org/932953002/diff/140001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeParser.h:445:
Cursor.setErrorHandler(ErrHandler);
Does "Cursor.setErrorHandler(make_unique<ErrorHandler>(this, Cursor));" or
something like that work (from STLExtras.h)?

https://codereview.chromium.org/932953002/diff/140001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeParser.h:513: // Overrides
NaClBitcodeParser::Error and defines the reporting
Similar question to below Fatal() about "Overrides"

https://codereview.chromium.org/932953002/diff/140001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeParser.h:529: // Overrides
NaClBitcodeParser::Fatal and defines the reporting
I don't quite understand the "Overrides NaClBitcodeParser::Fatal" comment.

Isn't this the definition of NaClBitcodeParser::Fatal method?

https://codereview.chromium.org/932953002/diff/140001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/932953002/diff/140001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:338: return
BitStream->getBitAddress(Bit, MinByteWidth);
Is it useful to have this wrapper or can the bitstream static function be called
directly? I can see the other "getCurrentBitAddress" being more useful since it
also calls GetCurrentBitNo()

https://codereview.chromium.org/932953002/diff/140001/unittests/Bitcode/NaClA...
File unittests/Bitcode/NaClAbbrevErrorTests.cpp (right):

https://codereview.chromium.org/932953002/diff/140001/unittests/Bitcode/NaClA...
unittests/Bitcode/NaClAbbrevErrorTests.cpp:27: TEST(MyDeathNaClOtherParseTest,
BadAbbreviationIndex) {
Could rename "MyDeathNaClOtherParseTest" to something related to abbrev errors
now

[Karl, 2015-02-23 20:46:23]

https://codereview.chromium.org/932953002/diff/140001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h (right):

https://codereview.chromium.org/932953002/diff/140001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h:227: /// errstream so that they can
be tested.
On 2015/02/23 18:58:55, jvoung wrote:
> "errs()" stream
> 
> Is it more about being able to see the output for debugging or is it really
> tested/checked by gtest?

It is about debugging. The problem is that a munge test buffers the output into
a memory buffer, so that it can be queried and compared after the test
completes. However, in death tests, the munge never completes, and hence, no
output is seen. This flag changes that by sending the output to errs() instead
of buffering.

Changing comment to clarify.

https://codereview.chromium.org/932953002/diff/140001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitcodeParser.h (right):

https://codereview.chromium.org/932953002/diff/140001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeParser.h:445:
Cursor.setErrorHandler(ErrHandler);
On 2015/02/23 18:58:56, jvoung wrote:
> Does "Cursor.setErrorHandler(make_unique<ErrorHandler>(this, Cursor));" or
> something like that work (from STLExtras.h)?

The problem is that there is two different types, and no conversion for
unique_ptr:
   NaClBitstreamCursor::ErrorHandler
   NaClBitcodeParser::ErrorHandler

https://codereview.chromium.org/932953002/diff/140001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeParser.h:513: // Overrides
NaClBitcodeParser::Error and defines the reporting
On 2015/02/23 18:58:55, jvoung wrote:
> Similar question to below Fatal() about "Overrides"

Not sure why this comment is here anymore. Removing.

https://codereview.chromium.org/932953002/diff/140001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeParser.h:529: // Overrides
NaClBitcodeParser::Fatal and defines the reporting
On 2015/02/23 18:58:55, jvoung wrote:
> I don't quite understand the "Overrides NaClBitcodeParser::Fatal" comment.
> 
> Isn't this the definition of NaClBitcodeParser::Fatal method?

Also removed comment here.

https://codereview.chromium.org/932953002/diff/140001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/932953002/diff/140001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:338: return
BitStream->getBitAddress(Bit, MinByteWidth);
On 2015/02/23 18:58:56, jvoung wrote:
> Is it useful to have this wrapper or can the bitstream static function be
called
> directly? I can see the other "getCurrentBitAddress" being more useful since
it
> also calls GetCurrentBitNo()

Removed.

https://codereview.chromium.org/932953002/diff/140001/unittests/Bitcode/NaClA...
File unittests/Bitcode/NaClAbbrevErrorTests.cpp (right):

https://codereview.chromium.org/932953002/diff/140001/unittests/Bitcode/NaClA...
unittests/Bitcode/NaClAbbrevErrorTests.cpp:27: TEST(MyDeathNaClOtherParseTest,
BadAbbreviationIndex) {
On 2015/02/23 18:58:56, jvoung wrote:
> Could rename "MyDeathNaClOtherParseTest" to something related to abbrev errors
> now

Done.

[jvoung (off chromium), 2015-02-23 22:57:38]

LGTM but please wait until the chrome 42 regression has been fixed before
landing:

https://code.google.com/p/nativeclient/issues/detail?id=4105

https://codereview.chromium.org/932953002/diff/160001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h (right):

https://codereview.chromium.org/932953002/diff/160001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h:143: std::vector<uint64_t>
AbbrevIndexLimitStack;
Might be able to just use a SmallVector, if typical nesting isn't that high?

[Karl, 2015-02-23 23:09:01]

Ready to commit once regression in chrome 42 fixed.

https://codereview.chromium.org/932953002/diff/160001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h (right):

https://codereview.chromium.org/932953002/diff/160001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h:143: std::vector<uint64_t>
AbbrevIndexLimitStack;
On 2015/02/23 22:57:38, jvoung wrote:
> Might be able to just use a SmallVector, if typical nesting isn't that high?

Good point. It shouldn't get deeper than 3 levels. Replacing with SmallVector.

[Karl, 2015-03-04 20:18:22]

Committed patchset #13 (id:240001) manually as
fad799848e661f81adfeca53660cd5b454928380 (presubmit successful).