Fix error reporting in the PNaCl bitcode reader.

Fix error reporting in the PNaCl bitcode reader.

When LLVM 3.5 was merged, the handling of errors was not fixed.
The effect is that error messages do not percolate up to the
top-level routines (Which expect: ErrorOr<Module*>). Rather,
pnacl-llc (and similar reading tools) core dump if there is
an error in the bitcode file.

This CL fixes this issue.

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

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

[Karl, 2014-12-01 21:08:56]

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

[jvoung (off chromium), 2014-12-01 23:23:50]

Was there a way to make a lit or unittest? I guess NaClBitcodeMunge only
unittests the nacl bitcode objdump, which is a different reader.

We do have some Chrome-side tests that when you try to load a non-finalized pexe
w/ the NaCl reader it will print and error message though. Perhaps a similar
basic test could be added here, though it will only test up to the initstream
parts?

https://codereview.chromium.org/770853002/diff/2/include/llvm/Bitcode/NaCl/Na...
File include/llvm/Bitcode/NaCl/NaClReaderWriter.h (right):

https://codereview.chromium.org/770853002/diff/2/include/llvm/Bitcode/NaCl/Na...
include/llvm/Bitcode/NaCl/NaClReaderWriter.h:74: Module
*getNaClStreamedBitcodeModule(const std::string &name,
Interesting... Didn't realize that upstream this isn't also an ErrorOr<Module
*>. I wonder why it is a different interface?

https://codereview.chromium.org/770853002/diff/2/include/llvm/Bitcode/NaCl/Na...
include/llvm/Bitcode/NaCl/NaClReaderWriter.h:78: std::string *ErrMsg = 0,
nullptr

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right):

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:187: return Error(MalformedBlock,
"Malformed block record");
similar, InvalidRecord or MalformedBlock?

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:619: return Error(MalformedBlock,
"Malformed block record");
Sometimes errors in EnterSubBlock are InvalidRecord instead of MalformedBlock.
Can we be consistent?

E.g., ParseValueSymbolTable right below here.

It seems that upstream always uses InvalidRecord for this.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:769: return
Error(NaClBitcodeReader::MalformedBlock,
Upstream they have a separate error code for this. Perhaps this should also be
more specific than MalformedBlock, since MalformedBlock seems to be used for
NaClBitstreamEntry::Error, etc.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1158: return Error(InvalidRecord,
"Malformed block record");
If these are all InvalidRecord, maybe the verbose message shouldn't be
"Malformed block..."

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1495: if (Op == 0)
nullptr instead of 0.

Actually, n/m. It may be better to do these misc 0 to nullptr cleanups in a
separate CL? I see a bunch in this file.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1509: Ptr = ConvertOpToType(Ptr,
Val->getType()->getPointerTo(), CurBBNo);
For loads, the code checks that ConvertOpToType returns a non-nullptr value. Why
was it that STORE did not check that?

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h (right):

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h:188: InvalidBBForInstruction,  // No
basic block for instruction.
Is this the same as InvalidInstructionWithNoBB in the vanilla LLVM reader?

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h:198: NoFunctionInStream, // Unable
to find function in bitcode stream.
Is this the same as "CouldNotFindFunctionInStream" in the vanilla LLVM reader?
It might be good to keep the same name where possible.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h:204: : Context(C), TheModule(0),
Verbose(Verbose), AllowedIntrinsics(&C),
nullptr instead of 0

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h:206: LazyStreamer(0),
NextUnreadBit(0), SeenValueSymbolTable(false),
LazyStreamer(nullptr)

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h:216: : Context(C), TheModule(0),
Verbose(Verbose), AllowedIntrinsics(&C),
TheModule(nullptr), etc.

https://codereview.chromium.org/770853002/diff/2/lib/IR/Module.cpp
File lib/IR/Module.cpp (right):

https://codereview.chromium.org/770853002/diff/2/lib/IR/Module.cpp#newcode398
lib/IR/Module.cpp:398: errs() << "-> Module::Materialize\n";
I assume these are for debugging -- can these be removed now?

https://codereview.chromium.org/770853002/diff/2/lib/IRReader/IRReader.cpp
File lib/IRReader/IRReader.cpp (right):

https://codereview.chromium.org/770853002/diff/2/lib/IRReader/IRReader.cpp#ne...
lib/IRReader/IRReader.cpp:111: ErrorOr<Module *> MOrErr =
"ModuleOrError" to be consistent with ParseIR() ?

https://codereview.chromium.org/770853002/diff/2/lib/IRReader/IRReader.cpp#ne...
lib/IRReader/IRReader.cpp:112: NaClParseBitcodeFile(Buffer, Context,
/*VerboseErrors=*/false);
how do you decide between setting VerboseErrors to true or false?

https://codereview.chromium.org/770853002/diff/2/tools/llvm-nm/llvm-nm.cpp
File tools/llvm-nm/llvm-nm.cpp (right):

https://codereview.chromium.org/770853002/diff/2/tools/llvm-nm/llvm-nm.cpp#ne...
tools/llvm-nm/llvm-nm.cpp:1014: ErrorOr<Module *>Result =
space between > and Result

https://codereview.chromium.org/770853002/diff/2/tools/pnacl-thaw/pnacl-thaw.cpp
File tools/pnacl-thaw/pnacl-thaw.cpp (right):

https://codereview.chromium.org/770853002/diff/2/tools/pnacl-thaw/pnacl-thaw....
tools/pnacl-thaw/pnacl-thaw.cpp:77: Context, /*VerboseErrors=*/false,
Why not set VerboseErrors to true for commandline tools?

[JF, 2014-12-02 00:49:04]

https://codereview.chromium.org/770853002/diff/2/include/llvm/Bitcode/NaCl/Na...
File include/llvm/Bitcode/NaCl/NaClReaderWriter.h (right):

https://codereview.chromium.org/770853002/diff/2/include/llvm/Bitcode/NaCl/Na...
include/llvm/Bitcode/NaCl/NaClReaderWriter.h:82: /// returning the module.  If
an error_code if error occurs.  This
This line doesn't make sense.

https://codereview.chromium.org/770853002/diff/2/include/llvm/Bitcode/NaCl/Na...
include/llvm/Bitcode/NaCl/NaClReaderWriter.h:98: /// VerboseErrors,
AcceptSupportedOnly.
This method doesn't have a VerboseErrors argument.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right):

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:147: return "Unknown bitsteam
error";
I wouldn't put a default here: the compiler will warn if you don't have a
default and you don't handle all of the enum values from
NaClBitcodeReader::ErrorType.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:149: }
Put llvm_unreachable here instead (like BitcodeReader.cpp does).

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:154: static
NaClBitcodeErrorCategoryType ErrCat;
BitcodeReader.cpp uses a ManagedStatic instead of a function-local static, I'd
do the same thing.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h (right):

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h:349: 
It's kind of ugly, but ReaderWriter.h also implements the following, which you
should probably do too:

namespace std {
template <> struct is_error_code_enum<llvm::BitcodeError> : std::true_type {};
}

https://codereview.chromium.org/770853002/diff/2/lib/IRReader/IRReader.cpp
File lib/IRReader/IRReader.cpp (right):

https://codereview.chromium.org/770853002/diff/2/lib/IRReader/IRReader.cpp#ne...
lib/IRReader/IRReader.cpp:111: ErrorOr<Module *> MOrErr =
On 2014/12/01 23:23:50, jvoung wrote:
> "ModuleOrError" to be consistent with ParseIR() ?

Upstream uses MOrErr, so I'd keep that.

[Karl, 2014-12-03 18:32:10]

I also modified NaClBitcodeMunge.{h,cpp} to be able to test parsing errors, and
added a simple test case.

https://codereview.chromium.org/770853002/diff/2/include/llvm/Bitcode/NaCl/Na...
File include/llvm/Bitcode/NaCl/NaClReaderWriter.h (right):

https://codereview.chromium.org/770853002/diff/2/include/llvm/Bitcode/NaCl/Na...
include/llvm/Bitcode/NaCl/NaClReaderWriter.h:74: Module
*getNaClStreamedBitcodeModule(const std::string &name,
On 2014/12/01 23:23:49, jvoung wrote:
> Interesting... Didn't realize that upstream this isn't also an ErrorOr<Module
> *>. I wonder why it is a different interface?

I noticed it too. However, since it wasn't changed on the LLVM side, I decided
not to change the NaCl side in this CL. Rather, since this CL is already large,
put that off to a separate CL. Added a TODO to make this clear.

https://codereview.chromium.org/770853002/diff/2/include/llvm/Bitcode/NaCl/Na...
include/llvm/Bitcode/NaCl/NaClReaderWriter.h:78: std::string *ErrMsg = 0,
On 2014/12/01 23:23:49, jvoung wrote:
> nullptr

Done.

https://codereview.chromium.org/770853002/diff/2/include/llvm/Bitcode/NaCl/Na...
include/llvm/Bitcode/NaCl/NaClReaderWriter.h:82: /// returning the module.  If
an error_code if error occurs.  This
On 2014/12/02 00:49:03, JF wrote:
> This line doesn't make sense.

Cleaned this up.

https://codereview.chromium.org/770853002/diff/2/include/llvm/Bitcode/NaCl/Na...
include/llvm/Bitcode/NaCl/NaClReaderWriter.h:98: /// VerboseErrors,
AcceptSupportedOnly.
On 2014/12/02 00:49:03, JF wrote:
> This method doesn't have a VerboseErrors argument.

Good point. Incorrectly changed comment to write routine. Reverting to previous
comment.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right):

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:147: return "Unknown bitsteam
error";
On 2014/12/02 00:49:04, JF wrote:
> I wouldn't put a default here: the compiler will warn if you don't have a
> default and you don't handle all of the enum values from
> NaClBitcodeReader::ErrorType.

Done.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:149: }
On 2014/12/02 00:49:04, JF wrote:
> Put llvm_unreachable here instead (like BitcodeReader.cpp does).

Done.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:154: static
NaClBitcodeErrorCategoryType ErrCat;
On 2014/12/02 00:49:03, JF wrote:
> BitcodeReader.cpp uses a ManagedStatic instead of a function-local static, I'd
> do the same thing.

Not in our local version. Waiting for merge?

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:187: return Error(MalformedBlock,
"Malformed block record");
On 2014/12/01 23:23:49, jvoung wrote:
> similar, InvalidRecord or MalformedBlock?

Changed to InvalidRecord.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:619: return Error(MalformedBlock,
"Malformed block record");
On 2014/12/01 23:23:49, jvoung wrote:
> Sometimes errors in EnterSubBlock are InvalidRecord instead of MalformedBlock.
> Can we be consistent?
> 
> E.g., ParseValueSymbolTable right below here.
> 
> It seems that upstream always uses InvalidRecord for this.

Done.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:769: return
Error(NaClBitcodeReader::MalformedBlock,
On 2014/12/01 23:23:49, jvoung wrote:
> Upstream they have a separate error code for this. Perhaps this should also be
> more specific than MalformedBlock, since MalformedBlock seems to be used for
> NaClBitstreamEntry::Error, etc.

Changed to match upstream.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1158: return Error(InvalidRecord,
"Malformed block record");
On 2014/12/01 23:23:49, jvoung wrote:
> If these are all InvalidRecord, maybe the verbose message shouldn't be
> "Malformed block..." 

Upstream uses InvalidRecord if SkipBlock fails. Adding new case to match
"InvalidSkippedBlock" with more readable message.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1495: if (Op == 0)
On 2014/12/01 23:23:49, jvoung wrote:
> nullptr instead of 0.
> 
> Actually, n/m. It may be better to do these misc 0 to nullptr cleanups in a
> separate CL? I see a bunch in this file.

In general, I agree that we should delay. However, I left those I already fixed.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1509: Ptr = ConvertOpToType(Ptr,
Val->getType()->getPointerTo(), CurBBNo);
On 2014/12/01 23:23:49, jvoung wrote:
> For loads, the code checks that ConvertOpToType returns a non-nullptr value.
Why
> was it that STORE did not check that?

Good point. Adding test.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h (right):

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h:188: InvalidBBForInstruction,  // No
basic block for instruction.
On 2014/12/01 23:23:50, jvoung wrote:
> Is this the same as InvalidInstructionWithNoBB in the vanilla LLVM reader?

Yes. I didn't like the name. However, it probably is better to follow the
same convention. Changing to InvalidInstructionWithNoBB.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h:198: NoFunctionInStream, // Unable
to find function in bitcode stream.
On 2014/12/01 23:23:50, jvoung wrote:
> Is this the same as "CouldNotFindFunctionInStream" in the vanilla LLVM reader?
> It might be good to keep the same name where possible.

Like above, changing to CouldNotFindFunctionInStream to be more consistent
with LLVM reader.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h:204: : Context(C), TheModule(0),
Verbose(Verbose), AllowedIntrinsics(&C),
On 2014/12/01 23:23:49, jvoung wrote:
> nullptr instead of 0

Done.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h:206: LazyStreamer(0),
NextUnreadBit(0), SeenValueSymbolTable(false),
On 2014/12/01 23:23:49, jvoung wrote:
> LazyStreamer(nullptr)

Done.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h:216: : Context(C), TheModule(0),
Verbose(Verbose), AllowedIntrinsics(&C),
On 2014/12/01 23:23:50, jvoung wrote:
> TheModule(nullptr), etc.

Done.

https://codereview.chromium.org/770853002/diff/2/lib/Bitcode/NaCl/Reader/NaCl...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h:349: 
On 2014/12/02 00:49:04, JF wrote:
> It's kind of ugly, but ReaderWriter.h also implements the following, which you
> should probably do too:
> 
> namespace std {
> template <> struct is_error_code_enum<llvm::BitcodeError> : std::true_type {};
> }

Not in our local code base yet.

https://codereview.chromium.org/770853002/diff/2/lib/IR/Module.cpp
File lib/IR/Module.cpp (right):

https://codereview.chromium.org/770853002/diff/2/lib/IR/Module.cpp#newcode398
lib/IR/Module.cpp:398: errs() << "-> Module::Materialize\n";
On 2014/12/01 23:23:50, jvoung wrote:
> I assume these are for debugging -- can these be removed now?

Reverted and removed from CL.

https://codereview.chromium.org/770853002/diff/2/lib/IRReader/IRReader.cpp
File lib/IRReader/IRReader.cpp (right):

https://codereview.chromium.org/770853002/diff/2/lib/IRReader/IRReader.cpp#ne...
lib/IRReader/IRReader.cpp:111: ErrorOr<Module *> MOrErr =
On 2014/12/01 23:23:50, jvoung wrote:
> "ModuleOrError" to be consistent with ParseIR() ?

Since our local version uses ModuleOrError, will make consistent with that.

https://codereview.chromium.org/770853002/diff/2/lib/IRReader/IRReader.cpp#ne...
lib/IRReader/IRReader.cpp:112: NaClParseBitcodeFile(Buffer, Context,
/*VerboseErrors=*/false);
On 2014/12/01 23:23:50, jvoung wrote:
> how do you decide between setting VerboseErrors to true or false?

Good point. Adding as argument so that calling tool can decide.

https://codereview.chromium.org/770853002/diff/2/tools/llvm-nm/llvm-nm.cpp
File tools/llvm-nm/llvm-nm.cpp (right):

https://codereview.chromium.org/770853002/diff/2/tools/llvm-nm/llvm-nm.cpp#ne...
tools/llvm-nm/llvm-nm.cpp:1014: ErrorOr<Module *>Result =
On 2014/12/01 23:23:50, jvoung wrote:
> space between > and Result

Done.

https://codereview.chromium.org/770853002/diff/2/tools/pnacl-thaw/pnacl-thaw.cpp
File tools/pnacl-thaw/pnacl-thaw.cpp (right):

https://codereview.chromium.org/770853002/diff/2/tools/pnacl-thaw/pnacl-thaw....
tools/pnacl-thaw/pnacl-thaw.cpp:77: Context, /*VerboseErrors=*/false,
On 2014/12/01 23:23:50, jvoung wrote:
> Why not set VerboseErrors to true for commandline tools?

Done.

[jvoung (off chromium), 2014-12-03 19:43:47]

A couple of nits, but looks pretty good, thanks!

https://codereview.chromium.org/770853002/diff/30001/include/llvm/Bitcode/NaC...
File include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h (right):

https://codereview.chromium.org/770853002/diff/30001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h:99: /// Returns the resulting
string generated by NaClObjDump.
cleanup for comment: this is now more generic than just NaClObjDump

https://codereview.chromium.org/770853002/diff/30001/tools/llvm-dis/llvm-dis.cpp
File tools/llvm-dis/llvm-dis.cpp (right):

https://codereview.chromium.org/770853002/diff/30001/tools/llvm-dis/llvm-dis....
tools/llvm-dis/llvm-dis.cpp:170: raw_ostream *Verbose = VerboseErrors ? &errs()
: nullptr;
I guess I was okay with this unconditionally having VerboseErrors. Thoughts?

https://codereview.chromium.org/770853002/diff/30001/tools/llvm-nm/llvm-nm.cpp
File tools/llvm-nm/llvm-nm.cpp (right):

https://codereview.chromium.org/770853002/diff/30001/tools/llvm-nm/llvm-nm.cp...
tools/llvm-nm/llvm-nm.cpp:1021:
NaClParseBitcodeFile(BufferOrErr.get().release(), VerboseErrors,
NaClParseBitcodeFile now takes a pointer instead of a bool.

https://codereview.chromium.org/770853002/diff/30001/tools/pnacl-llc/pnacl-ll...
File tools/pnacl-llc/pnacl-llc.cpp (right):

https://codereview.chromium.org/770853002/diff/30001/tools/pnacl-llc/pnacl-ll...
tools/pnacl-llc/pnacl-llc.cpp:335: raw_ostream *Verbose = VerboseErrors ?
&errs() : nullptr;
Now that this is a stream, __native_client__/pnacl-llc in the browser could
potentially have a raw_string_ostream, and concat/prepend that to the
Err.getMessage below. On the other hand, maybe the errors are sufficient and
nullptr is fine.

https://codereview.chromium.org/770853002/diff/30001/unittests/Bitcode/CMakeL...
File unittests/Bitcode/CMakeLists.txt (right):

https://codereview.chromium.org/770853002/diff/30001/unittests/Bitcode/CMakeL...
unittests/Bitcode/CMakeLists.txt:17: NaClParseDumpTypesTest.cpp
File is actually "NaClParseTypesTest.cpp"?

https://codereview.chromium.org/770853002/diff/30001/unittests/Bitcode/NaClPa...
File unittests/Bitcode/NaClParseTypesTest.cpp (right):

https://codereview.chromium.org/770853002/diff/30001/unittests/Bitcode/NaClPa...
unittests/Bitcode/NaClParseTypesTest.cpp:2: //     Tests objdump stream for
PNaCl bitcode.
Tests ... parser ... 
instead of "objdump stream" ?

https://codereview.chromium.org/770853002/diff/30001/unittests/Bitcode/NaClPa...
unittests/Bitcode/NaClParseTypesTest.cpp:26: TEST(NaClParseTypesTest,
BadTypeReferences) {
Thanks for adding the test!

[Karl, 2014-12-03 20:53:44]

https://codereview.chromium.org/770853002/diff/30001/include/llvm/Bitcode/NaC...
File include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h (right):

https://codereview.chromium.org/770853002/diff/30001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h:99: /// Returns the resulting
string generated by NaClObjDump.
On 2014/12/03 19:43:46, jvoung wrote:
> cleanup for comment: this is now more generic than just NaClObjDump

Done.

https://codereview.chromium.org/770853002/diff/30001/tools/llvm-dis/llvm-dis.cpp
File tools/llvm-dis/llvm-dis.cpp (right):

https://codereview.chromium.org/770853002/diff/30001/tools/llvm-dis/llvm-dis....
tools/llvm-dis/llvm-dis.cpp:170: raw_ostream *Verbose = VerboseErrors ? &errs()
: nullptr;
On 2014/12/03 19:43:46, jvoung wrote:
> I guess I was okay with this unconditionally having VerboseErrors. Thoughts?

Since this is an LLVM tool, I guess we should be just as cryptic. Changing to
use nullptr.

https://codereview.chromium.org/770853002/diff/30001/tools/llvm-nm/llvm-nm.cpp
File tools/llvm-nm/llvm-nm.cpp (right):

https://codereview.chromium.org/770853002/diff/30001/tools/llvm-nm/llvm-nm.cp...
tools/llvm-nm/llvm-nm.cpp:1021:
NaClParseBitcodeFile(BufferOrErr.get().release(), VerboseErrors,
On 2014/12/03 19:43:46, jvoung wrote:
> NaClParseBitcodeFile now takes a pointer instead of a bool.

Done.

https://codereview.chromium.org/770853002/diff/30001/tools/pnacl-llc/pnacl-ll...
File tools/pnacl-llc/pnacl-llc.cpp (right):

https://codereview.chromium.org/770853002/diff/30001/tools/pnacl-llc/pnacl-ll...
tools/pnacl-llc/pnacl-llc.cpp:335: raw_ostream *Verbose = VerboseErrors ?
&errs() : nullptr;
On 2014/12/03 19:43:46, jvoung wrote:
> Now that this is a stream, __native_client__/pnacl-llc in the browser could
> potentially have a raw_string_ostream, and concat/prepend that to the
> Err.getMessage below. On the other hand, maybe the errors are sufficient and
> nullptr is fine.

Went ahead and added verbose errors if command line argument says so.

https://codereview.chromium.org/770853002/diff/30001/unittests/Bitcode/CMakeL...
File unittests/Bitcode/CMakeLists.txt (right):

https://codereview.chromium.org/770853002/diff/30001/unittests/Bitcode/CMakeL...
unittests/Bitcode/CMakeLists.txt:17: NaClParseDumpTypesTest.cpp
On 2014/12/03 19:43:46, jvoung wrote:
> File is actually "NaClParseTypesTest.cpp"?

Yes. Fixed.

https://codereview.chromium.org/770853002/diff/30001/unittests/Bitcode/NaClPa...
File unittests/Bitcode/NaClParseTypesTest.cpp (right):

https://codereview.chromium.org/770853002/diff/30001/unittests/Bitcode/NaClPa...
unittests/Bitcode/NaClParseTypesTest.cpp:2: //     Tests objdump stream for
PNaCl bitcode.
On 2014/12/03 19:43:46, jvoung wrote:
> Tests ... parser ... 
> instead of "objdump stream" ?

Done.

https://codereview.chromium.org/770853002/diff/30001/unittests/Bitcode/NaClPa...
unittests/Bitcode/NaClParseTypesTest.cpp:26: TEST(NaClParseTypesTest,
BadTypeReferences) {
On 2014/12/03 19:43:47, jvoung wrote:
> Thanks for adding the test!

Acknowledged.

[jvoung (off chromium), 2014-12-03 22:11:13]

https://codereview.chromium.org/770853002/diff/30001/tools/pnacl-llc/pnacl-ll...
File tools/pnacl-llc/pnacl-llc.cpp (right):

https://codereview.chromium.org/770853002/diff/30001/tools/pnacl-llc/pnacl-ll...
tools/pnacl-llc/pnacl-llc.cpp:335: raw_ostream *Verbose = VerboseErrors ?
&errs() : nullptr;
On 2014/12/03 20:53:44, Karl wrote:
> On 2014/12/03 19:43:46, jvoung wrote:
> > Now that this is a stream, __native_client__/pnacl-llc in the browser could
> > potentially have a raw_string_ostream, and concat/prepend that to the
> > Err.getMessage below. On the other hand, maybe the errors are sufficient and
> > nullptr is fine.
> 
> Went ahead and added verbose errors if command line argument says so.

What I meant was why not pass in the VerboseStrm always? There's no way to set
this particular commandline flag when loading a pexe in the browser. Only a
subset of flags are set in the browser e.g., O0 vs O2 via the NMF, or when the
browser detects that the machine has SSE4 it'll set --mattr=...).  Then we get a
tad more info when people copy-paste whatever shows up in the JS console. 
Otherwise, this is "dead code" but not in the sense that the compiler can
optimize it out (vs what you had in patch set 2, where you hardcoded it to
false... well, potentially optimized out).

That said, probably the more common type of problems we'll run into are when we
extend the bitcode format and the developer is using an old browser w/ a new
pexe. In that case, we could be more careful bumping the bitcode version or
feature vector, so that the error message is indicated by the header check
instead of deep down somewhere having the reader says some error about an
invalid record.

[Karl, 2014-12-03 22:28:51]

https://codereview.chromium.org/770853002/diff/30001/tools/pnacl-llc/pnacl-ll...
File tools/pnacl-llc/pnacl-llc.cpp (right):

https://codereview.chromium.org/770853002/diff/30001/tools/pnacl-llc/pnacl-ll...
tools/pnacl-llc/pnacl-llc.cpp:335: raw_ostream *Verbose = VerboseErrors ?
&errs() : nullptr;
On 2014/12/03 22:11:13, jvoung wrote:
> On 2014/12/03 20:53:44, Karl wrote:
> > On 2014/12/03 19:43:46, jvoung wrote:
> > > Now that this is a stream, __native_client__/pnacl-llc in the browser
could
> > > potentially have a raw_string_ostream, and concat/prepend that to the
> > > Err.getMessage below. On the other hand, maybe the errors are sufficient
and
> > > nullptr is fine.
> > 
> > Went ahead and added verbose errors if command line argument says so.
> 
> What I meant was why not pass in the VerboseStrm always? There's no way to set
> this particular commandline flag when loading a pexe in the browser. Only a
> subset of flags are set in the browser e.g., O0 vs O2 via the NMF, or when the
> browser detects that the machine has SSE4 it'll set --mattr=...).  Then we get
a
> tad more info when people copy-paste whatever shows up in the JS console. 
> Otherwise, this is "dead code" but not in the sense that the compiler can
> optimize it out (vs what you had in patch set 2, where you hardcoded it to
> false... well, potentially optimized out).
> 
> That said, probably the more common type of problems we'll run into are when
we
> extend the bitcode format and the developer is using an old browser w/ a new
> pexe. In that case, we could be more careful bumping the bitcode version or
> feature vector, so that the error message is indicated by the header check
> instead of deep down somewhere having the reader says some error about an
> invalid record.

Ok. I'll make it be always when in __native_client__. I double checked the code,
and it will (correctly) add verbose errors corresponding to bad header (and then
return) if there are any problem. Hence, the common type of problem will be
reported in this case.

[Karl, 2014-12-03 22:32:53]

Fixed typo (used VerboseStrm instead of &VerboseStrm in pnacl-llc.cpp).

[jvoung (off chromium), 2014-12-03 22:34:14]

LGTM

[Karl, 2014-12-03 22:47:40]

Committed patchset #6 (id:90001) manually as
12f562956efe164da47f3134eff27b56e4f56174 (presubmit successful).