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

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.

On Feb 5, 2014, at 11:56 AM, Simon Atanasyan <simon at atanasyan.com>
wrote:
> 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

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

On Thu, Feb 6, 2014 at 3:06 AM, Nick Kledzik <kledzik at apple.com>
wrote:
>
> On Feb 5, 2014, at 11:56 AM, Simon Atanasyan <simon at atanasyan.com>
wrote:
>
>> 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.
Thanks for the discussion.
The fix committed at r200911.

-- 
Simon