[PATCH] D142163: [LLD][ELF] Add --lto-export-symbol-list

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/LTO.cpp:262
+    bool useExports = config->ltoExportSymbols && !sym->isUsedInRegularObj &&
+                      objSym.isExecutable();
+    uint8_t binding = sym->computeBinding(useExports);
----------------
`objSym.isExecutable()` this condition is strange and does not fit into the spirit that we ignore symbol types for various symbol list features.
Can the user specify all data symbols?


================
Comment at: lld/ELF/Symbols.cpp:272
 
-uint8_t Symbol::computeBinding() const {
+uint8_t Symbol::computeBinding(bool useExports /*=false*/) const {
+  if (useExports && config->ltoExportSymbols && binding == STB_GLOBAL &&
----------------
This makes the Writer.cpp caller slow. Suggest: add a new function. I need to think more why we don't make the feature available to non-LTO use cases.


================
Comment at: lld/test/ELF/driver.test:74
 
+## Attempt to use --lto-export-symbol-list without -r
+# RUN: echo "{ };" > %t.list
----------------
Move this to the end of lto-export-symbol-list.ll.


================
Comment at: lld/test/ELF/lto/lto-export-symbol-list-unexpected-end.s:1
+# REQUIRES: x86
+
----------------
Move this to the end of `lto-export-symbol-list.ll`. Use `RUN: rm -rf %t && split-file %s %t && cd %t`.


================
Comment at: lld/test/ELF/lto/lto-export-symbol-list.ll:5
+; RUN: ld.lld %t1.o -r -o %t2.o
+; RUN: llvm-readobj --symbols %t2.o | FileCheck %s --check-prefixes=CHECK,NOLIST
+
----------------
`llvm-readobj -s` is not recommended for new tests (too verbose). Use `llvm-readelf -s` (see my changes rewriting some tests to use it)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142163/new/

https://reviews.llvm.org/D142163