Fix locking for printing error messages.

src/PNaClTranslator.cpp

Index: src/PNaClTranslator.cpp
diff --git a/src/PNaClTranslator.cpp b/src/PNaClTranslator.cpp
index 35cdcf7ab6a81134edf4129eec2767de8f9dcd9e..ce462396537e1990410eae780ab88bf4289e49db 100644
--- a/src/PNaClTranslator.cpp
+++ b/src/PNaClTranslator.cpp
@@ -486,10 +486,12 @@ bool TopLevelParser::ErrorAt(naclbitc::ErrorLevel Level, uint64_t Bit,
   ErrorStatus.assign(Ice::EC_Bitcode);
   ++NumErrors;
   Ice::GlobalContext *Context = Translator.getContext();
-  Ice::OstreamLocker L(Context);
-  raw_ostream &OldErrStream = setErrStream(Context->getStrDump());
-  NaClBitcodeParser::ErrorAt(Level, Bit, Message);
-  setErrStream(OldErrStream);
+  { // Lock while printing out error message.
+    Ice::OstreamLocker L(Context);
+    raw_ostream &OldErrStream = setErrStream(Context->getStrDump());
+    NaClBitcodeParser::ErrorAt(Level, Bit, Message);
+    setErrStream(OldErrStream);
+  }
   if (Level >= naclbitc::Error &&
       !Translator.getFlags().getAllowErrorRecovery())
     Fatal();

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

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]

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]

Ok right, because this call to Fatal() has no parameters.
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().