Simon Atanasyan
2014-Feb-04 22:19 UTC
[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
Rui Ueyama
2014-Feb-04 23:54 UTC
[LLVMdev] [lld] Allow atoms with empty name in the RefNameBuilder::buildDuplicateNameMap()
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>
Nick Kledzik
2014-Feb-05 01:06 UTC
[LLVMdev] [lld] Allow atoms with empty name in the RefNameBuilder::buildDuplicateNameMap()
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
Simon Atanasyan
2014-Feb-05 19:56 UTC
[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.