Report fatal translator errors to the browser

Report fatal translator errors to the browser

Install a fatal error handler for the translator, which stores the error
string, signals an error to the RPC thread, and terminates the thread
(instead of terminating the whole program). This will cause the error to
go to the Javascript console in Chrome

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

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

[jvoung (off chromium), 2013-09-09 23:21:01]

"error: old chunk mismatch" -- reupload?

[Derek Schuff, 2013-09-09 23:24:04]

On 2013/09/09 23:21:01, jvoung (cr) wrote:
> "error: old chunk mismatch" -- reupload?

done

[jvoung (off chromium), 2013-09-09 23:47:52]

https://codereview.chromium.org/23753003/diff/14001/lib/Bitcode/NaCl/Reader/N...
File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right):

https://codereview.chromium.org/23753003/diff/14001/lib/Bitcode/NaCl/Reader/N...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1761: if (AcceptHeader())
Not quite sure the difference in when "return Error(...)" happens.    Doesn't it
just set a field that will be read later (so doesn't matter too much when it
happens)?

You also have to return a value AcceptHeader() returns no error?

https://codereview.chromium.org/23753003/diff/14001/tools/pnacl-llc/SRPCStrea...
File tools/pnacl-llc/SRPCStreamer.cpp (right):

https://codereview.chromium.org/23753003/diff/14001/tools/pnacl-llc/SRPCStrea...
tools/pnacl-llc/SRPCStreamer.cpp:122: if (__sync_fetch_and_add(&Error, 0))
return 0; // Atomic read.
Hmm hopefully an add of 0 doesn't get optimized out =)

https://codereview.chromium.org/23753003/diff/14001/tools/pnacl-llc/SRPCStrea...
tools/pnacl-llc/SRPCStreamer.cpp:129: __sync_synchronize();
Does pthread_join form a happens-before edge?  Do we need the
__sync_synchronize?

[Derek Schuff, 2013-09-10 18:40:22]

https://codereview.chromium.org/23753003/diff/14001/lib/Bitcode/NaCl/Reader/N...
File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right):

https://codereview.chromium.org/23753003/diff/14001/lib/Bitcode/NaCl/Reader/N...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1761: if (AcceptHeader())
On 2013/09/09 23:47:53, jvoung (cr) wrote:
> Not quite sure the difference in when "return Error(...)" happens.    Doesn't
it
> just set a field that will be read later (so doesn't matter too much when it
> happens)?
> 
> You also have to return a value AcceptHeader() returns no error?

Error just sets a field that will be read later. But Unsupported was only set
when the header is valid but the version is unsupported, so as it was,
InitLazyStream was setting the error on line 1786 but ParseBitcodeInto was
overwriting it. Since InitStream and its callees still set the error, I've kept
this, but also improved the error messages they set.

[jvoung (off chromium), 2013-09-10 20:39:55]

https://codereview.chromium.org/23753003/diff/21001/lib/Bitcode/NaCl/Reader/N...
File lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp (right):

https://codereview.chromium.org/23753003/diff/21001/lib/Bitcode/NaCl/Reader/N...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:148: "finalized using
pnacl-finalize";
add an end parenthesis to match the beginning "(to run..."

https://codereview.chromium.org/23753003/diff/21001/lib/Bitcode/NaCl/Reader/N...
File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right):

https://codereview.chromium.org/23753003/diff/21001/lib/Bitcode/NaCl/Reader/N...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1756: return Error("Invalid PNaCl
bitcode header");
A bit difficult to figure out which error message gets through.

Doesn't this early return of Error("...") mean that we never get to the
Header.Unsupported() call on line 1762, so the UnsupportedMessage that was set
from NaClBitcodeHeader::ReadPrefix/Read won't be read?

[Derek Schuff, 2013-09-13 19:53:36]

Also added Karl to look at the bitcode reader changes.

https://codereview.chromium.org/23753003/diff/21001/lib/Bitcode/NaCl/Reader/N...
File lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp (right):

https://codereview.chromium.org/23753003/diff/21001/lib/Bitcode/NaCl/Reader/N...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:148: "finalized using
pnacl-finalize";
On 2013/09/10 20:39:56, jvoung (cr) wrote:
> add an end parenthesis to match the beginning "(to run..."
> 

Done.

https://codereview.chromium.org/23753003/diff/21001/lib/Bitcode/NaCl/Reader/N...
File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right):

https://codereview.chromium.org/23753003/diff/21001/lib/Bitcode/NaCl/Reader/N...
lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1756: return Error("Invalid PNaCl
bitcode header");
On 2013/09/10 20:39:56, jvoung (cr) wrote:
> A bit difficult to figure out which error message gets through.
Agreed.
I added some comments and made the error handling a bit more explicit in the
bitcode reader and header.

[Karl, 2013-09-13 20:13:14]

Bitcode reader changes look good to me.

[jvoung (off chromium), 2013-09-13 20:20:23]

lgtm

https://codereview.chromium.org/23753003/diff/33001/lib/Bitcode/NaCl/Reader/N...
File lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp (right):

https://codereview.chromium.org/23753003/diff/33001/lib/Bitcode/NaCl/Reader/N...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:204: // ReadPrefix sets
UnsupportedMessage but readBytes does not
readBytes could happen first w/ a chance to set the message, then ReadPrefix()
w/out needing to set the message ?

[Derek Schuff, 2013-09-13 20:33:14]

https://codereview.chromium.org/23753003/diff/33001/lib/Bitcode/NaCl/Reader/N...
File lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp (right):

https://codereview.chromium.org/23753003/diff/33001/lib/Bitcode/NaCl/Reader/N...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:204: // ReadPrefix sets
UnsupportedMessage but readBytes does not
On 2013/09/13 20:20:23, jvoung (cr) wrote:
> readBytes could happen first w/ a chance to set the message, then ReadPrefix()
> w/out needing to set the message ?
I mean that if If readBytes fails, ReadPrefix will not run and the message will
not be set. So it is set explicitly here.

[jvoung (off chromium), 2013-09-13 20:35:25]

https://codereview.chromium.org/23753003/diff/33001/lib/Bitcode/NaCl/Reader/N...
File lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp (right):

https://codereview.chromium.org/23753003/diff/33001/lib/Bitcode/NaCl/Reader/N...
lib/Bitcode/NaCl/Reader/NaClBitcodeHeader.cpp:204: // ReadPrefix sets
UnsupportedMessage but readBytes does not
On 2013/09/13 20:33:15, Derek Schuff wrote:
> On 2013/09/13 20:20:23, jvoung (cr) wrote:
> > readBytes could happen first w/ a chance to set the message, then
ReadPrefix()
> > w/out needing to set the message ?
> I mean that if If readBytes fails, ReadPrefix will not run and the message
will
> not be set. So it is set explicitly here.

but is there a case where readBytes doesn't fail, but ReadPrefix does fail -- in
that case, does the "return UnsupportedError" end up giving us a less detailed
error message?

[Derek Schuff, 2013-09-13 21:29:15]

Committed patchset #8 manually as rf8cb03f (presubmit successful).