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

[Rafael Espíndola]

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

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?

>
> After both patches are applied:
> + ARM/X86/MBlaze share the bulk of RecordRelocation
> + ARM/X86/MBlaze's special case is limited to SetRelocType()
> - RecordRelocation now slightly more messier, but can be fixed later
> to get rid of the references.
>
> The patches apply cleanly (-p1)  to -r120721
>
> Thanks
> -jason
>

Cheers,
Rafael