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

[Jason Kim]

+llvmdev

On Fri, Nov 12, 2010 at 7:57 AM, Jim Grosbach <grosbach at apple.com> wrote:
>
> 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.

Thanks for the feedback, Jim. I'll wait a bit to see if there's an
interesting answer to:
Anyone have any idea why the ELFObjectWriter/ and WriterImpl classes
were split?


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