llvm dev - Feb 2014 - [LLVMdev] [lld] Allow atoms with empty name in the RefNameBuilder::buildDuplicateNameMap()

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

I'm not opposed to remove the assertion from the file, as I believe a
symbol with empty name is a valid symbol. Not 100% sure what the ELF spec
says, though.


On Tue, 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140204/f9249c27/attachment.html>

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

I have read ELF specification once again. There is no any explicit
references to absolute symbols with no name. The only real case when I
see such symbol is a local absolute symbol with "STT_FILE" type. The
name of "STT_FILE" symbol is a name of the source file associated with
the object file and the "STT_FILE" symbol precedes the other STB_LOCAL
symbols from this object file. So it is just a "marker" symbol.

If think it's enough to do not call buildDuplicateNameMap() if the
absolute atom has no name. I suggest to fix that in the code and write
a test covers this case.

-- 
Simon

On Wed, Feb 5, 2014 at 5:06 AM, Nick Kledzik <kledzik at apple.com>
wrote:
> 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?
[...]
> On Feb 4, 2014, at 2:19 PM, Simon Atanasyan <simon at atanasyan.com>
wrote:
>> 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.
[...]
>> I suggest to remove assert from the
>> RefNameBuilder::buildDuplicateNameMap() method. I think we do not
>> break anything in that case.