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

[• joshishr]

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

[ruiu]

ELF/Config.h

67

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

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

129

"Trace references to symbol"

ELF/SymbolTable.cpp

348–349

Remove.

test/ELF/trace-symbols.s

1

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

[ruiu]

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]

ELF/InputFiles.cpp

316

Comments should be started from uppercase and ended with dot.

test/ELF/Inputs/trace-symbols-foo-global.s

22

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

[• joshishr]

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"

[• joshishr]

Incorporated review comments.

[• rafael]

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?

399

Don't you want to trace this?

552

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]

ELF/Config.h

67

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

552

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

125

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]

In D21550#463636, @rafael wrote:

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

Added.

ELF/InputFiles.cpp

399

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

552

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]

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

[ruiu]

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

Do you have a commit access?

ELF/Driver.cpp

516

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

546

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

[• joshishr]

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.

[• joshishr]

Incorporated all review comments and created updated patch.

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

[ruiu]

Submitted as 273536.

[• joshishr]

In D21550#465360, @ruiu wrote:

Submitted as 273536.

Thank you
-Shridhar Joshi

ELF/Config.h

struct Configuration {
   llvm::StringRef SoName;
   llvm::StringRef Sysroot;
   std::string RPath;
   std::vector<Version> SymbolVersions;
   std::vector<llvm::StringRef> DynamicList;
   std::vector<llvm::StringRef> SearchPaths;
   std::vector<llvm::StringRef> Undefined;
   std::vector<llvm::StringRef> VersionScriptGlobals;
+  std::vector<llvm::StringRef> TraceSymbolList;
   std::vector<uint8_t> BuildIdVector;
   bool AllowMultipleDefinition;
   bool AsNeeded = false;
   bool Bsymbolic;
   bool BsymbolicFunctions;
   bool Demangle = true;
   bool DisableVerify;
   bool DiscardAll;

ELF/Driver.cpp

void LinkerDriver::readConfigs(opt::InputArgList &Args) {
 
   for (auto *Arg : Args.filtered(OPT_export_dynamic_symbol))
     Config->DynamicList.push_back(Arg->getValue());
 
   if (auto *Arg = Args.getLastArg(OPT_version_script)) {
     if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue()))
       parseVersionScript(*Buffer);
   }
+  for (auto *Arg : Args.filtered(OPT_y)) {
+    Config->TraceSymbolList.push_back(Arg->getValue());
+  }
+  for (auto *Arg : Args.filtered(OPT_trace_symbol)) {
+    Config->TraceSymbolList.push_back(Arg->getValue());
+  }
 }
 
 void LinkerDriver::createFiles(opt::InputArgList &Args) {
   for (auto *Arg : Args) {
     switch (Arg->getOption().getID()) {
     case OPT_l:
       addLibrary(Arg->getValue());
       break;
template <class ELFT> void LinkerDriver::link(opt::InputArgList &Args) {
 
   Symtab.addCombinedLtoObject();
   if (HasError)
     return;
 
   for (auto *Arg : Args.filtered(OPT_wrap))
     Symtab.wrap(Arg->getValue());
 
   // Write the result to the file.
   if (Config->GcSections)
     markLive<ELFT>();
   if (Config->ICF)
     doIcf<ELFT>();
 
   // MergeInputSection::splitIntoPieces needs to be called before
   // any call of MergeInputSection::getOffset. Do that.
   for (const std::unique_ptr<elf::ObjectFile<ELFT>> &F :
        Symtab.getObjectFiles())
     for (InputSectionBase<ELFT> *S : F->getSections())
       if (S && S != &InputSection<ELFT>::Discarded && S->Live)
         if (auto *MS = dyn_cast<MergeInputSection<ELFT>>(S))
           MS->splitIntoPieces();
 
   writeResult<ELFT>(&Symtab);
 }

ELF/Error.h

 }
 
 template <class T> T check(ErrorOr<T> EO, const Twine &Prefix) {
   if (EO)
     return std::move(*EO);
   fatal(EO.getError().message(), Prefix);
 }
 
+void info(const Twine &Msg);
+
 } // namespace elf
 } // namespace lld
 
 #endif

ELF/Error.cpp

void fatal(const Twine &Msg, const Twine &Prefix) {
   fatal(Prefix + ": " + Msg);
 }
 
 void check(std::error_code EC) {
   if (EC)
     fatal(EC.message());
 }
 
+void info(const Twine &Msg) {
+  llvm::outs() << Msg << "\n"; 
+}
+
 } // namespace elf
 } // namespace lld

ELF/InputFiles.h

enum Kind {
     BitcodeKind,
   };
 
   Kind kind() const { return FileKind; }
 
   StringRef getName() const { return MB.getBufferIdentifier(); }
   MemoryBufferRef MB;
 
+  void notify(StringRef &Name, bool isDefined);
+
   // Filename of .a which contained this file. If this file was
   // not in an archive file, it is the empty string. We use this
   // string for creating error messages.
   StringRef ArchiveName;
 
 protected:
   InputFile(Kind K, MemoryBufferRef M) : MB(M), FileKind(K) {}
 

ELF/InputFiles.cpp

elf::ObjectFile<ELFT>::createInputSection(const Elf_Shdr &Sec) {
   // output was choosen. Proccess them as usual input sections.
   if (!Config->Relocatable && Name == ".eh_frame")
     return new (EHAlloc.Allocate()) EhInputSection<ELFT>(this, &Sec);
   if (shouldMerge<ELFT>(Sec))
     return new (MAlloc.Allocate()) MergeInputSection<ELFT>(this, &Sec);
   return new (IAlloc.Allocate()) InputSection<ELFT>(this, &Sec);
 }
 
+// print the module names which references and defines the 
+// notified symbols provided through -y or trace-symbol option
+void InputFile::notify(StringRef &Name, bool isDefined) {
+  if (!Config->TraceSymbolList.empty()) {
+    for (StringRef S : Config->TraceSymbolList) {
+      if (!S.compare(Name)) {
+        std::string Msg;
+        if (!ArchiveName.empty())
+          Msg += ArchiveName.str() + "(" + this->getName().str() + ")";
+        else
+          Msg += this->getName().str();
+        if (isDefined)
+          Msg += ": definition of " + Name.str();
+        else
+          Msg += ": reference to " + Name.str();
+        info(Msg);
+        break;
+      }
+    }
+  }
+}
+
 template <class ELFT> void elf::ObjectFile<ELFT>::initializeSymbols() {
   this->initStringTable();
   Elf_Sym_Range Syms = this->getElfSymbols(false);
   uint32_t NumSymbols = std::distance(Syms.begin(), Syms.end());
   SymbolBodies.reserve(NumSymbols);
   for (const Elf_Sym &Sym : Syms)
     SymbolBodies.push_back(createSymbolBody(&Sym));
 }
 
 template <class ELFT>
 InputSectionBase<ELFT> *
 elf::ObjectFile<ELFT>::getSection(const Elf_Sym &Sym) const {
if (Sym->st_shndx == SHN_UNDEF)
       return new (Alloc) Undefined(Sym->st_name, Sym->st_other, Sym->getType());
     return new (Alloc) DefinedRegular<ELFT>(*Sym, Sec);
   }
 
   StringRef Name = check(Sym->getName(this->StringTable));
 
   switch (Sym->st_shndx) {
   case SHN_UNDEF:
+    InputFile::notify(Name, false);
     return elf::Symtab<ELFT>::X
         ->addUndefined(Name, Binding, Sym->st_other, Sym->getType(),
                        /*CanOmitFromDynSym*/ false, this)
         ->body();
   case SHN_COMMON:
+    InputFile::notify(Name, true);
     return elf::Symtab<ELFT>::X
         ->addCommon(Name, Sym->st_size, Sym->st_value, Binding, Sym->st_other,
                     Sym->getType(), this)
         ->body();
   }
 
   switch (Binding) {
   default:
     fatal("unexpected binding");
   case STB_GLOBAL:
   case STB_WEAK:
   case STB_GNU_UNIQUE:
     if (Sec == &InputSection<ELFT>::Discarded)
       return elf::Symtab<ELFT>::X
           ->addUndefined(Name, Binding, Sym->st_other, Sym->getType(),
                          /*CanOmitFromDynSym*/ false, this)
           ->body();
     return elf::Symtab<ELFT>::X->addRegular(Name, *Sym, Sec)->body();
   }
 }
 
 template <class ELFT> void ArchiveFile::parse() {
   File = check(Archive::create(MB), "failed to parse archive");
 
   // Read the symbol table to construct Lazy objects.
   for (const Archive::Symbol &Sym : File->symbols())
for (const Elf_Sym &Sym : Syms) {
     unsigned VersymIndex = 0;
     if (Versym) {
       VersymIndex = Versym->vs_index;
       ++Versym;
     }
 
     StringRef Name = check(Sym.getName(this->StringTable));
     if (Sym.isUndefined()) {
+      InputFile::notify(Name, false);
       Undefs.push_back(Name);
       continue;
     }
 
+    if (Sym.isDefined())
+       InputFile::notify(Name, true);
+
     if (Versym) {
       // Ignore local symbols and non-default versions.
       if (VersymIndex == VER_NDX_LOCAL || (VersymIndex & VERSYM_HIDDEN))
         continue;
     }
 
     const Elf_Verdef *V =
         VersymIndex == VER_NDX_GLOBAL ? nullptr : Verdefs[VersymIndex];

ELF/Options.td

 def whole_archive: F<"whole-archive", "Force load of all members in a static library">;
 
 def wrap : Separate<["--", "-"], "wrap">, MetaVarName<"<symbol>">,
   HelpText<"Use wrapper functions for symbol">;
 
 def z : JoinedOrSeparate<["-"], "z">, MetaVarName<"<option>">,
   HelpText<"Linker option extensions">;
 
+def y : JoinedOrSeparate<["-"], "y">,
+  HelpText<"Trace module names which define or reference symbols ">;
+
+def trace_symbol : Joined<["--", "-"], "trace-symbol=">,
+  HelpText<"Trace module names which define or reference symbols ">;
+
 // Aliases
 def alias_Bdynamic_call_shared: Flag<["-"], "call_shared">, Alias<Bdynamic>;
 def alias_Bdynamic_dy: Flag<["-"], "dy">, Alias<Bdynamic>;
 def alias_Bstatic_dn: Flag<["-"], "dn">, Alias<Bstatic>;
 def alias_Bstatic_non_shared: Flag<["-"], "non_shared">, Alias<Bstatic>;
 def alias_Bstatic_static: Flag<["-"], "static">, Alias<Bstatic>;
 def alias_L__library_path : Joined<["--"], "library-path=">, Alias<L>;
 def alias_discard_all_x: Flag<["-"], "x">, Alias<discard_all>;

ELF/SymbolTable.cpp

Symbol *SymbolTable<ELFT>::addRegular(StringRef Name, const Elf_Sym &Sym,
                                       InputSectionBase<ELFT> *Section) {
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) =
       insert(Name, Sym.getType(), Sym.getVisibility(),
              /*CanOmitFromDynSym*/ false, /*IsUsedInRegularObj*/ true,
              Section ? Section->getFile() : nullptr);
   int Cmp = compareDefinedNonCommon(S, WasInserted, Sym.getBinding());
+  if (Cmp >= 0)
+    cast<InputFile>(Section->getFile())->notify(Name, true);
   if (Cmp > 0)
     replaceBody<DefinedRegular<ELFT>>(S, Name, Sym, Section);
   else if (Cmp == 0)
     reportDuplicate(S->body(), Section->getFile());
   return S;
 }
 
 template <typename ELFT>
for (StringRef Name : V.Globals)
         B->symbol()->VersionId = I;
     ++I;
   }
 }
 
 template class elf::SymbolTable<ELF32LE>;
 template class elf::SymbolTable<ELF32BE>;
 template class elf::SymbolTable<ELF64LE>;
 template class elf::SymbolTable<ELF64BE>;

test/ELF/Inputs/trace-symbols-foo-global.s

+  .file "trace-symbols-bar.c"
+  .comm common,4,4
+  .text
+  .globl  foo
+  .type foo, @function
+foo:
+.LFB0:
+  .cfi_startproc
+  pushq %rbp
+  .cfi_def_cfa_offset 16
+  .cfi_offset 6, -16
+  movq  %rsp, %rbp
+  .cfi_def_cfa_register 6
+  nop
+  popq  %rbp
+  .cfi_def_cfa 7, 8
+  ret
+  .cfi_endproc
+.LFE0:
+  .size foo, .-foo
+  .ident  "GCC: (Ubuntu 4.8.4-2ubuntu1~14.04.1) 4.8.4"
+  .section  .note.GNU-stack,"",@progbits

test/ELF/Inputs/trace-symbols-foo-weak.s

+.comm common,4,4
+.text
+.weak foo
+.type foo, @function
+foo:
+nop

test/ELF/Inputs/trace-symbols-main.s

+.hidden hsymbol 
+.text
+.globl  _start
+.type _start, @function
+_start:
+
+.globl  main
+.type main, @function
+main:
+call foo

test/ELF/trace-symbols.s

+# Test -y symbol and -trace-symbol=symbol
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux \
+# RUN: %p/Inputs/trace-symbols-main.s -o %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux \
+# RUN: %p/Inputs/trace-symbols-foo-weak.s -o %t1
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux \
+# RUN: %p/Inputs/trace-symbols-foo-global.s -o %t2
+# RUN: ld.lld -y foo -trace-symbol=common -trace-symbol=hsymbol %t %t1 %t2 -o %t3 2>&1 | FileCheck -check-prefix=OBJECTRFOO %s
+# RUN: ld.lld -y foo -trace-symbol=common -trace-symbol=hsymbol %t %t1 %t2 -o %t3 2>&1 | FileCheck -check-prefix=OBJECTDCOMMON %s
+# RUN: ld.lld -y foo -trace-symbol=common -trace-symbol=hsymbol %t %t1 %t2 -o %t3 2>&1 | FileCheck -check-prefix=OBJECTD1FOO %s
+# RUN: ld.lld -y foo -trace-symbol=common -trace-symbol=hsymbol %t %t1 %t2 -o %t3 2>&1 | FileCheck -check-prefix=OBJECTD2FOO %s
+# RUN: ld.lld -y foo -y common --trace-symbol=hsymbol %t %t2 %t1 -o %t4 2>&1 | FileCheck -check-prefix=OBJECTD2FOO %s
+# RUN: ld.lld -y foo %t %t2 %t1 -o %t4 2>&1 | not FileCheck -check-prefix=OBJECTD1FOO %s
+# RUN: ld.lld -shared %t1 -o %t1.so
+# RUN: ld.lld -y foo -y common %t %t1.so %t2 -o %t5 2>&1 | FileCheck -check-prefix=SHLIBDCOMMON %s
+# RUN: ld.lld -y foo -y common %t %t1.so %t2 -o %t5 2>&1 | FileCheck -check-prefix=SHLIBD1FOO %s
+# RUN: ld.lld -y foo -y common %t %t1.so %t2 -o %t5 2>&1 | FileCheck -check-prefix=OBJECTD2FOO %s
+# RUN: ld.lld -shared %t2 -o %t2.so
+# RUN: ld.lld -y foo -y common %t %t1.so %t2.so -o %t5 2>&1 | FileCheck -check-prefix=SHLIBD1FOO %s
+# RUN: ld.lld -y foo -y common %t %t2.so %t1.so -o %t5 2>&1 | FileCheck -check-prefix=SHLIBD2FOO %s
+# RUN: ar -r %t1.a %t1 
+# RUN: ld.lld -y foo -y common %t %t2 %t1.a -o %t6 2>&1 | FileCheck -check-prefix=OBJECTD2FOO %s
+# RUN: ld.lld -y foo -y common %t %t2 %t1.a -o %t6 2>&1 | not FileCheck -check-prefix=ARCHIVEDCOMMON %s
+# RUN: ar -r %t2.a %t2 
+# RUN: ld.lld -y foo %t %t1.a %t2.so -o %t8 | FileCheck -check-prefix=ARCHIVEDFOO %s
+# RUN: ld.lld -y foo %t %t1.a %t2.so -o %t8 | FileCheck -check-prefix=SHLIBD2FOO %s
+# RUN: ld.lld -y foo %t %t1.so %t2.a -o %t8 | FileCheck -check-prefix=SHLIBD1FOO %s
+# OBJECTRFOO: trace-symbols.s.tmp: reference to foo
+# OBJECTD1FOO: trace-symbols.s.tmp1: definition of foo
+# OBJECTD2FOO: trace-symbols.s.tmp2: definition of foo
+# OBJECTDCOMMON: trace-symbols.s.tmp1: definition of common
+# SHLIBD1FOO: trace-symbols.s.tmp1.so: definition of foo
+# SHLIBD2FOO: trace-symbols.s.tmp2.so: definition of foo
+# SHLIBDCOMMON: trace-symbols.s.tmp1.so: definition of common
+# ARCHIVEDFOO: trace-symbols.s.tmp1.a(trace-symbols.s.tmp1): definition of foo
+# ARCHIVEDCOMMON: trace-symbols.s.tmp1.a(trace-symbols.s.tmp1): definition of common