Additional clean ups on errors in bitcode parsing.

Additional clean ups on errors in bitcode parsing.

Earlier CL's introduced the notion of ErrorAt to a bitcode parser.
However, a couple of parsers were not updated to use the new virtual,
resulting in bitposition being lost in error messages.

This CL also further consolidates error message formats, defining a
common naclbitc::ErrorAt static method to print the
standard error message prefix. That is, a label (i.e. "Warning", "Error" or
"Fatal") followed by the bit position in the file, followed by a
colon.

It also now defines a single virtual method ErrorAt in bitcode parsers, for which all other forms of error reporting are fed through. This means that derived classes must only redefine a single virtual to change how errors are reported in bitcode parsers.

BUG=None
R=jvoung@chromium.org

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

[Karl, 2015-03-06 21:23:14]

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

[jvoung (off chromium), 2015-03-06 22:30:28]

misc: remember to update COMPONENT_REVISIONS after landing too

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
File include/llvm/Bitcode/NaCl/NaClBitcodeParser.h (right):

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClBitcodeParser.h:491: //    the method Error() is
overridden in the derived class, and
Error() is no longer overridden (changed at some point?)?

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:32: /// Returns the Bit as a
Byte:BitInByte string. MinByteWidth is the
There's no MinByteWidth argument for this function.

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:347: /// MinByteWidth is the
minimum number of characters to print out
There's no MinByteWidth parameter here anymore.

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
File include/llvm/Bitcode/NaCl/NaClObjDumpStream.h (right):

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClObjDumpStream.h:937: /// to fill in the
corresponding bit to associate with the error
On second read, I'm not sure you need to call out all the callers/users of this.
It seems reasonable for this just to be "Changes the internal state, to assume
one is processing a record at the given bit.". Then you don't need to update the
comments based on new usage.

WDYT?

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
File lib/Bitcode/NaCl/Analysis/NaClBitcodeAnalyzer.cpp (right):

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
lib/Bitcode/NaCl/Analysis/NaClBitcodeAnalyzer.cpp:42: }
namespace ends down here instead?

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
File lib/Bitcode/NaCl/Analysis/NaClCompress.cpp (left):

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
lib/Bitcode/NaCl/Analysis/NaClCompress.cpp:1264: 
wow so many =)

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
File lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp (right):

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:594: if (Level == naclbitc::Fatal)
How come Fatal is called out specially?

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
File lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp (right):

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp:257: // lets the caller fill in
the rest of the error, we can't stop
we can't stop "the" executable ?

[Karl, 2015-03-06 22:54:21]

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
File include/llvm/Bitcode/NaCl/NaClBitcodeParser.h (right):

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClBitcodeParser.h:491: //    the method Error() is
overridden in the derived class, and
On 2015/03/06 22:30:27, jvoung wrote:
> Error() is no longer overridden (changed at some point?)?

Fixed to refer to the right method.

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:32: /// Returns the Bit as a
Byte:BitInByte string. MinByteWidth is the
On 2015/03/06 22:30:27, jvoung wrote:
> There's no MinByteWidth argument for this function.

Good catch. Removing the sentence.

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:347: /// MinByteWidth is the
minimum number of characters to print out
On 2015/03/06 22:30:27, jvoung wrote:
> There's no MinByteWidth parameter here anymore.

Removed the sentence.

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
File include/llvm/Bitcode/NaCl/NaClObjDumpStream.h (right):

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClObjDumpStream.h:937: /// to fill in the
corresponding bit to associate with the error
On 2015/03/06 22:30:27, jvoung wrote:
> On second read, I'm not sure you need to call out all the callers/users of
this.
> It seems reasonable for this just to be "Changes the internal state, to assume
> one is processing a record at the given bit.". Then you don't need to update
the
> comments based on new usage.
> 
> WDYT?

Sounds good. Removing sentence.

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
File lib/Bitcode/NaCl/Analysis/NaClBitcodeAnalyzer.cpp (right):

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
lib/Bitcode/NaCl/Analysis/NaClBitcodeAnalyzer.cpp:42: }
On 2015/03/06 22:30:27, jvoung wrote:
> namespace ends down here instead?

Oops, fixing.

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
lib/Bitcode/NaCl/Analysis/NaClBitcodeAnalyzer.cpp:407: return ::Error("Bitcode
stream should be a multiple of 4 bytes in length");
Also removed the "::" because we aren't inside a bitcode parser (same with cases
below).

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
File lib/Bitcode/NaCl/Analysis/NaClCompress.cpp (left):

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
lib/Bitcode/NaCl/Analysis/NaClCompress.cpp:1264: 
On 2015/03/06 22:30:28, jvoung wrote:
> wow so many =)

Acknowledged.

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
File lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp (right):

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:594: if (Level == naclbitc::Fatal)
On 2015/03/06 22:30:28, jvoung wrote:
> How come Fatal is called out specially?

Under the new scheme, within a bitcode parser, all error reporting (including
fatal errors) are passed through this single virtual.

However, for a dump stream, ErrorAt and FatalAt work differently.

That is, to make reporting (non-fatal) errors, ErrorAt only adds the
"Error(...): " prefix, and returns the stream to write the rest of the message
to. It is then the responsibility of the calling routine to fill in the rest of
the message. This makes error reporting easier.

For fatal errors, just printing out the prefix "Fatal(...): " is not enough. It
must also terminate the executable. Hence, the obj stream expects the error
message as an argument. This allows method FatalAt to do the necessary shutdown
of the executable.

I did toy with the idea of making the Error and Fatal methods behave the same,
requiring a second call (or special fatal "eoln" token) to cause the shutdown
separately. However, I did not implement it (yet).

WDYT about this alternative approach? The main advantage of the new approach
would be simplified errors (one can just redirect the rest of the error message
to the stream generated by Error and Fatal, rather than having to generate it
into a raw_string_ostream). The down side is that you must remember to make the
second shutdown call, or the fatal error will not be handled correctly.

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
File lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp (right):

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp:257: // lets the caller fill in
the rest of the error, we can't stop
On 2015/03/06 22:30:28, jvoung wrote:
> we can't stop "the" executable ?

Done.

[jvoung (off chromium), 2015-03-07 00:04:35]

reupload also?

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
File include/llvm/Bitcode/NaCl/NaClObjDumpStream.h (right):

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClObjDumpStream.h:851: void Fatal(const std::string
&Message) {
Is this method still used?

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
File lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp (right):

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:594: if (Level == naclbitc::Fatal)
On 2015/03/06 22:54:20, Karl wrote:
> On 2015/03/06 22:30:28, jvoung wrote:
> > How come Fatal is called out specially?
> 
> Under the new scheme, within a bitcode parser, all error reporting (including
> fatal errors) are passed through this single virtual.
> 
> However, for a dump stream, ErrorAt and FatalAt work differently.
> 
> That is, to make reporting (non-fatal) errors, ErrorAt only adds the
> "Error(...): " prefix, and returns the stream to write the rest of the message
> to. It is then the responsibility of the calling routine to fill in the rest
of
> the message. This makes error reporting easier.
> 
> For fatal errors, just printing out the prefix "Fatal(...): " is not enough.
It
> must also terminate the executable. Hence, the obj stream expects the error
> message as an argument. This allows method FatalAt to do the necessary
shutdown
> of the executable.
> 
> I did toy with the idea of making the Error and Fatal methods behave the same,
> requiring a second call (or special fatal "eoln" token) to cause the shutdown
> separately. However, I did not implement it (yet).
> 
> WDYT about this alternative approach? The main advantage of the new approach
> would be simplified errors (one can just redirect the rest of the error
message
> to the stream generated by Error and Fatal, rather than having to generate it
> into a raw_string_ostream). The down side is that you must remember to make
the
> second shutdown call, or the fatal error will not be handled correctly.

I think if (a) only this method needs to worry about remembering the second
shutdown call and (b) the # of methods goes down or are simplified, then the
alternative approach would be a win. However, there is currently a second
FatalAt() method that also dumps a record and I'm not sure how that would fit
in... so (b) might not happen.

Otherwise it would be good to leave a comment about why it's a special case.

[Karl, 2015-03-09 18:10:54]

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
File include/llvm/Bitcode/NaCl/NaClObjDumpStream.h (right):

https://codereview.chromium.org/986453002/diff/80001/include/llvm/Bitcode/NaC...
include/llvm/Bitcode/NaCl/NaClObjDumpStream.h:851: void Fatal(const std::string
&Message) {
On 2015/03/07 00:04:35, jvoung wrote:
> Is this method still used?
> 

Removed.

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
File lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp (right):

https://codereview.chromium.org/986453002/diff/80001/lib/Bitcode/NaCl/Analysi...
lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:594: if (Level == naclbitc::Fatal)
On 2015/03/07 00:04:35, jvoung wrote:
> On 2015/03/06 22:54:20, Karl wrote:
> > On 2015/03/06 22:30:28, jvoung wrote:
> > > How come Fatal is called out specially?
> > 
> > Under the new scheme, within a bitcode parser, all error reporting
(including
> > fatal errors) are passed through this single virtual.
> > 
> > However, for a dump stream, ErrorAt and FatalAt work differently.
> > 
> > That is, to make reporting (non-fatal) errors, ErrorAt only adds the
> > "Error(...): " prefix, and returns the stream to write the rest of the
message
> > to. It is then the responsibility of the calling routine to fill in the rest
> of
> > the message. This makes error reporting easier.
> > 
> > For fatal errors, just printing out the prefix "Fatal(...): " is not enough.
> It
> > must also terminate the executable. Hence, the obj stream expects the error
> > message as an argument. This allows method FatalAt to do the necessary
> shutdown
> > of the executable.
> > 
> > I did toy with the idea of making the Error and Fatal methods behave the
same,
> > requiring a second call (or special fatal "eoln" token) to cause the
shutdown
> > separately. However, I did not implement it (yet).
> > 
> > WDYT about this alternative approach? The main advantage of the new approach
> > would be simplified errors (one can just redirect the rest of the error
> message
> > to the stream generated by Error and Fatal, rather than having to generate
it
> > into a raw_string_ostream). The down side is that you must remember to make
> the
> > second shutdown call, or the fatal error will not be handled correctly.
> 
> I think if (a) only this method needs to worry about remembering the second
> shutdown call and (b) the # of methods goes down or are simplified, then the
> alternative approach would be a win. However, there is currently a second
> FatalAt() method that also dumps a record and I'm not sure how that would fit
> in... so (b) might not happen.
> 
> Otherwise it would be good to leave a comment about why it's a special case.

Ok. I was able to clean things up and remove both Fatal and FatalAt from an
NaClObjStream class. All errors now go through the single ErrorAt method. For
Fatal errors you call ErrorAt(naclbitc::Fatal, ....) followed by a call to
FlushThenQuit. This pattern is only needed in one spot (NaClObjDump::ErrorAt).

https://codereview.chromium.org/986453002/diff/100001/lib/Bitcode/NaCl/Analys...
File lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp (right):

https://codereview.chromium.org/986453002/diff/100001/lib/Bitcode/NaCl/Analys...
lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:594: ObjDump.ErrorAt(Level, Bit) <<
Message << "\n";
Here is where FlushThenQuit is called.

[jvoung (off chromium), 2015-03-09 18:50:23]

https://codereview.chromium.org/986453002/diff/100001/lib/Bitcode/NaCl/Analys...
File lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp (right):

https://codereview.chromium.org/986453002/diff/100001/lib/Bitcode/NaCl/Analys...
lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:594: ObjDump.ErrorAt(Level, Bit) <<
Message << "\n";
On 2015/03/09 18:10:54, Karl wrote:
> Here is where FlushThenQuit is called.

Interesting, there used to be a special case for ObjDump.Fatal where Message is
empty, but I'm not sure where that matters.

https://codereview.chromium.org/986453002/diff/120001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitcodeParser.h (right):

https://codereview.chromium.org/986453002/diff/120001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeParser.h:493: //    calling base method
Error(), and followed by a second call to
Another mention of "base method Error()".

Probably don't need to be so specific about exactly where this is used. At a
high level, this is either (1) to make all error messages "always" go to the
given stream, or to (2) to temporarily make a single error message go to a
different stream (which requires returning the old stream so it can be
restored).

https://codereview.chromium.org/986453002/diff/120001/lib/Bitcode/NaCl/Analys...
File lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp (right):

https://codereview.chromium.org/986453002/diff/120001/lib/Bitcode/NaCl/Analys...
lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp:261: Flush();
How does the caller know that the MaxErrors is hit? Otherwise will it just take
the returned raw_ostream and continue to write to it?

[Karl, 2015-03-09 19:49:25]

https://codereview.chromium.org/986453002/diff/100001/lib/Bitcode/NaCl/Analys...
File lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp (right):

https://codereview.chromium.org/986453002/diff/100001/lib/Bitcode/NaCl/Analys...
lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:594: ObjDump.ErrorAt(Level, Bit) <<
Message << "\n";
On 2015/03/09 18:50:22, jvoung wrote:
> On 2015/03/09 18:10:54, Karl wrote:
> > Here is where FlushThenQuit is called.
> 
> Interesting, there used to be a special case for ObjDump.Fatal where Message
is
> empty, but I'm not sure where that matters.

There was. See GetFunctionType on line 660. However, the base class
NaClBitcodeParser now has a non-virtual Fatal() method that converts to a call
to ErrorAt (where all errors are now piped through).

https://codereview.chromium.org/986453002/diff/120001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitcodeParser.h (right):

https://codereview.chromium.org/986453002/diff/120001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitcodeParser.h:493: //    calling base method
Error(), and followed by a second call to
On 2015/03/09 18:50:23, jvoung wrote:
> Another mention of "base method Error()".
> 
> Probably don't need to be so specific about exactly where this is used. At a
> high level, this is either (1) to make all error messages "always" go to the
> given stream, or to (2) to temporarily make a single error message go to a
> different stream (which requires returning the old stream so it can be
> restored).

Done.

https://codereview.chromium.org/986453002/diff/120001/lib/Bitcode/NaCl/Analys...
File lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp (right):

https://codereview.chromium.org/986453002/diff/120001/lib/Bitcode/NaCl/Analys...
lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp:261: Flush();
On 2015/03/09 18:50:23, jvoung wrote:
> How does the caller know that the MaxErrors is hit? Otherwise will it just
take
> the returned raw_ostream and continue to write to it?

I guess my comment wasn't clear enough. I thought that he sentence "Instead, we
let Flush empty the buffers, and print out existing errors, and then exit the
executable." was explaining this. That is why we call Flush.

Note: I fixed this sentence to: "Instead, we let Flush empty the buffers, print
out reported errors, and then exits the executable."

[jvoung (off chromium), 2015-03-09 20:03:01]

lgtm

https://codereview.chromium.org/986453002/diff/120001/lib/Bitcode/NaCl/Analys...
File lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp (right):

https://codereview.chromium.org/986453002/diff/120001/lib/Bitcode/NaCl/Analys...
lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp:261: Flush();
On 2015/03/09 19:49:25, Karl wrote:
> On 2015/03/09 18:50:23, jvoung wrote:
> > How does the caller know that the MaxErrors is hit? Otherwise will it just
> take
> > the returned raw_ostream and continue to write to it?
> 
> I guess my comment wasn't clear enough. I thought that he sentence "Instead,
we
> let Flush empty the buffers, and print out existing errors, and then exit the
> executable." was explaining this. That is why we call Flush.
> 
> Note: I fixed this sentence to: "Instead, we let Flush empty the buffers,
print
> out reported errors, and then exits the executable."

Okay, I think I understand now. I didn't look at Flush() closely enough. Flush()
itself does the check again and exits the executable.

I guess I was confused by "we can't stop the executable *at this point*". I
wasn't sure what the later point would be, which made me think that this would
return to the caller, and the caller would exit (but the caller wouldn't know!).
What you meant by a different point in time is the end of Flush(). It wasn't
clear to me that Flush would exit, so how about put an llvm_unreachable() after
Flush()?

"exits" ---back-to---> "exit" ?

Otherwise, would it also help to not-really talk about what can't be done and
just say: "If we've hit the maximum number of errors, let Flush empty the
buffers, print out reported errors, and then exit the executable."?

[Karl, 2015-03-12 16:31:40]

Committed patchset #8 (id:140001) manually as
090ce49bad69b6bcd22c1d1ba1d3eb679ad367a4 (presubmit successful).

[Karl, 2015-03-12 16:32:26]

https://codereview.chromium.org/986453002/diff/120001/lib/Bitcode/NaCl/Analys...
File lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp (right):

https://codereview.chromium.org/986453002/diff/120001/lib/Bitcode/NaCl/Analys...
lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp:261: Flush();
On 2015/03/09 20:03:01, jvoung wrote:
> On 2015/03/09 19:49:25, Karl wrote:
> > On 2015/03/09 18:50:23, jvoung wrote:
> > > How does the caller know that the MaxErrors is hit? Otherwise will it just
> > take
> > > the returned raw_ostream and continue to write to it?
> > 
> > I guess my comment wasn't clear enough. I thought that he sentence "Instead,
> we
> > let Flush empty the buffers, and print out existing errors, and then exit
the
> > executable." was explaining this. That is why we call Flush.
> > 
> > Note: I fixed this sentence to: "Instead, we let Flush empty the buffers,
> print
> > out reported errors, and then exits the executable."
> 
> Okay, I think I understand now. I didn't look at Flush() closely enough.
Flush()
> itself does the check again and exits the executable.
> 
> I guess I was confused by "we can't stop the executable *at this point*". I
> wasn't sure what the later point would be, which made me think that this would
> return to the caller, and the caller would exit (but the caller wouldn't
know!).
> What you meant by a different point in time is the end of Flush(). It wasn't
> clear to me that Flush would exit, so how about put an llvm_unreachable()
after
> Flush()?
> 
> "exits" ---back-to---> "exit" ?
> 
> Otherwise, would it also help to not-really talk about what can't be done and
> just say: "If we've hit the maximum number of errors, let Flush empty the
> buffers, print out reported errors, and then exit the executable."?

Followed your suggestion and used your suggested change. Also added
llvm_unreachable.