Fix locking for printing error messages.

src/PNaClTranslator.cpp

 //===- subzero/src/PNaClTranslator.cpp - ICE from bitcode -----------------===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the PNaCl bitcode file to Ice, to machine code
@@ skipping 468 matching lines @@
   // Reports that there is no corresponding ICE type for LLVMTy, and
   // returns ICE::IceType_void.
   Ice::Type convertToIceTypeError(Type *LLVMTy);
 };
 
 bool TopLevelParser::ErrorAt(naclbitc::ErrorLevel Level, uint64_t Bit,
                              const std::string &Message) {
   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().

   return true;
 }
 
 void TopLevelParser::reportBadTypeIDAs(unsigned ID, const ExtendedType *Ty,
                                        ExtendedType::TypeKind WantedType) {
   std::string Buffer;
   raw_string_ostream StrBuf(Buffer);
   if (Ty == nullptr) {
     StrBuf << "Can't find extended type for type id: " << ID;
   } else {
@@ skipping 2508 matching lines @@
   }
   if (InputStreamFile.getBitcodeBytes().getExtent() % 4 != 0) {
     errs() << IRFilename
            << ": Bitcode stream should be a multiple of 4 bytes in length.\n";
     ErrorStatus.assign(EC_Bitcode);
     return;
   }
 }
 
 } // end of namespace Ice