Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(41)

Issue 1310883003: Install notion of diagnostic handler into PNaCl bitcode readers. (Closed)

Created:
5 years, 4 months ago by Karl
Modified:
5 years, 3 months ago
CC:
native-client-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/native_client/pnacl-llvm.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Reformat and fix nits. #

Total comments: 18

Patch Set 3 : Fix issues in patch set 2. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+390 lines, -424 lines) Patch
M include/llvm/Bitcode/NaCl/NaClBitcodeMunge.h View 1 2 chunks +6 lines, -3 lines 0 comments Download
M include/llvm/Bitcode/NaCl/NaClReaderWriter.h View 1 3 chunks +22 lines, -21 lines 0 comments Download
M include/llvm/IRReader/IRReader.h View 1 2 chunks +11 lines, -16 lines 0 comments Download
M lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp View 1 85 chunks +212 lines, -303 lines 0 comments Download
M lib/Bitcode/NaCl/TestUtils/NaClBitcodeMunge.cpp View 1 chunk +1 line, -3 lines 0 comments Download
M lib/Bitcode/NaCl/TestUtils/NaClBitcodeTextReader.cpp View 1 chunk +2 lines, -1 line 0 comments Download
M lib/IRReader/IRReader.cpp View 1 5 chunks +13 lines, -14 lines 0 comments Download
M test/NaCl/Bitcode/invalid.test View 1 chunk +1 line, -1 line 0 comments Download
M test/NaCl/Bitcode/pnacl-bcdis/invalid.test View 1 chunk +2 lines, -2 lines 0 comments Download
M tools/llvm-dis/llvm-dis.cpp View 1 2 1 chunk +3 lines, -2 lines 0 comments Download
M tools/pnacl-abicheck/pnacl-abicheck.cpp View 1 2 chunks +5 lines, -3 lines 0 comments Download
M tools/pnacl-llc/pnacl-llc.cpp View 1 2 2 chunks +7 lines, -5 lines 0 comments Download
M tools/pnacl-thaw/pnacl-thaw.cpp View 1 4 chunks +7 lines, -16 lines 0 comments Download
M unittests/Bitcode/CMakeLists.txt View 1 chunk +1 line, -0 lines 0 comments Download
M unittests/Bitcode/NaClBitReaderTest.cpp View 1 4 chunks +14 lines, -8 lines 0 comments Download
M unittests/Bitcode/NaClMungeTest.h View 1 chunk +4 lines, -0 lines 0 comments Download
A unittests/Bitcode/NaClMungeTest.cpp View 1 2 1 chunk +62 lines, -0 lines 0 comments Download
M unittests/Bitcode/NaClMungeWriteErrorTests.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M unittests/Bitcode/NaClParseInstsTest.cpp View 1 4 chunks +12 lines, -16 lines 0 comments Download
M unittests/Bitcode/NaClParseTypesTest.cpp View 1 1 chunk +4 lines, -9 lines 0 comments Download

Messages

Total messages: 12 (1 generated)
Karl
5 years, 4 months ago (2015-08-24 19:49:31 UTC) #2
Derek Schuff
https://codereview.chromium.org/1310883003/diff/20001/include/llvm/Bitcode/NaCl/NaClReaderWriter.h File include/llvm/Bitcode/NaCl/NaClReaderWriter.h (right): https://codereview.chromium.org/1310883003/diff/20001/include/llvm/Bitcode/NaCl/NaClReaderWriter.h#newcode81 include/llvm/Bitcode/NaCl/NaClReaderWriter.h:81: std::string *ErrMsg = nullptr, bool AcceptSupportedOnly = true); is ...
5 years, 4 months ago (2015-08-24 21:22:01 UTC) #3
Karl
https://codereview.chromium.org/1310883003/diff/20001/include/llvm/Bitcode/NaCl/NaClReaderWriter.h File include/llvm/Bitcode/NaCl/NaClReaderWriter.h (right): https://codereview.chromium.org/1310883003/diff/20001/include/llvm/Bitcode/NaCl/NaClReaderWriter.h#newcode81 include/llvm/Bitcode/NaCl/NaClReaderWriter.h:81: std::string *ErrMsg = nullptr, bool AcceptSupportedOnly = true); On ...
5 years, 4 months ago (2015-08-24 22:23:18 UTC) #4
Karl
https://codereview.chromium.org/1310883003/diff/20001/include/llvm/Bitcode/NaCl/NaClReaderWriter.h File include/llvm/Bitcode/NaCl/NaClReaderWriter.h (right): https://codereview.chromium.org/1310883003/diff/20001/include/llvm/Bitcode/NaCl/NaClReaderWriter.h#newcode81 include/llvm/Bitcode/NaCl/NaClReaderWriter.h:81: std::string *ErrMsg = nullptr, bool AcceptSupportedOnly = true); On ...
5 years, 4 months ago (2015-08-24 22:23:18 UTC) #5
Derek Schuff
https://codereview.chromium.org/1310883003/diff/20001/tools/llvm-dis/llvm-dis.cpp File tools/llvm-dis/llvm-dis.cpp (right): https://codereview.chromium.org/1310883003/diff/20001/tools/llvm-dis/llvm-dis.cpp#newcode193 tools/llvm-dis/llvm-dis.cpp:193: Context, DiagnosticHandler, why not just pass nullptr here instead ...
5 years, 4 months ago (2015-08-24 22:46:04 UTC) #6
jvoung (off chromium)
small comments, otherwise I'll defer to Derek for the rest review https://codereview.chromium.org/1310883003/diff/20001/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right): ...
5 years, 4 months ago (2015-08-25 13:31:10 UTC) #7
Karl
https://codereview.chromium.org/1310883003/diff/20001/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right): https://codereview.chromium.org/1310883003/diff/20001/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp#newcode198 lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:198: explicit NaClBitcodeReader(MemoryBuffer *buffer, LLVMContext &C, On 2015/08/25 13:31:10, jvoung ...
5 years, 4 months ago (2015-08-25 16:15:48 UTC) #8
Derek Schuff
LGTM with the one nit https://codereview.chromium.org/1310883003/diff/20001/tools/llvm-dis/llvm-dis.cpp File tools/llvm-dis/llvm-dis.cpp (right): https://codereview.chromium.org/1310883003/diff/20001/tools/llvm-dis/llvm-dis.cpp#newcode193 tools/llvm-dis/llvm-dis.cpp:193: Context, DiagnosticHandler, On 2015/08/24 ...
5 years, 4 months ago (2015-08-25 17:06:05 UTC) #9
Karl
Committed patchset #3 (id:40001) manually as 91f9db5ac6b069656f3190fbd0c4d109f497ebb9 (presubmit successful).
5 years, 4 months ago (2015-08-25 20:45:58 UTC) #10
Karl
https://codereview.chromium.org/1310883003/diff/20001/tools/llvm-dis/llvm-dis.cpp File tools/llvm-dis/llvm-dis.cpp (right): https://codereview.chromium.org/1310883003/diff/20001/tools/llvm-dis/llvm-dis.cpp#newcode193 tools/llvm-dis/llvm-dis.cpp:193: Context, DiagnosticHandler, On 2015/08/25 17:06:04, Derek Schuff wrote: > ...
5 years, 3 months ago (2015-08-26 17:39:49 UTC) #11
Derek Schuff
5 years, 3 months ago (2015-09-02 23:04:05 UTC) #12
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698