Install notion of diagnostic handler into PNaCl bitcode readers.

Install notion of diagnostic handler into PNaCl bitcode readers.

There has been much divergence between the LLVM and PNaCl bitcode
readers.  Part of this has been due to incremental changes in upstream
LLVM, that resulted in two different models of error handling.

The first change in upstream LLVM was a simplification of error
messages to a short list of error codes. When this was ported to
PNaCl, a notion of "verbose" was added to keep more descriptive error
messages.

The second change in upstream LLVM was than to replace the error codes
with a call to a "diagnostic handler function" that took an error_code
and an error message. The diagnostic handler function is responsable
for generating the corresponding error message and (optionally)
stopping the program after generating the corresponding error.

By default, the LLVM bitcode parsers use the diagnostic handler
defined by the LLVM context of the module, which generates a
corresponding fatal error (terminating) the program.

The advantage of this second scheme can be seen for testing. In
testing, we want the test function to complete (rather than fatally
terminate), so that the corresponding tests on the results can be
applied. Hence, the diagnostic handler function (in such cases)
records the diagnostics in a string stream, so that it can be
extracted after the test completes. This latter form of diagnostic
handler supercedes the need for "verbose" arguments to bitcode
parsers.

This patch changes the PNaCl bitcode readers to use a diagnostic
handler, instead of simple error_codes and a "verbose" argument to
record error messages for testing.  To make redirecting diagnostic
error messages easier, the function redirectNaClDiagnosticToStream was
added.

In addition, to simplify testing, a "stripErrorPrefix" function was
added to some unit tests. This function removes the diagnostic
sevevity prefix from the string containing error messages. This makes
tests more stable in that they do not need to test severity levels,
and the many variants of how severity is inserted.

BUG=None
R=dschuff@chromium.org

Committed: https://chromium.googlesource.com/native_client/pnacl-llvm/+/91f9db5ac6b069656f3190fbd0c4d109f497ebb9

[Karl, 2015-08-24 19:49:19]

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

[Derek Schuff, 2015-08-24 21:22:01]

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

https://codereview.chromium.org/1310883003/diff/20001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClReaderWriter.h:81: std::string *ErrMsg = nullptr,
bool AcceptSupportedOnly = true);
is ErrMsg used now that we have the diagnostic handler?

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

https://codereview.chromium.org/1310883003/diff/20001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:229: std::error_code
Error(std::error_code EC, const Twine &Message) const;
so error_code is still used internally in the bitcode reader, though not exposed
in its external API? Is the plan to get rid of it entirely?

[Karl, 2015-08-24 22:23:18]

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

https://codereview.chromium.org/1310883003/diff/20001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClReaderWriter.h:81: std::string *ErrMsg = nullptr,
bool AcceptSupportedOnly = true);
On 2015/08/24 21:22:01, Derek Schuff wrote:
> is ErrMsg used now that we have the diagnostic handler?

I didn't clean this up in this patch. I removed the Verbose argument, bit did
not remove ErrMsg, since it had other (small) ramifications on llvm-dis.cpp,
pnacl-llc.cpp, and pnacl-thaw.cpp.

Since I will bit fixing the return type of this function anyway, to match the
external API's used by LLVM, I also play to remove ErrMsg in a separate CL.

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

https://codereview.chromium.org/1310883003/diff/20001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:229: std::error_code
Error(std::error_code EC, const Twine &Message) const;
On 2015/08/24 21:22:01, Derek Schuff wrote:
> so error_code is still used internally in the bitcode reader, though not
exposed
> in its external API? Is the plan to get rid of it entirely? 

It is exposed. The error_code is what is returned to the caller, when an error
occurs. It is used when the "diagnostic handler function" returns (i.e. doesn't
generate a fatal error). In such cases, it is the error to propagate back to the
(ultimate) caller.

[Karl, 2015-08-24 22:23:18]

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

https://codereview.chromium.org/1310883003/diff/20001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClReaderWriter.h:81: std::string *ErrMsg = nullptr,
bool AcceptSupportedOnly = true);
On 2015/08/24 21:22:01, Derek Schuff wrote:
> is ErrMsg used now that we have the diagnostic handler?

I didn't clean this up in this patch. I removed the Verbose argument, bit did
not remove ErrMsg, since it had other (small) ramifications on llvm-dis.cpp,
pnacl-llc.cpp, and pnacl-thaw.cpp.

Since I will bit fixing the return type of this function anyway, to match the
external API's used by LLVM, I also play to remove ErrMsg in a separate CL.

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

https://codereview.chromium.org/1310883003/diff/20001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:229: std::error_code
Error(std::error_code EC, const Twine &Message) const;
On 2015/08/24 21:22:01, Derek Schuff wrote:
> so error_code is still used internally in the bitcode reader, though not
exposed
> in its external API? Is the plan to get rid of it entirely? 

It is exposed. The error_code is what is returned to the caller, when an error
occurs. It is used when the "diagnostic handler function" returns (i.e. doesn't
generate a fatal error). In such cases, it is the error to propagate back to the
(ultimate) caller.

[Derek Schuff, 2015-08-24 22:46:04]

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

https://codereview.chromium.org/1310883003/diff/20001/tools/llvm-dis/llvm-dis...
tools/llvm-dis/llvm-dis.cpp:193: Context, DiagnosticHandler,
why not just pass nullptr here instead of creating an explicit DH?

https://codereview.chromium.org/1310883003/diff/20001/tools/pnacl-llc/pnacl-l...
File tools/pnacl-llc/pnacl-llc.cpp (right):

https://codereview.chromium.org/1310883003/diff/20001/tools/pnacl-llc/pnacl-l...
tools/pnacl-llc/pnacl-llc.cpp:370: DiagnosticHandlerFunction DiagnosticHandler =
nullptr;
can we not redirect error output in the streaming case? What does the default
diagnostic handler do?

https://codereview.chromium.org/1310883003/diff/20001/unittests/Bitcode/NaClM...
File unittests/Bitcode/NaClMungeTest.cpp (right):

https://codereview.chromium.org/1310883003/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeTest.cpp:47: size_t NextEoln =
Message.find_first_of("\n", StartPos);
could this just be find instead of find_first_of?

https://codereview.chromium.org/1310883003/diff/20001/unittests/Bitcode/NaClP...
File unittests/Bitcode/NaClParseTypesTest.cpp (right):

https://codereview.chromium.org/1310883003/diff/20001/unittests/Bitcode/NaClP...
unittests/Bitcode/NaClParseTypesTest.cpp:65:
stripErrorPrefix(Munger.getTestResults()));
I don't exactly understand what this is testing for, but is it intentional that
you've reduced the number of tests from 2 to 1?

[jvoung (off chromium), 2015-08-25 13:31:10]

small comments, otherwise I'll defer to Derek for the rest review

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

https://codereview.chromium.org/1310883003/diff/20001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:198: explicit
NaClBitcodeReader(MemoryBuffer *buffer, LLVMContext &C,
btw: No need for explicit, but the random cleanups can be a separate change

similar below

https://codereview.chromium.org/1310883003/diff/20001/tools/pnacl-llc/pnacl-l...
File tools/pnacl-llc/pnacl-llc.cpp (right):

https://codereview.chromium.org/1310883003/diff/20001/tools/pnacl-llc/pnacl-l...
tools/pnacl-llc/pnacl-llc.cpp:381: InputFilename, Cache.release(), Context);
Similar to what Derek asked: can the LLVMFormat case also take a DH? (though it
didn't touch VerboseStrm prior to this CL...)

Otherwise, with this change, VerboseStrm isn't filled in the
PNACL_BROWSER_TRANSLATOR case anymore, even though the code below will try to
pull from it:

#if defined(PNACL_BROWSER_TRANSLATOR)
     report_fatal_error(VerboseStrm.str() + Err.getMessage());
#else
...

That code is trying to take the contents of VerboseStrm and plumb it to the
browser's JS console via report_fatal_error's handler (vs stdout/stderr, which
goes nowhere unless people launch the browser from the terminal).

https://codereview.chromium.org/1310883003/diff/20001/unittests/Bitcode/NaClM...
File unittests/Bitcode/NaClMungeTest.cpp (right):

https://codereview.chromium.org/1310883003/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeTest.cpp:1: //===-
llvm/unittest/Bitcode/NaClMungeTest.cpp - Test munging utils -------===//
unittest -> unittests

[Karl, 2015-08-25 16:15:48]

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

https://codereview.chromium.org/1310883003/diff/20001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:198: explicit
NaClBitcodeReader(MemoryBuffer *buffer, LLVMContext &C,
On 2015/08/25 13:31:10, jvoung wrote:
> btw: No need for explicit, but the random cleanups can be a separate change
> 
> similar below

Acknowledged.

https://codereview.chromium.org/1310883003/diff/20001/tools/pnacl-llc/pnacl-l...
File tools/pnacl-llc/pnacl-llc.cpp (right):

https://codereview.chromium.org/1310883003/diff/20001/tools/pnacl-llc/pnacl-l...
tools/pnacl-llc/pnacl-llc.cpp:370: DiagnosticHandlerFunction DiagnosticHandler =
nullptr;
On 2015/08/24 22:46:04, Derek Schuff wrote:
> can we not redirect error output in the streaming case? What does the default
> diagnostic handler do?

I admit, I just followed what LLVM did (below). Jan makes a good point about
using the verbose stream to capture the error messages, both here and for the
LLVMFormat case. Changing.

https://codereview.chromium.org/1310883003/diff/20001/unittests/Bitcode/NaClM...
File unittests/Bitcode/NaClMungeTest.cpp (right):

https://codereview.chromium.org/1310883003/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeTest.cpp:1: //===-
llvm/unittest/Bitcode/NaClMungeTest.cpp - Test munging utils -------===//
On 2015/08/25 13:31:10, jvoung wrote:
> unittest -> unittests

Done.

https://codereview.chromium.org/1310883003/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeTest.cpp:47: size_t NextEoln =
Message.find_first_of("\n", StartPos);
On 2015/08/24 22:46:04, Derek Schuff wrote:
> could this just be find instead of find_first_of?

Changed to use find.

https://codereview.chromium.org/1310883003/diff/20001/unittests/Bitcode/NaClP...
File unittests/Bitcode/NaClParseTypesTest.cpp (right):

https://codereview.chromium.org/1310883003/diff/20001/unittests/Bitcode/NaClP...
unittests/Bitcode/NaClParseTypesTest.cpp:65:
stripErrorPrefix(Munger.getTestResults()));
On 2015/08/24 22:46:04, Derek Schuff wrote:
> I don't exactly understand what this is testing for, but is it intentional
that
> you've reduced the number of tests from 2 to 1?

The history is related to what happened when the "verbose" option for parsing
was added to the bitcode parser, and simple "error_codes" were returned as
values.

When you ran the test in non-verbose mode (i.e. runTEST(.., false), you only got
the message from the "error_code" (which is quite generic). When you ran it in
verbose mode (i.e. runTest(.., True)), you also got the more descriptive error
messages as well.

It turns out that this test example was the only case that was exercising this
difference. Hence, I decided to simplify the runTest method to just always use
the diagnostic handler function to capture the more descriptive error messages,
and follow it with the corresponding error_code message. Hence, we now only have
one test.

[Derek Schuff, 2015-08-25 17:06:05]

LGTM with the one nit

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

https://codereview.chromium.org/1310883003/diff/20001/tools/llvm-dis/llvm-dis...
tools/llvm-dis/llvm-dis.cpp:193: Context, DiagnosticHandler,
On 2015/08/24 22:46:04, Derek Schuff wrote:
> why not just pass nullptr here instead of creating an explicit DH?

From in-person conversations, this is just for self-documenting. Can we use the
LLVM style for that, i.e.

M.reset(getNaClStreamedBitcodeModule(DisplayFilename, Buffer.release(),
        Context, /* DiagnosticHandler = */ nullptr,

(here and also in pnacl-llc)

[Karl, 2015-08-25 20:45:58]

Committed patchset #3 (id:40001) manually as
91f9db5ac6b069656f3190fbd0c4d109f497ebb9 (presubmit successful).

[Karl, 2015-08-26 17:39:49]

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

https://codereview.chromium.org/1310883003/diff/20001/tools/llvm-dis/llvm-dis...
tools/llvm-dis/llvm-dis.cpp:193: Context, DiagnosticHandler,
On 2015/08/25 17:06:04, Derek Schuff wrote:
> On 2015/08/24 22:46:04, Derek Schuff wrote:
> > why not just pass nullptr here instead of creating an explicit DH?
> 
> From in-person conversations, this is just for self-documenting. Can we use
the
> LLVM style for that, i.e.
> 
> M.reset(getNaClStreamedBitcodeModule(DisplayFilename, Buffer.release(),
>         Context, /* DiagnosticHandler = */ nullptr,
> 
> (here and also in pnacl-llc)

Done.

[Derek Schuff, 2015-09-02 23:04:05]

On 2015/08/26 17:39:49, Karl wrote:
>
https://codereview.chromium.org/1310883003/diff/20001/tools/llvm-dis/llvm-dis...
> File tools/llvm-dis/llvm-dis.cpp (right):
> 
>
https://codereview.chromium.org/1310883003/diff/20001/tools/llvm-dis/llvm-dis...
> tools/llvm-dis/llvm-dis.cpp:193: Context, DiagnosticHandler,
> On 2015/08/25 17:06:04, Derek Schuff wrote:
> > On 2015/08/24 22:46:04, Derek Schuff wrote:
> > > why not just pass nullptr here instead of creating an explicit DH?
> > 
> > From in-person conversations, this is just for self-documenting. Can we use
> the
> > LLVM style for that, i.e.
> > 
> > M.reset(getNaClStreamedBitcodeModule(DisplayFilename, Buffer.release(),
> >         Context, /* DiagnosticHandler = */ nullptr,
> > 
> > (here and also in pnacl-llc)
> 
> Done.

It looks like the CMake build is failing now because the use of
BitcodeErrorCategory and BitcodeDiagnosticInfo means that libNaClBitReader now
depends on libBitReader. I guess this dependence is ok? we just need to fix the
LLVMBuild file so libNaClBitReader.so will link in CMake.