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

[Nick Kledzik]

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?


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


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


-Nick