[llvm-commits] [llvm] r75027 - in /llvm/trunk: include/llvm/CodeGen/ include/llvm/Support/ lib/Target/Alpha/ lib/Target/Alpha/AsmPrinter/ lib/Target/IA64/ lib/Target/IA64/AsmPrinter/ lib/Target/MSP430/ lib/Target/Mips/AsmPrinter/ lib/Target/PIC16/ lib/Target/Sparc/AsmPrinter/ lib/Target/X86/ lib/Target/XCore/ utils/TableGen/

[Török Edwin]

On 2009-07-08 23:00, Chris Lattner wrote:
> On Jul 8, 2009, at 12:04 PM, Torok Edwin wrote:
>   
>> Log:
>> Convert more abort() calls to llvm_report_error().
>> Also remove trailing semicolon.
>>     
>
> Ok.
>   

Thanks for reviewing my commit, looks like I got it wrong in many places.
At least error handling should be more consistent now, asserts that are
meant to be assert are ;)

>   
>> +++ llvm/trunk/include/llvm/CodeGen/MachineCodeEmitter.h Wed Jul  8  
>> 14:04:27 2009
>> @@ -18,6 +18,8 @@
>> #define LLVM_CODEGEN_MACHINECODEEMITTER_H
>>
>> #include "llvm/Support/DataTypes.h"
>> +#include "llvm/Support/ErrorHandling.h"
>> +#include "llvm/Support/raw_ostream.h"
>>     
>
> Please don't include headers in other headers unless you cannot avoid  
> it.
>
>   

I included it because then I don't need to include it in all the
*ISelDAGToDAG.cpp files.
I removed it now, and included the necessary headers in each .cpp file.

>   
>> +++ llvm/trunk/lib/Target/XCore/XCoreAsmPrinter.cpp Wed Jul  8  
>> 14:04:27 2009
>> @@ -204,14 +204,11 @@
>>     case GlobalValue::PrivateLinkage:
>>       break;
>>     case GlobalValue::GhostLinkage:
>> -      cerr << "Should not have any unmaterialized functions!\n";
>> -      abort();
>> +      llvm_report_error("Should not have any unmaterialized  
>> functions!");
>>     case GlobalValue::DLLImportLinkage:
>> -      cerr << "DLLImport linkage is not supported by this target!\n";
>> -      abort();
>> +      llvm_report_error("DLLImport linkage is not supported by this  
>> target!");
>>     case GlobalValue::DLLExportLinkage:
>> -      cerr << "DLLExport linkage is not supported by this target!\n";
>> -      abort();
>> +      llvm_report_error("DLLExport linkage is not supported by this  
>> target!");
>>     
>
> assertions. :)
>   

Thanks, I changed them. Quite a lot of copy+pasting between the targets,
isn't it?

>> +++ llvm/trunk/lib/Target/XCore/XCoreISelLowering.cpp Wed Jul  8  
>> 14:04:27 2009
>> @@ -270,9 +270,8 @@
>>   }
>>   const Type *Ty = cast<PointerType>(GV->getType())->getElementType();
>>   if (!Ty->isSized() || isZeroLengthArray(Ty)) {
>> -    cerr << "Size of thread local object " << GVar->getName()
>> -         << " is unknown\n";
>> -    abort();
>> +    llvm_report_error("Size of thread local object " + GVar- 
>>     
>>> getName()
>>>       
>> +                      + " is unknown");
>>     
>
> This should be an assertion, does the verifier catch this?  If not, it  
> should.
>   

I will check this tomorrow.

On 2009-07-08 22:50, Chris Lattner wrote:
> On Jul 8, 2009, at 11:01 AM, Torok Edwin wrote:
>   
>> URL: http://llvm.org/viewvc/llvm-project?rev=75018&view=rev
>> Log:
>> Start converting to new error handling API.
>> cerr+abort -> llvm_report_error
>> assert(0)+abort -> LLVM_UNREACHABLE (assert(0)+llvm_unreachable->  
>> abort() included)
>>     
>
> Thanks Edwin, most of this looks great.  Several of these below are  
> assertions that have a message: the preferred style is something like:
>
> #ifndef NDEBUG
>    cerr << "WHATEVER\n";
> #endif
>    unreachable();
>   

Ok, I changed LLVM_UNREACHABLE to do that.
I also used this style when a more complicated message was printed.

>
>> +++ llvm/trunk/lib/Target/X86/X86ISelLowering.cpp Wed Jul  8  
>> 13:01:40 2009
>> @@ -33,6 +33,7 @@
>> #include "llvm/CodeGen/PseudoSourceValue.h"
>> #include "llvm/Support/MathExtras.h"
>> #include "llvm/Support/Debug.h"
>> +#include "llvm/Support/ErrorHandling.h"
>> #include "llvm/Target/TargetOptions.h"
>> #include "llvm/ADT/SmallSet.h"
>> #include "llvm/ADT/StringExtras.h"
>> @@ -6054,8 +6055,7 @@
>>   SDValue SrcPtr = Op.getOperand(1);
>>   SDValue SrcSV = Op.getOperand(2);
>>
>> -  assert(0 && "VAArgInst is not yet implemented for x86-64!");
>> -  abort();
>> +  LLVM_UNREACHABLE("VAArgInst is not yet implemented for x86-64!");
>>   return SDValue();
>>     
>
> This should be a report_error.
>   

Ok :)

> The verifier really should abort when the action is  
> AbortProcessAction.  Clients that want to use the verifier in other  
> modes can pick a different reaction.
>   

Ok, I restored the abort there, and added a comment explaining why.

Best regards,
--Edwin