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

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.

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

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.

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.

Sounds good.

-Nick

Thanks for the discussion.
The fix committed at r200911.