Fix locking for printing error messages.

Fix locking for printing error messages.

The method TopLevelParser::ErrorAt applies a lock to print the error
message. Unfortunately, it keeps the lock longer than necessary,
resulting in deadlock (on following fatal message) if error recovery
is not allowed.

Fixed by limiting scope of lock to only apply to the printing of the
error message.

Modified ClFlags to allow a "reset", and made ClFlags modifiable
by bitcode munge tests. This allowed us to test this problem as
a unit test.

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

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

[Karl, 2015-04-16 17:06:29]

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

[jvoung (off chromium), 2015-04-16 19:11:52]

Thanks Karl! Mostly looks good but I had a question.

https://codereview.chromium.org/1091023002/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1091023002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:497: Fatal();
I was wondering why not just report_fatal_error here. Does the error message get
printed twice?

=====

Separately, I wasn't sure what we should do for the browser. Right now the
browser will just get "Some error occurred", if the return status shows an
error:
https://chromium.googlesource.com/native_client/pnacl-subzero/+/master/src/Ic...

stdout/stderr do not get redirected to the JS console or the NaCl modules
"lastError" JS attribute
(https://developer.chrome.com/native-client/devguide/coding/progress-events#th...)
so it's pretty much just lost (long discussion here
https://code.google.com/p/chromium/issues/detail?id=406558#c2)

With LLVM, it would set up a report_fatal_error(str) handler and capture the
given "str" and return that, but that has some down sides too (str is a bit more
limited). So... what should Subzero do? Should we make a version of
Ostream/StrDump which stores the streamed contents into a string in-addition-to
writing to stdout/err, and return that string content to the browser?

[Karl, 2015-04-16 19:58:51]

https://codereview.chromium.org/1091023002/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1091023002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:497: Fatal();
On 2015/04/16 19:11:52, jvoung wrote:
> I was wondering why not just report_fatal_error here. Does the error message
get
> printed twice?
> 
> =====
> 
> Separately, I wasn't sure what we should do for the browser. Right now the
> browser will just get "Some error occurred", if the return status shows an
> error:
>
https://chromium.googlesource.com/native_client/pnacl-subzero/+/master/src/Ic...
> 
> stdout/stderr do not get redirected to the JS console or the NaCl modules
> "lastError" JS attribute
>
(https://developer.chrome.com/native-client/devguide/coding/progress-events#th...)
> so it's pretty much just lost (long discussion here
> https://code.google.com/p/chromium/issues/detail?id=406558#c2)
> 
> With LLVM, it would set up a report_fatal_error(str) handler and capture the
> given "str" and return that, but that has some down sides too (str is a bit
more
> limited). So... what should Subzero do? Should we make a version of
> Ostream/StrDump which stores the streamed contents into a string
in-addition-to
> writing to stdout/err, and return that string content to the browser?

The point of calling fatal() is that it uses existing machinery to create and
format the error, and then comes back to ErrorAt to synchronize the output. We
could have done it here, I just wanted more consistency in the way messages are
generated.

It doesn't reprint the error message. It prints "Fatal error occurred!", and
then calls report_fatal_error("Unable to continue").

I think I can do the second issue by adding a command line flag that will
redirect the errors to a separate string stream. In addition, I need to make a
slight change to the NaClBitcodeParser to add a virtual method
ReportFatalError(message), which defaults to calling
report_fatal_error(message). I can then redirect the collected error messages in
an overridden virtual TopLevelParser::ReportFatalError(message).

I'll start working on the changes to llvm first.

[jvoung (off chromium), 2015-04-16 20:22:47]

LGTM

https://codereview.chromium.org/1091023002/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1091023002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:497: Fatal();
On 2015/04/16 19:58:51, Karl wrote:
> On 2015/04/16 19:11:52, jvoung wrote:
> > I was wondering why not just report_fatal_error here. Does the error message
> get
> > printed twice?
> > 
> > =====
> > 
> > Separately, I wasn't sure what we should do for the browser. Right now the
> > browser will just get "Some error occurred", if the return status shows an
> > error:
> >
>
https://chromium.googlesource.com/native_client/pnacl-subzero/+/master/src/Ic...
> > 
> > stdout/stderr do not get redirected to the JS console or the NaCl modules
> > "lastError" JS attribute
> >
>
(https://developer.chrome.com/native-client/devguide/coding/progress-events#th...)
> > so it's pretty much just lost (long discussion here
> > https://code.google.com/p/chromium/issues/detail?id=406558#c2)
> > 
> > With LLVM, it would set up a report_fatal_error(str) handler and capture the
> > given "str" and return that, but that has some down sides too (str is a bit
> more
> > limited). So... what should Subzero do? Should we make a version of
> > Ostream/StrDump which stores the streamed contents into a string
> in-addition-to
> > writing to stdout/err, and return that string content to the browser?
> 
> The point of calling fatal() is that it uses existing machinery to create and
> format the error, and then comes back to ErrorAt to synchronize the output. We
> could have done it here, I just wanted more consistency in the way messages
are
> generated.
> 
> It doesn't reprint the error message. It prints "Fatal error occurred!", and
> then calls report_fatal_error("Unable to continue").

Ok right, because this call to Fatal() has no parameters.



> I think I can do the second issue by adding a command line flag that will
> redirect the errors to a separate string stream. In addition, I need to make a
> slight change to the NaClBitcodeParser to add a virtual method
> ReportFatalError(message), which defaults to calling
> report_fatal_error(message). I can then redirect the collected error messages
in
> an overridden virtual TopLevelParser::ReportFatalError(message).
> 
> I'll start working on the changes to llvm first.


I don't quite understand how that would compare to some other options. There are
other ways to redirect the streams:

(a) We could set construct "LogStream = <whatever you want>; ...; new
GlobalContext(LogStream, ..." here, under the browser-main:

https://chromium.googlesource.com/native_client/pnacl-subzero/+/master/src/Ic....
In unittests, you could also set up the log stream/context however you like, but
I guess if you also wanted commandline lit tests that would be different and
would need a flag.

That would collect everything (which only works well if there isn't too much),
vs only what's passed to the ReportFatalError virtual?

(b) You can redirect report_fatal_error() to do what you want:
http://llvm.org/docs/doxygen/html/namespacellvm.html#a2aeb96bbf49b1dd8f8a6cf1....
 We may have to do that, if there are other report_fatal_error calls outside of
the parser. We can't keep the default behavior, which will terminate the whole
process. We have to wait until the browser has pulled the error message out. We
should, however, terminate translation/parser/etc threads -- just not the
browser communication thread.

(c) We also have to figure out what to do with errors elsewhere, like
IceCfg::setError(msg)/getError().

[Karl, 2015-04-16 20:33:00]

You make a good point about using a "LogStream" to put errors to, rather than
having the bitcode parser collect the error messages and then send them through
report_fatal_error.

After I check this CL in, I will look into building a log stream and put error
messages (at least from the parser) to this stream.

By default, I would make Context->getStrDump() and Context->getStrLog() return
the same stream. However, with a CL flag, keep these separate.

[Karl, 2015-04-16 20:50:48]

Committed patchset #2 (id:20001) manually as
187b3dfab5cca45e2af192bf7f5db09bcc156314 (presubmit successful).