[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

[Duncan Sands]

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.

if the place that outputs the data already has enough info to get the
format right, then doesn't that mean that the getPreferredLSDADataFormat
target hook is redundant?  It could presumably be implemented as a
utility in DwarfException.cpp, and be driven by the same target info
that is driving LSDA output.
> 
> 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.

Sure, it should all be driven by a logical and orthogonal set of
target information.  That's why I don't see the point of introducing
a new target hook (getPreferredLSDADataFormat) which is not orthogonal
at all, since it can apparently be calculated from existing target
settings...

> 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.

I guess 16 bit machines are more likely :)  Anyway, the only real point
I want to make is that rather than having stuff like "if (4 bytes) then
x; else (y)" all over the place, it would be neater to have methods that
work without any assumptions on pointer sizes, even if no such machines
exist.

Ciao,

Duncan.