[llvm-commits] [lld] Patch for ReaderELF split into ELFAtoms.h

[Michael Spencer]

On Fri, Dec 21, 2012 at 6:10 AM, Shankar Easwaran
<shankare at codeaurora.org> wrote:
> Hi Nick,
>
>
> On 12/20/2012 7:59 PM, Nick Kledzik wrote:
>>
>> On Dec 20, 2012, at 2:47 PM, Shankar Easwaran wrote:
>>>
>>> The attached patch contains changes for the below :-
>>>
>>> a) Split ReaderELF.cpp into ELFAtoms.h so that atoms can be shared
>>> between Reader/Writer
>>
>> The ReaderELF and WriterELF should not be sharing ELFAtom.   Everything
>> the writer needs should be in the atoms already.  You should be able to
>> create atoms in YAML that the ELF writer can process.     How were you
>> thinking of using ELFAtoms in the writer?
>
> I was thinking of using the ELFUndefinedAtoms/ELFAbsoluteAtom in the writer
> to add ELF's own atoms for symbol resolution. I have to duplicate this code
> otherwise. Example (__bss_start(absolute), _main(undefined) )

This use case is fine, but it does make it easier for the Writer to
accidentally depend on the ELF reader being used. I believe all
writers will need the ability to create atoms, so it may be useful to
have atoms designed for this usecase.

I don't believe that this needs to be done for this patch though, as
it's easy enough to verify that we don't depend on the ELF reader by
going through YAML or Native.

>
>>> b) Changes to the DefinedAtom.h so that we can easily sort the segments
>>> and sections by descending order of permissions.
>>
>> This seems a little magical and may not make sense for other platforms.
>> If there is supposed to be a specific ordering of segments in an ELF
>> executable, the ELF writer code should do that explicitly.  Relying on the
>> ordering of symbolic values in an enum is too fragile.
>
> Right now, this change works for ELF, In future may be the ELF Writer could
> have its own set of enumerations derived from permissions.
>

It would be simple to just have a function that takes a permission and
returns its numerical order and use that when sorting.

>>> c) Fix testcase for the behaviour to be consistent
>>
>>
>>> diff --git a/include/lld/Core/DefinedAtom.h
>>> b/include/lld/Core/DefinedAtom.h
>>> index c70b6c9..e3988bc 100644
>>> --- a/include/lld/Core/DefinedAtom.h
>>> +++ b/include/lld/Core/DefinedAtom.h
>>> @@ -97,6 +97,7 @@ public:
>>>       enum Merge {
>>>       mergeNo,                // Another atom with same name is error
>>> +    mergeByContent,         // Merge sections using content
>>>       mergeAsTentative,       // Is ANSI C tentative defintion, can be
>>> coalesced
>>>       mergeAsWeak,            // is C++ inline definition that was not
>>> inlined,
>>>                               // but address was not taken, so atom can
>>> be hidden
>>
>> This change should be its own patch which adds support for this concept
>> throughout lld.
>> The yaml reader/writer need to know about this, as well as the native
>> reader/writer.
>> The Resolver should also do something with this to actually merge by
>> content.
>> There should be generic (not ELF) yaml test cases which exercise this
>> functionality
>
> I can remove this for now and add it in a later patch, when we start
> supporting it.

Sounds good.

>
>
> Thanks
>
> Shankar Easwaran
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
> the Linux Foundation
>

- Michael Spencer