[llvm-commits] [llvm] r49006 - in /llvm/trunk/lib: CodeGen/DwarfWriter.cpp CodeGen/LLVMTargetMachine.cpp CodeGen/SelectionDAG/SelectionDAGISel.cpp Target/PowerPC/PPCAsmPrinter.cpp Target/PowerPC/PPCRegisterInfo.cpp Target/X86/X86AsmPrinter.cpp Target/X86/X86RegisterInfo.cpp Transforms/Utils/LowerInvoke.cpp

[Dale Johannesen]

On Apr 1, 2008, at 4:49 AM, Duncan Sands wrote:

> Hi Dale,
>
>> Emit exception handling info for functions which are
>> not marked nounwind,
>
> that doesn't sound right.  As I had already explained to you
> in a private email, the dwarf info has two parts related to
> exception handling: the eh table, and frame moves info.  If
> a function is nounwind then there is no need to generate the
> frame moves info.  If a function has no invokes then there
> is no need to generate the eh table.  These are independent.
> A nounwind function can have invokes (indeed this is quite
> common after optimization - see prune-eh), I see that Anton
> sent you an example.

His example doesn't work.  If it did, quite a few things would be  
wrong, as you say.

>  For such a function you should generate an EH table but no frame  
> moves.

I have not tried to prune the frame moves yet, that isn't needed for  
correctness.

> If a nounwind function has no invokes then you can just not
> generate dwarf info at all (I'm ignoring debug intrinsics
> here, which in general require dwarf info).
> And if a not-nounwind function has no invokes then you don't
> have to generate an EH table.  I think this is already the
> case (hopefully Anton will correct me if I'm wrong): we only
> generate an EH table if there is a personality function, and
> the only way to have a personality function is if there are
> invokes (it is also possible to have invokes without a personality,
> but since we don't know how to codegen those you can just
> ignore this).
>
> So I think all you need to do is this: make generation of
> frame moves optional.  Generate frame moves for a function
> if and only if it is not-nounwind (or has debug intrinsics).
> If the function doesn't require frame moves or an EH table
> then don't generate a dwarf frame for it at all.
>
>> or for all functions when -enable-eh
>> is set, provided the target supports Dwarf EH.
>
> Presumably this is for debugging purposes?  If so, how about
> making this a hidden option and renaming it to -debug-eh ?
> Otherwise people are going to think they need this for eh
> support.  Which they don't, right?  (If I understand you
> correctly, you've basically turned on eh support
> unconditionally).

I am trying to "turn on" EH support based on the IR information rather  
than a command line flag.

Chris did not want to remove -enable-eh yet.  It and ExceptionHandling  
should go away eventually.

>> llvm-gcc generates nounwind in the right places; other FEs
>> will need to do so also.  Given such a FE, -enable-eh should
>> no longer be needed.
>
>> +  /// shouldEmit - Per-function flag to indicate if EH information  
>> should
>> +  /// be emitted.
>>   bool shouldEmit;
>
> This is too crude.  I think you need to distinguish between frame  
> moves
> (= locating the values of variables) and the EH table (matching  
> exceptions
> to actions).  So you need two variables here.
>
>> +    if ((ExceptionHandling || !MF->getFunction()->doesNotThrow()) &&
>>         TAI->doesSupportExceptionHandling()) {
>>       shouldEmit = true;
>
> This looks badly wrong: you just broke nounwind functions that  
> contain invokes.

If such exist, that is true.

>> -  if (!ExceptionHandling)
>> -    PM.add(createLowerInvokePass(getTargetLowering()));
>> +  PM.add(createLowerInvokePass(getTargetLowering()));
>
> Does this mean that you just lowered all exception handling to  
> setjmp/longjmp?

No.  The idea was to postpone deciding to do LowerInvoke until  
LowerInvoke is actually executed, when we have a function to look at.   
But probably this should look at  getTargetAsmInfo()- 
 >doesSupportExceptionHandling() instead.  I think we want to do this  
lowering whenever the target doesn't support Dwarf EH.

> Hopefully not, but I vaguely recall from the last time I looked at  
> this stuff
> that it is surprisingly easy to make a mistake here.

That is true, but I did test it.

>> -  if (!ExceptionHandling)
>> -    PM.add(createLowerInvokePass(getTargetLowering()));
>> +  PM.add(createLowerInvokePass(getTargetLowering()));
>
> Likewise.
>
>> +  // Figure out whether we need to generate EH info.  Currently we  
>> do this for
>> +  // all functions not marked no-unwind, or if requested via - 
>> enable-eh.
>> +  needsExceptionHandling = ExceptionHandling || !Fn.doesNotThrow();
>
> This is wrong, see above.  There's already a test somewhere that  
> only outputs
> the EH table if there is a personality function.
>
>>   case Intrinsic::eh_exception: {
>> -    if (ExceptionHandling) {
>> +    if (FuncInfo.needsExceptionHandling) {
>
> This is backwards.  If a function has invokes then it requires an EH  
> table.
> The only way to get the eh_exception intrinsic is to have an  
> invoke.  Thus if
> you see eh_exception then the function has an invoke so for sure you  
> want to
> lower it (unless eh has been globally turned off, which is what the  
> previous
> test was for).  There should be no test here.  You should really just
> substitute "true" for ExceptionHandling everywhere...
>
>> -    if (ExceptionHandling && MMI) {
>> +    if (FuncInfo.needsExceptionHandling && MMI) {
>
> Likewise for all the other examples of this.
>
>> -  if (LandingPad && ExceptionHandling && MMI) {
>> +  if (LandingPad && FuncInfo.needsExceptionHandling && MMI) {
>>     // Insert a label before the invoke call to mark the try  
>> range.  This can be
>
> Likewise: you're only getting here if you have an invoke, so no test
> is needed (see below also).
>
>> bool LowerInvoke::runOnFunction(Function &F) {
>> +  // If we will be generating exception info, don't do anything  
>> here.
>> +  if ((ExceptionHandling || !F.doesNotThrow()) &&
>> +      TLI &&
>> +      TLI->getTargetMachine().getTargetAsmInfo()->
>> +                              doesSupportExceptionHandling())
>> +    return false;
>
> The !F.doesNotThrow() test is bogus.  Also, what is  
> ExceptionHandling doing
> here?  Shouldn't the test be: do this if -enable-correct-eh-support  
> was
> specified and the target does not support exception handling?


Currently, if -enable-correct-eh-support is not on here, we simply  
change invokes to calls.  I could be persuaded that an error is better.