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

[Jason Kim]

Hi everyone.

Just a heads up:
Recent commits broke my patches. Please apply to  -r118656 to examine.

Thanks
-jason

On Wed, Nov 10, 2010 at 12:50 PM, Jason Kim <jasonwkim at google.com> wrote:
> 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
>>
>