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

[Jason Kim]

On Wed, Nov 10, 2010 at 9:57 AM, Jason Kim <jasonwkim at google.com> 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
>
> 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.
>


diffview at http://codereview.chromium.org/4703006

TryB is actually a little bit smaller than tryA, and by keeping the
factory within ELFObjectWriter(), I was able to avoid the linkage
issue I ran into earlier.
It seems this is probably a little bit better than tryA

Its smaller
No switch between ELF and MachO
Rest of the ELFObjectWriter is not modified - the manual trampoline is
still in place.

-jason

> Thanks for reading.
> -jason
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tryB-combined.patch
Type: text/x-patch
Size: 22132 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101110/add7b197/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tryB.tgz
Type: application/x-gzip
Size: 5283 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101110/add7b197/attachment-0001.bin>