Page MenuHomePhabricator

[lld] support --trace-symbol (alias -y) option
ClosedPublic

Authored by joshishr on Jun 21 2016, 2:29 AM.

Details

Reviewers
ruiu
llvm-commits
Summary

I have implemented "--trace-symbol=symbol" (alias -y symbol) option.

This option provides names of all the link time modules which define and reference symbols requested by user. This helps to speed up application development by detecting references causing undefined symbols. It also helps in detecting symbols being resolved to wrong (unintended) definitions in case of applications containing multiple definitions for same symbols with different types, bindings.

Implementation contains symbols from object files, shared and archive libraries and executable.

Bugzilla - Bug - PR#28226

subversion revision in lld trunk on which this patch is created: #273248

Patch by Shridhar Joshi

Diff Detail

Event Timeline

joshishr retitled this revision from to [lld] support --trace-symbol (alias -y) option.
joshishr updated this object.
joshishr added a reviewer: llvm-commits.
ruiu added a subscriber: ruiu.Jun 21 2016, 3:04 AM
ruiu added inline comments.
ELF/Config.h
70

Use StringSet<> instead. I'd name this TraceSymbol since all members in Config class are named after their corresponding command line options.

ELF/Driver.cpp
420

Remove {}

423

Define -y as an alias to --trace-symbol in Options.td instead of handling the two as different options.

ELF/Error.cpp
53 ↗(On Diff #61347)

No need to define this function -- just print out to out().

ELF/InputFiles.cpp
318

This generic name is overkill. I'd define two functions

void ObjectFile<ELFT>::traceDefined(StringRef Name);
void ObjectFile<ELFT>::traceUndefined(StringRef Name);

where traceDefined prints out "definition of" message and traceUndefined prints out "reference to" message, and call them from ObjectFile<ELFT>::createSymbolBody.

ELF/Options.td
171

"Trace references to symbol"

ELF/SymbolTable.cpp
348–349

Remove.

test/ELF/trace-symbols.s
2

Insert blank lines to separate this file in meaningful blocks. Wrap in 80 columns.

ruiu added a comment.Jun 21 2016, 3:25 AM

Noticed that there's a better way to print out "definition of" messages.

First, just add traceUndefined member function to ObjectFile and print out "reference to" messages if a given string is a member of Config->TraceSymbol.

And then define traceSymbol member function to SymbolTable. That function should iterate over all strings in Config->TraceSymbol. For each string, it looks up a defined symbol, and if it is defined, it prints out a "definition of" message. SymbolTable has scan* functions, and they are good examples how to do it.

In this way, I think code for --trace-symbol would be separated nicely from the main code.

grimar added a subscriber: grimar.Jun 21 2016, 4:40 AM
grimar added inline comments.Jun 21 2016, 6:23 AM
ELF/InputFiles.cpp
316

Comments should be started from uppercase and ended with dot.

test/ELF/Inputs/trace-symbols-foo-global.s
22 ↗(On Diff #61347)

I think this file contains too much excessive data for test. Like file name, .ident and asm code.
Can you cleanup this ?

emaste added a subscriber: emaste.Jun 21 2016, 10:26 AM
joshishr marked 9 inline comments as done.Jun 21 2016, 12:10 PM
In D21550#463069, @ruiu wrote:

Noticed that there's a better way to print out "definition of" messages.

First, just add traceUndefined member function to ObjectFile and print out "reference to" messages if a given string is a member of Config->TraceSymbol.

This will not address references from shared file.

And then define traceSymbol member function to SymbolTable. That function should iterate over all strings in Config->TraceSymbol. For each string, it looks up a defined symbol, and if it is defined, it prints out a "definition of" message. SymbolTable has scan* functions, and they are good examples how to do it.

In this way, I think code for --trace-symbol would be separated nicely from the main code.

This will not address symbol defined in different object files with strong and weak bindings and with different link order of those object files. It also will not address same symbol defined in archive and shared library with archive library being first in link order.

ELF/SymbolTable.cpp
348–349

This is required to print object files names containing weak and strong definitions as below.
main.o references foo function
foo-weak.o contains weak definition of foo
foo-strong.o contains strong definition of foo

$ld.lld --trace-symbol=foo main.o foo-weak.o foo-strong.o
main.o: reference to foo
foo-weak.o: definition of foo
foo-strong.o: definition of foo

now if the link order of foo-weak.o and foo-strong.o is changed then:
$ld.lld --trace-symbol=foo main.o foo-strong.o foo-weak.o
main.o: reference to foo
foo-strong.o: definition of foo

Please note it has not print "foo-weak.o definition of foo"

Incorporated review comments.

Please also add a test with --start-lib/--end-lib.

ELF/InputFiles.cpp
322

This is duplicated with elf::getFilename(InputFile *F).

326

I don't think you need the llvm:: prefix.

341

Can you share a bit more code with traceDefined?

384

Don't you want to trace this?

537

Do you really want to trace this? It is not a "real" undefined symbol. We will not fetch archive members or report an error for it.

ruiu added inline comments.Jun 21 2016, 2:25 PM
ELF/Config.h
70

As you can see, the members of this class are sorted alphabetically. So, move this right after llvm::StringRef Sysroot.

ELF/InputFiles.cpp
318

StringRef is a small object (containing two pointers) and usually passed by value. Remove &.

319

Guard this with

if (Config->TraceSymbol.empty() || !Config->TraceSymbol.count(Name))
  return;

Because empty() is going to be inlined, the first expression is compiled to two machine instructions. On the other hand, I believe count() is expensive.

320

Do not construct a string to print it out because it's needlessly slow. You can print it out directly to outs().

537

Agreed. That's why I asked to define traceUndefined to ObjectFile instead of InputFile since the class is the only class that needs the method.

ELF/Options.td
167

Please rebase this patch and use J macro.

ELF/SymbolTable.cpp
348–349

I don't think we need to print out weak symbols that are not linked to the resulting executable. We are trying hard to minimize the amount of code that's executed for each symbol, so I don't want to add new code here. Please iterate only over symbols passed as --trace-symbol arguments instead of iterating over all symbols in all input files.

test/ELF/trace-symbols.s
5

We usually indent continued lines with two spaces.

11–12

We usually group a test command and its expected result as one block, instead of writing all commands at beginning of file and all CHECK conditions at end of file.

joshishr marked 14 inline comments as done.Jun 22 2016, 10:12 AM

Please also add a test with --start-lib/--end-lib.

Added.

ELF/InputFiles.cpp
384

It was traced inside addRegular to distinguish between weak and strong symbol. In updated patch it is handled in SymbolTable.

537

This was required to print undefined symbols from shared library and not from archive members.
For e.g. main calls foo() defined in libfoo.so and this foo() calls bar() defined in libbar.so. this call to traceUndefined() was required to print reference to bar(). Anyway i have removed it in updated patch and so updated patch does not report references from shared library.

Anyway if you think this is required and we should also move traceUndefined() to ObjectFile then also we can report this using Undefs inside SymbolTable. But then we have to rename traceDefined() as it will handle undefined symbols from shared library as well as defined symbols from object files, shared and archive libraries.

Please confirm.

ELF/SymbolTable.cpp
348–349

With traceDefined() moved to SymbolTable class, there is change in behaviour:

  1. Definition to weak function will not be printed even if the object file or shared shared library defining it comes before in the link order than object file or shared library defining the same symbol strong.
  2. Definition of common symbols from object files and archive library will not be printed but common symbols from shared library will be printed. This looks like because DefinedCommon class does not contain source file information.
joshishr marked 2 inline comments as done.

Moved trace* functions out of core symbol resolution code. Incorporated more review comments.

ruiu added a comment.Jun 22 2016, 7:42 PM

Thank you for making the changes. Almost looking good, a few nits.

Do you have a commit access?

ELF/Driver.cpp
514

What this function does is similar to scan* functions, so move this after scanVersionScript.

ELF/InputFiles.cpp
321

outs() knows how to print out StringRefs, so remove .str().

ELF/SymbolTable.cpp
548

Let's name B instead of L since it is a sort of convention in LLD.

joshishr marked 3 inline comments as done.EditedJun 22 2016, 11:42 PM
In D21550#465268, @ruiu wrote:

Thank you for making the changes. Almost looking good, a few nits.

Do you have a commit access?

Incorporated all review comments.

No i don't have commit access. I will apply to get commit access.
For now, if no issue please check it in.

ruiu removed a subscriber: ruiu.Jun 22 2016, 11:46 PM
ruiu added a subscriber: ruiu.

Incorporated all review comments and created updated patch.

subversion revision in lld trunk on which this patch is created: #273532

ruiu accepted this revision.Jun 23 2016, 12:07 AM
ruiu added a reviewer: ruiu.

Submitted as 273536.

This revision is now accepted and ready to land.Jun 23 2016, 12:07 AM
ruiu closed this revision.Jun 23 2016, 12:07 AM
In D21550#465360, @ruiu wrote:

Submitted as 273536.

Thank you
-Shridhar Joshi