[LLVMdev] [lld] Allow atoms with empty name in the RefNameBuilder::buildDuplicateNameMap()

[Nick Kledzik]

The point of the RefNameBuilder is to create unique names for atoms that need them.  For instance, if you have two files each with a static function named “foo” and you merge the .o files together and dump that as yaml, then when some other function has a reference to “foo” the yaml would be ambiguous.  In that case some alternate (unambiguous) name is generated for the “foo” atoms and all uses would  use the alternate name.  

In your case, you seem to have absolute symbols with no name.  If the atoms for those symbols need to be referenced in the yaml, then they would need a name generated, and the assert is blocking that.  If they are never referenced, then they should not be passed to buildDuplicateNameMap().

Notice earlier in the code, for DefinedAtoms, buildDuplicateNameMap() is *not* called if the atom has no name.  That was done because there are unnamed atoms (such as literals) which have no name.  But if ELF allows absolute symbols with no name, does it allow symbols with no name to content in sections?  

-Nick

On Feb 4, 2014, at 2:19 PM, Simon Atanasyan <simon at atanasyan.com> wrote:
> Hi,
> 
> Method RefNameBuilder::buildDuplicateNameMap() has an assert which
> blocks atoms with empty name. In general it looks reasonable but some
> toolchains (for example Sourcery CodeBench in both MIPS and ARM
> editions) can generate an object file contains absolute STT_FILE
> symbols with empty name. Moreover crt1.o object file from this
> toolchain has such symbol. I do not know is it a feature or bug but
> that behavior exists for a long time.
> 
> Here is the reproduction script:
> [[
>  $ cat test.c
>  void foo() {}
>  $ mips-linux-gnu-gcc -c test.c
>  $ mips-linux-gnu-gcc -r -nostdlib test.o
>  $ readelf -s a.out | grep FILE
>       9: 00000000     0 FILE    LOCAL  DEFAULT  ABS test.c
>      10: 00000000     0 FILE    LOCAL  DEFAULT  ABS
> ]]
> 
> I suggest to remove assert from the
> RefNameBuilder::buildDuplicateNameMap() method. I think we do not
> break anything in that case.
> 
> Any objections, comments, suggestions?
> 
> [[
> --- a/lib/ReaderWriter/YAML/ReaderWriterYAML.cpp
> +++ b/lib/ReaderWriter/YAML/ReaderWriterYAML.cpp
> @@ -101,7 +101,6 @@ public:
>   }
> 
>   void buildDuplicateNameMap(const lld::Atom &atom) {
> -    assert(!atom.name().empty());
>     NameToAtom::iterator pos = _nameMap.find(atom.name());
>     if (pos != _nameMap.end()) {
>       // Found name collision, give each a unique ref-name.
> ]]
> 
> -- 
> Simon Atanasyan