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

[Shankar Easwaran]

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

Thanks

Shankar Easwaran

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