[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 :)

> 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 :)

Ciao,

Duncan.