Fix error reporting in the PNaCl bitcode reader.

tools/pnacl-llc/pnacl-llc.cpp

Index: tools/pnacl-llc/pnacl-llc.cpp
diff --git a/tools/pnacl-llc/pnacl-llc.cpp b/tools/pnacl-llc/pnacl-llc.cpp
index 5aefdf4e001f1716fb68a2658873b66152d31714..f958db42da3981efe643952f87e6c875174fd43e 100644
--- a/tools/pnacl-llc/pnacl-llc.cpp
+++ b/tools/pnacl-llc/pnacl-llc.cpp
@@ -90,6 +90,12 @@ InputFilename(cl::Positional, cl::desc("<input bitcode>"), cl::init("-"));
 static cl::opt<std::string>
 OutputFilename("o", cl::desc("Output filename"), cl::value_desc("filename"));
 
+static cl::opt<bool>
+VerboseErrors(
+    "verbose-parse-errors",
+    cl::desc("Print out more descriptive PNaCl bitcode parse errors"),
+    cl::init(false));
+
 // Using bitcode streaming allows compilation of one function at a time. This
 // allows earlier functions to be compiled before later functions are read from
 // the bitcode but of course means no whole-module optimizations. This means
@@ -326,9 +332,11 @@ static Module* getModule(StringRef ProgramName, LLVMContext &Context,
   if (LazyBitcode) {
     std::string StrError;
     if (InputFileFormat == PNaClFormat) {
+      raw_ostream *Verbose = VerboseErrors ? &errs() : nullptr;

[jvoung (off chromium), 2014/12/03 19:43:46]

Now that this is a stream, __native_client__/pnacl-llc in the browser could
potentially have a raw_string_ostream, and concat/prepend that to the
Err.getMessage below. On the other hand, maybe the errors are sufficient and
nullptr is fine.

[Karl, 2014/12/03 20:53:44]

Went ahead and added verbose errors if command line argument says so.

[jvoung (off chromium), 2014/12/03 22:11:13]

What I meant was why not pass in the VerboseStrm always? There's no way to set
this particular commandline flag when loading a pexe in the browser. Only a
subset of flags are set in the browser e.g., O0 vs O2 via the NMF, or when the
browser detects that the machine has SSE4 it'll set --mattr=...).  Then we get a
tad more info when people copy-paste whatever shows up in the JS console. 
Otherwise, this is "dead code" but not in the sense that the compiler can
optimize it out (vs what you had in patch set 2, where you hardcoded it to
false... well, potentially optimized out).

That said, probably the more common type of problems we'll run into are when we
extend the bitcode format and the developer is using an old browser w/ a new
pexe. In that case, we could be more careful bumping the bitcode version or
feature vector, so that the error message is indicated by the header check
instead of deep down somewhere having the reader says some error about an
invalid record.

[Karl, 2014/12/03 22:28:51]

Ok. I'll make it be always when in __native_client__. I double checked the code,
and it will (correctly) add verbose errors corresponding to bad header (and then
return) if there are any problem. Hence, the common type of problem will be
reported in this case.

       M = getNaClStreamedBitcodeModule(
           InputFilename,
-          new ThreadedStreamingCache(StreamingObject), Context, &StrError);
+          new ThreadedStreamingCache(StreamingObject), Context, Verbose,
+          &StrError);
     } else if (InputFileFormat == LLVMFormat) {
       M = getStreamedBitcodeModule(
           InputFilename,
@@ -344,7 +352,9 @@ static Module* getModule(StringRef ProgramName, LLVMContext &Context,
 #else
     // Parses binary bitcode as well as textual assembly
     // (so pulls in more code into pnacl-llc).
-    M = NaClParseIRFile(InputFilename, InputFileFormat, Err, Context);
+    raw_ostream *Verbose = VerboseErrors ? &errs() : nullptr;
+    M = NaClParseIRFile(InputFilename, InputFileFormat, Err, Verbose,
+                        Context);
 #endif
   }
   if (!M) {