[llvm-commits] [patch] ARM/MC/ELF RecordRelocation refactoring

[Jason Kim]

On Sat, Dec 4, 2010 at 2:30 PM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
>> The first patch
>> It adds a new method to X86 GetRelocType() and lifts RecordRelocation
>> to ELFObjectWriter
>>
>> + ARM/X86 no longer require their own versions of RecordRelocation
>> - MBlaze still does, because it is expecting to access RelocSymbol and
>> Addend when selecting the reloc type.
>>
>> The first patch leaves MBlaze completely alone.
>
>
> +  protected:
> +    virtual unsigned GetRelocType(const MCValue &Target, const MCFixup &Fixup,
> +                                  bool IsPCRel) {
> +      assert(0 && "GetRelocType() needs to be more specific.");
> +    };
> +    virtual bool isFixupKindPCRel(unsigned Kind) const {
> +      assert(0 && "isFixupKindPCRel() needs to be more specific.");
> +    };
>
> Can you make them pure virtuals?

Hi Rafael.

Thanks for feedback.
Yup! Can do.

>
> Not sure if we will be able to share this much of RecordRocation, but
> it is really nice if we can :-)
>
> The first patch is OK.
>
>>
>> The second patch deals with MBlaze by using ERE early, and changes
>> GetRelocType() to SetRelocType.
>> The new set method takes in the ERE instance so that MBlaze has access
>> to RelocSymbol and Addend.
>>
>> Peckw: question for you
>> Is the RelocSymbol variable really necessary when selecting the
>> relocation? or can it be done more simply?
>
> If I read it correctly, the check
>
>      Type = (RelocSymbol || Addend !=0) ? ELF::R_MICROBLAZE_32
>                                         : ELF::R_MICROBLAZE_64;
>
> is just that this is a relocation with a section and has a non zero
> addend. Can you just add the Addend and a Boolean
> IsRelocationWithSymbol to the getRelocType method?

I thought about  adding new parameter(s) to GetRelocType, but given
that these two (RelocSymbol and Addend) are already available as part
of the ERE, it was a matter of adding two arguments just for the sake
of MBlaze, or isolate the arch specific code within the single
(renamed) function, and to pass the ERE directly (i.e. 1 addl' param).
I chose the latter -

For now, I'll commit the first patch.

Thanks
-jason