[llvm-commits] [PATCH] elfobjectwriter patch (ARM/MC/ELF)

[Jim Grosbach]

On Nov 10, 2010, at 9:57 AM, Jason Kim wrote:

> Refactoring the x86 dependent code from ELFObjectWriter class has
> repercussions among several (conflicting) axes of consideration,
> 
> 1. namespace pollution - minimize pollution of the llvm:  namespace
> 2. consistency - try to maintain as small a delta between the changes
> 3. linking - minimize the number of additional cross dependency
> between the existing libraries
> 4. clarity - avoid special case switching as much as possible -
> 5. Xfactor - how clean is the overall resulting design?
> 
> The possible  ways forward I see are
> 
> SMALL: keep the code nearly as is - place a switch inside
> ELFObjectWriter::RecordRelocation and dispatch to
> ELFObjectWriterImpl::RecordRelocation<ARCH>
> 1. +1 no new classes
> 2. +1 tiny patch
> 3. +1 no new classes, just one additional function so far.
> 4. -2 need to have special case switching for every routine that needs
> to be tweaked.
> 5. -2 Terrible! So far, its just one new switch, but ...
> 
> tryA: move the functionality of the ELFObjectWriterImpl class into
> ELFObjectWriter, and subclass ELFObjectWriter to
> <ARCH>ELFObjectWriter.
> Change most ELF specific routines to be virtual - except for the low
> level Write* routines -
> 1. -1 at least new classes ARMELFObjectWriter and X86ELFObjectWriter
> 2. -1 large patch
> 3. +2 Resulting special cases are isolated in their own class
> 4. +1  Depends upon virtual dispatch for higher level differentiation
> - removes unnecessary trampoline between ELFObjectWriter and
> ELFObjectWriterImpl
> 5. +2 This approach is the best in terms of the resulting design. The
> only drawback is the distinction between MachO and ELF
> 

This looks OK to me, and is similar to what I'm looking at for MachO. I confess I'm not completely clear on why the ELF writer is currently split into the Writer and WriterImpl classes, so that may be worth answering before proceeding too far.



> tryB: subclass ELFObjectWriterImpl instead - I am still working out
> the details on this one - but as of right now, it is just as complex
> as the tryA case.
> The only benefit to this is approach is the superficial similarity
> between the ELFObjectWriter and the MachObjectWriter - in that both
> still trampoline into an *Impl class to do the actual work.
> Unfortunately, it also adds a requirement for registering a NEW
> <ARCH>ELFObjecWriterImpl class (i.e. in addition to the existing
> createELFObjectWriter, and without namespace pollution to llvm,
> creates a linkage dependency failure).
> I will reply to this thread with a patch as soon as I finish this variant.
> 
> There are several attachments:
> 
> small:
> small-elfwriter-cpp (application/octet-stream) 2K
> small-elfwriter-rename-record (application/octet-stream) 2K
> 
> tryA
>  arm-mc-elf-s07-elfwriter-tryA-combined.patch (text/x-patch) 43K -
> this is the combined patch - In order to make it more clear, I broke
> up the steps into a dozen or so smaller patches.
>  tryA.tgz  - archive of patches
>  They are combined into a tar archive - the README is reproduced here.
> 
> Thanks for reading.
> -jason
> <arm-mc-elf-s07-elfwriter-tryA-combined.patch><small-elfwriter-cpp><small-elfwriter-rename-record><tryA.tgz><README>