[llvm-commits] [llvm] r80428 - in /llvm/trunk: include/llvm/Target/TargetLowering.h lib/CodeGen/AsmPrinter/DwarfException.cpp lib/Target/PowerPC/PPCISelLowering.cpp lib/Target/PowerPC/PPCISelLowering.h lib/Target/X86/X86ISelLowering.cpp lib/Target/X86/X86ISelLowering.h

[Bill Wendling]

On Aug 30, 2009, at 7:06 AM, Duncan Sands wrote:

> Hi Bill,
>
>>>> -    Asm->EmitInt8(dwarf::DW_EH_PE_pcrel | dwarf::DW_EH_PE_sdata4);
>>>> -    Asm->EOL("FDE Encoding (pcrel sdata4)");
>>>> +    Encoding = Asm->TM.getTargetLowering()- 
>>>> >getPreferredLSDADataFormat();
>>>> +    Asm->EmitInt8(Encoding);
>>>> +    Asm->EOL("LSDA Encoding", Encoding);
>>>
>>> here you state how you will output the data.  But I don't see the
>>> corresponding changes to the places where the data is actually  
>>> output.
>>>
>> Eric got the LSDA to look sensible before this change. :-)  
>> Essentially, this is fixing a long-standing bug on Darwin where we  
>> weren't emitting the encoding in the CIE properly. But I'll verify  
>> that the LSDA looks good.
>
> I don't get it.  You have a target method (getPreferredLSDADataFormat)
> to tell you the LSDA format.  Surely the place the LSDA is written
> should query this method and output data according to what it returns?
> Instead it looks like the LSDA output stuff has it's own way of  
> working
> out the format, so you have two places that need to know the same bit
> of information (the format), but each is driven by different target
> methods and different logic that needs to be kept in sync.  That's  
> just
> asking for trouble!  At this point I should confess that I didn't  
> bother
> looking at the LSDA code, so hopefully it's not the duplicated mess my
> fevered brain is imagining :)
>
If that were the only thing wrong with our exception handling output,  
I would be happy. :-)

The naming of this method is probably unfortunate. I'm going off of  
the GCC naming, which seems reasonable. The actual output of the LSDA  
doesn't need to query this (it needs other formatting information, but  
not this particular one). It just magically outputs it in the correct  
format. Or at least the unwinder will see it as such.

I'll mention that this isn't the optimal solution. Eric and I talked  
with Chris, and he wants to go back to "first principles" with this  
stuff. For instance, "why do we use pcrel for one architecture and  
pcrel+sdata4 for another?" It's probably a function of other factors  
in the object file. If we can do it *that* way, special ugly calls  
like getPreferred*DataFormat will go away.

>> True. But I much prefer using sizeof(int32_t) to a "magic constant"  
>> here. It at least shows where this value is coming from.
>
> I guess this is a question of taste.  int32_t seems just as magic as 4
> to me :)
>
It would probably be better to simply use the actual byte size of the  
pointer. Though I'm not at all sure how that would work on machines  
with ptrs > 8 bytes.

-bw