[llvm-commits] Error-handling fixes for the x86 disassembler

[Chris Lattner]

On Mar 31, 2010, at 5:00 PM, Sean Callanan wrote:

> Eric, Daniel,
> 
> thanks for your comments.  I've updated the patch to reflect your concerns:
> 
> - I return 0 or -1 in all cases, and never just plain return.  (0 is sometimes the right thing to return, given the return type.)

Thanks.

> - All fprintf(stderr)s are #ifdef'd out pending a proper error-reporting API, which I'll propose separately

Please don't check in #if 0'd code.  The idiom that we use in other apis that can fail is something like this:

int dosomething_that_could_fail(..., std::string *Error) {

...
  // Oh crap an error happened!
  if (Error) *Error = "whatever";
  return -1;
}

Then clients can handle this or not with code like:
std::string ErrorInfo;
if (dosomething_that_could_fail(..., &ErrorInfo) == -1)
  print(ErrorInfo); // or whatever.

However, I don't see how error messages from the disassembler are going to be very useful.  Errors like:

Corrupt table!  Unknown modrm_type
Cannot have Mod = 0b11 and a SIB byte
Expected a REG or R/M encoding in fixupReg
No modifier but an operand expects one.

Are not going to mean anything to anyone.  I don't think there is such thing as a useful error message that can come out of a disassembler other than "unrecognized instruction".  Given this, I don't see the value in an error reporting api at all here.


> 	The error macro is useful because it uses __FILE__ and __LINE__.

You don't want to report FILE and LINE through an error reporting API.  If you want this for debugging,
then just make these things abort when in debug mode or something.

> I think we ought to discuss a more general error-retrieval API on llvmdev, but I don't want to backpack this patch, which needs to be resolved pretty quickly, onto that discussion.

This patch looks ok to me once we resolve the value of the error message reporting stuff.  I think you should just rip it out or turn it into asserts (which are disabled in production mode).

-Chris