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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Config.h:68
@@ -66,2 +67,3 @@
   std::vector<llvm::StringRef> VersionScriptGlobals;
+  llvm::StringSet<> TraceSymbol;
   std::vector<uint8_t> BuildIdVector;
----------------
As you can see, the members of this class are sorted alphabetically. So, move this right after `llvm::StringRef Sysroot`.

================
Comment at: ELF/InputFiles.cpp:318
@@ +317,3 @@
+// symbols provided through -y or --trace-symbol option.
+void InputFile::traceDefined(StringRef &Name) {
+  if (Config->TraceSymbol.count(Name)) {
----------------
StringRef is a small object (containing two pointers) and usually passed by value. Remove `&`.

================
Comment at: ELF/InputFiles.cpp:319
@@ +318,3 @@
+void InputFile::traceDefined(StringRef &Name) {
+  if (Config->TraceSymbol.count(Name)) {
+    std::string Msg;
----------------
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.

================
Comment at: ELF/InputFiles.cpp:320
@@ +319,3 @@
+  if (Config->TraceSymbol.count(Name)) {
+    std::string Msg;
+    if (!ArchiveName.empty())
----------------
Do not construct a string to print it out because it's needlessly slow. You can print it out directly to outs().

================
Comment at: ELF/InputFiles.cpp:558
@@ -527,2 +557,3 @@
     if (Sym.isUndefined()) {
+      InputFile::traceUndefined(Name);
       Undefs.push_back(Name);
----------------
rafael wrote:
> 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.
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.

================
Comment at: ELF/Options.td:125
@@ -124,1 +124,3 @@
 
+def trace_symbol : Joined<["--", "-"], "trace-symbol=">,
+  HelpText<"Trace references to symbols">;
----------------
Please rebase this patch and use `J` macro.

================
Comment at: ELF/SymbolTable.cpp:348-349
@@ -347,2 +347,4 @@
   int Cmp = compareDefinedNonCommon(S, WasInserted, Sym.getBinding());
+  if (Cmp >= 0)
+    cast<InputFile>(Section->getFile())->traceDefined(Name);
   if (Cmp > 0)
----------------
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.

================
Comment at: test/ELF/trace-symbols.s:4
@@ +3,3 @@
+# 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 \
----------------
We usually indent continued lines with two spaces.

================
Comment at: test/ELF/trace-symbols.s:10-11
@@ +9,4 @@
+
+# RUN: ld.lld -y foo -trace-symbol=common -trace-symbol=hsymbol \ 
+# RUN: %t %t1 %t2 -o %t3 2>&1 | FileCheck -check-prefix=OBJECTRFOO %s
+# RUN: ld.lld -y foo -trace-symbol=common -trace-symbol=hsymbol \ 
----------------
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.


http://reviews.llvm.org/D21550