This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Allow LLD to produce file symbols.
ClosedPublic

Authored by grimar on Apr 4 2018, 8:20 AM.

Details

Summary

This is for PR36716 and
this enables emitting file local symbols.

Output size affect is minor:
lld binary size changes from 52,883,408 to 52,949,400
clang binary size changes from 83,136,456 to 83,219,600

Diff Detail

Event Timeline

grimar created this revision.Apr 4 2018, 8:20 AM
grimar edited the summary of this revision. (Show Details)Apr 4 2018, 8:24 AM
espindola added inline comments.Apr 4 2018, 8:43 AM
test/ELF/file-sym.s
9

You can probably just delete this file now. We will add a more complete one with the fix to PR36716.

BTW, how come lld is larger than clang in your measurements?

grimar added a comment.Apr 4 2018, 8:46 AM

BTW, how come lld is larger than clang in your measurements?

It was a mistake in the description (wrong numbers order), I fixed it.

grimar added a comment.Apr 4 2018, 8:48 AM

BTW, how come lld is larger than clang in your measurements?

Ah, sorry. I need to change 'lld' and 'clang' too. My mistake. Sure lld < clang.

grimar edited the summary of this revision. (Show Details)Apr 4 2018, 8:48 AM
grimar updated this revision to Diff 140970.Apr 4 2018, 8:51 AM
  • Remove the file-sym.s test case.
espindola accepted this revision.Apr 4 2018, 9:17 AM

LGTM, now we just have to sort them :-)

This revision is now accepted and ready to land.Apr 4 2018, 9:17 AM
ruiu added a comment.Apr 4 2018, 11:19 AM

By the way, when you mention a bug in a commit message, can you use a link (e.g. https://bugs.llvm.org/show_bug.cgi?id=36716) instead of PRxxxxx for me? Currently it takes like 10 seconds to just open a bug described in a commit message.

@ruiu just in case you didn't know, the URL https://llvm.org/PR36716 will do a redirect to the right place.

ruiu added a comment.Apr 4 2018, 12:40 PM

Thank you for the tip!

George, please just append https://llvm.org/ when you mention a bug. :)

ruiu added a reviewer: pcc.Apr 4 2018, 12:46 PM

Peter,

What do you think about this change?

pcc added a comment.Apr 4 2018, 1:06 PM

LGTM, now we just have to sort them :-)

Don't we need to do that at the same time as emitting them? The generic ABI says:

STT_FILE
Conventionally, the symbol's name gives the name of the source file associated with the object file. A file symbol has STB_LOCAL binding, its section index is SHN_ABS, and it precedes the other STB_LOCAL symbols for the file, if it is present.

STT_FILE
Conventionally, the symbol's name gives the name of the source file associated with the object file. A file symbol has STB_LOCAL binding, its section index is SHN_ABS, and it precedes the other STB_LOCAL symbols for the file, if it is present.

Yes, looks like we have to fix PR36716 in one go. Do you have a preference over the two orders listed on the bug?

pcc added a comment.Apr 4 2018, 3:08 PM

This one seems a little better in my opinion, for the reason you mentioned.

file1, local1, hidden1, file2, local2, hidden2

It also seems to be the only one that conforms to the ABI because hidden symbols will be STB_LOCAL in the output file.

grimar added a comment.Apr 5 2018, 8:05 AM

STT_FILE
Conventionally, the symbol's name gives the name of the source file associated with the object file. A file symbol has STB_LOCAL binding, its section index is SHN_ABS, and it precedes the other STB_LOCAL symbols for the file, if it is present.

Yes, looks like we have to fix PR36716 in one go.

precedes the other STB_LOCAL symbols for the file

I think this is always true with this patch. Even with asm below, first local symbol in the object is a file symbol.
LLD naturally places all local symbols in order in copyLocalSymbols.

.global bar1
.hidden bar1
bar1:

.file "file1"
foo1:

I'll post the patch fixing PR36716 in a few minutes, but I think we are safe to land this first to simplify the review of the second one.

True, this patch seems safe.

Please check in, but add a trivial test from PR36716 so that we see what it is producing.

This revision was automatically updated to reflect the committed changes.