[ELF] - Implemented --retain-symbols-file option

[grimar]

Summary

--retain-symbols-file=filename
Retain only the symbols listed in the file filename, discarding all others. filename is simply a flat file, with one symbol name per line. This option is especially useful in environments (such as VxWorks) where a large global symbol table is accumulated gradually, to conserve run-time memory.

Note: though documentation says "--retain-symbols-file does not discard undefined symbols, or symbols needed for relocations.", both bfd and gold do that, and this patch too, like testcase show.

[grimar]

This is PR31334.

[emaste]

How do we handle combinations of related options?

ELF/Config.h

39 ↗

(On Diff #81238)

DiscardPolicy File seems confusing to me - in that it seems File would list the symbols to discard. Not sure of a good word to encompass Discard/Retain though.

[ruiu]

ELF/Config.h

76 ↗

(On Diff #81238)

Using CachedHashStringRef doesn't make sense here. You still compute string hash values before looking up strings, so it doesn't really cache anything (unless you have a gigantic --retain-symbols-file which is unrealistic). Just use a string set.

ELF/Driver.cpp

648 ↗

(On Diff #81238)

If --retain-symbol-file is used, we'll retail only the symbols listed in the file and discard all others.

[grimar]

• Addressed review comments.

ELF/Config.h

39 ↗

(On Diff #81238)

I agree it was confusing. What about RetainFile ?

76 ↗

(On Diff #81238)

Ok. Currently we know 2 users of this functionality. One of them uses /dev/null as file. That was done I think as workaround for gold which in case of empty file thinks it was not used, what is wrong and inconsistent with bfd.
So I would probably agree now with statement about "unrealistic", though I implemented that initially assuming it is possible to have such file.

ELF/Driver.cpp

648 ↗

(On Diff #81238)

Done. Thanks.

[ruiu]

ELF/Config.h

76 ↗

(On Diff #81238)

Sort.

ELF/Driver.cpp

477 ↗

(On Diff #81356)

Can you return std::vector<StringRef> instead of SmallVector? We don't use SmallVector that much in our code, so I'd want to avoid using it as part of a function interface.

480 ↗

(On Diff #81356)

To return a std::vector, I believe you can just do return {Arr.begin(), Arr.end()}.

498–499 ↗

(On Diff #81356)

I think that you are inserting an empty string to the set because usually a text file ends with "\n", so the last line is empty. I'd do trim() and remove blank lines in getLines().

652 ↗

(On Diff #81356)

This flag should be set to true whether we readFile succeded or not.

[grimar]

• Addressed review comments.

ELF/Config.h

76 ↗

(On Diff #81238)

Not sure what is expected place then. I think we just put variable here in sorted groups, since there is no other StrringSets, I assumed it is sorted.
I moved that line after last llvm::StringRef then.

ELF/Driver.cpp

477 ↗

(On Diff #81356)

Done.

498–499 ↗

(On Diff #81356)

No I am not, last argument of split() does all job:

MB.getBuffer().split(Arr, '\n', INT32_MAX, /*KeepEmpty*/ false);

Also set would assert that:

std::pair<typename base::iterator, bool> insert(StringRef Key) {
  assert(!Key.empty());
  return base::insert(std::make_pair(Key, '\0'));
}

652 ↗

(On Diff #81356)

I think we error out if something is wrong inside readFile() anyways. Though I did that change to
make code shorter.

[ruiu]

ELF/Driver.cpp

498–499 ↗

(On Diff #81356)

What if the file ends with "\n " (newline and an space character)?

[grimar]

• Addressed review comments.

ELF/Driver.cpp

498–499 ↗

(On Diff #81356)

It would assert, you right. Fixed, thanks !

[ruiu]

ELF/Driver.cpp

480–483 ↗

(On Diff #81717)

I think plain, boring, non-higher-order code is easier to read.

for (StringRef S : Arr) {
  S = S.trim();
  if (!S.empty())
    Res.push_back(S);
}

[grimar]

• Addressed review comment.

ELF/Driver.cpp

480–483 ↗

(On Diff #81717)

Looks so..

[ruiu]

ELF/Options.td

179–180 ↗

(On Diff #81718)

I guess you also want to add S<"retain-symbols-file">? Please check GNU ld if it accepts that.
.

ELF/Writer.cpp

425 ↗

(On Diff #81718)

// If --retain-symbols-file is given, we'll keep only symbols listed in that file.

427 ↗

(On Diff #81718)

Does this compile? You are still using CachedHashString.

[grimar]

ELF/Writer.cpp

427 ↗

(On Diff #81718)

Unusual, but it was. I think because of

operator StringRef() const { return val(); }

I'll fix.

[grimar]

• Addressed review comments.

ELF/Options.td

179–180 ↗

(On Diff #81718)

Yes, bfd accepts S too, with single or double "-":

umb@ubuntu:~/LLVM/build$ ld.bfd --retain-symbols-file xx
ld.bfd: xx: No such file or directory
ld.bfd: no input files
umb@ubuntu:~/LLVM/build$ ld.bfd -retain-symbols-file xx
ld.bfd: xx: No such file or directory
ld.bfd: no input files
umb@ubuntu:~/LLVM/build$ ld.bfd -retain-symbols-file=xx
ld.bfd: xx: No such file or directory
ld.bfd: no input files
umb@ubuntu:~/LLVM/build$ ld.bfd --retain-symbols-file=xx
ld.bfd: xx: No such file or directory
ld.bfd: no input files

I updated testcase to check both forms.

ELF/Writer.cpp

425 ↗

(On Diff #81718)

Done.

[grimar]

• clang-formatted comment.

[ruiu]

LGTM with a nit.

ELF/Writer.cpp

427–428 ↗

(On Diff #81720)

We usually use && instead of two ifs. I'd think I've pointed out too many small errors in this review. Maybe you want to take a little bit more time to read your entire patch before requesting another round of code review? I usually read my patch many times from top to bottom until it looks perfect to me before sending it to review.

lld/trunk/ELF/Config.h

 //===- Config.h -------------------------------------------------*- C++ -*-===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_ELF_CONFIG_H
 #define LLD_ELF_CONFIG_H
 
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/ELF.h"
 
 #include <vector>
 
 namespace lld {
 namespace elf {
 
 class InputFile;
 struct Symbol;
 
 enum ELFKind {
   ELFNoneKind,
   ELF32LEKind,
   ELF32BEKind,
   ELF64LEKind,
   ELF64BEKind
 };
 
 // For --build-id.
 enum class BuildIdKind { None, Fast, Md5, Sha1, Hexstring, Uuid };
 
-// For --discard-{all,locals,none}.
-enum class DiscardPolicy { Default, All, Locals, None };
+// For --discard-{all,locals,none} and --retain-symbols-file.
+enum class DiscardPolicy { Default, All, Locals, RetainFile, None };
 
 // For --strip-{all,debug}.
 enum class StripPolicy { None, All, Debug };
 
 // For --unresolved-symbols.
 enum class UnresolvedPolicy { NoUndef, ReportError, Warn, Ignore };
 
 // For --sort-section and linkerscript sorting rules.
struct Configuration {
   llvm::StringRef Emulation;
   llvm::StringRef Fini;
   llvm::StringRef Init;
   llvm::StringRef LTOAAPipeline;
   llvm::StringRef LTONewPmPasses;
   llvm::StringRef OutputFile;
   llvm::StringRef SoName;
   llvm::StringRef Sysroot;
+  llvm::StringSet<> RetainSymbolsFile;
   std::string RPath;
   std::vector<VersionDefinition> VersionDefinitions;
   std::vector<llvm::StringRef> AuxiliaryList;
   std::vector<llvm::StringRef> SearchPaths;
   std::vector<llvm::StringRef> Undefined;
   std::vector<SymbolVersion> VersionScriptGlobals;
   std::vector<SymbolVersion> VersionScriptLocals;
   std::vector<uint8_t> BuildIdVector;

lld/trunk/ELF/Driver.cpp

if (S == "alignment")
     return SortSectionPolicy::Alignment;
   if (S == "name")
     return SortSectionPolicy::Name;
   if (!S.empty())
     error("unknown --sort-section rule: " + S);
   return SortSectionPolicy::Default;
 }
 
+static std::vector<StringRef> getLines(MemoryBufferRef MB) {
+  std::vector<StringRef> Ret;
+  SmallVector<StringRef, 0> Arr;
+  MB.getBuffer().split(Arr, '\n');
+  for (StringRef S : Arr) {
+    S = S.trim();
+    if (!S.empty())
+      Ret.push_back(S);
+  }
+  return Ret;
+}
+
 // Parse the --symbol-ordering-file argument. File has form:
 // symbolName1
 // [...]
 // symbolNameN
 static void parseSymbolOrderingList(MemoryBufferRef MB) {
   unsigned I = 0;
-  SmallVector<StringRef, 0> Arr;
-  MB.getBuffer().split(Arr, '\n');
-  for (StringRef S : Arr)
-    Config->SymbolOrderingFile.insert({S.trim(), I++});
+  for (StringRef S : getLines(MB))
+    Config->SymbolOrderingFile.insert({S, I++});
+}
+
+// Parse the --retain-symbols-file argument. File has form:
+// symbolName1
+// [...]
+// symbolNameN
+static void parseRetainSymbolsList(MemoryBufferRef MB) {
+  for (StringRef S : getLines(MB))
+    Config->RetainSymbolsFile.insert(S);
 }
 
 // Initializes Config members by the command line options.
 void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   for (auto *Arg : Args.filtered(OPT_L))
     Config->SearchPaths.push_back(Arg->getValue());
 
   std::vector<StringRef> RPaths;
void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   if (auto *Arg = Args.getLastArg(OPT_dynamic_list))
     if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue()))
       readDynamicList(*Buffer);
 
   if (auto *Arg = Args.getLastArg(OPT_symbol_ordering_file))
     if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue()))
       parseSymbolOrderingList(*Buffer);
 
+  // If --retain-symbol-file is used, we'll retail only the symbols listed in
+  // the file and discard all others.
+  if (auto *Arg = Args.getLastArg(OPT_retain_symbols_file)) {
+    Config->Discard = DiscardPolicy::RetainFile;
+    if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue()))
+      parseRetainSymbolsList(*Buffer);
+  }
+
   for (auto *Arg : Args.filtered(OPT_export_dynamic_symbol))
     Config->VersionScriptGlobals.push_back(
         {Arg->getValue(), /*IsExternCpp*/ false, /*HasWildcard*/ false});
 
   // Dynamic lists are a simplified linker script that doesn't need the
   // "global:" and implicitly ends with a "local:*". Set the variables needed to
   // simulate that.
   if (Args.hasArg(OPT_dynamic_list) || Args.hasArg(OPT_export_dynamic_symbol)) {

lld/trunk/ELF/Options.td

 
 def reproduce: S<"reproduce">,
   HelpText<"Dump linker invocation and input files for debugging">;
 
 def rpath: S<"rpath">, HelpText<"Add a DT_RUNPATH to the output">;
 
 def relocatable: F<"relocatable">, HelpText<"Create relocatable object file">;
 
+def retain_symbols_file: J<"retain-symbols-file=">, MetaVarName<"<file>">,
+  HelpText<"Retain only the symbols listed in the file">;
+
 def script: S<"script">, HelpText<"Read linker script">;
 
 def section_start: S<"section-start">, MetaVarName<"<address>">,
   HelpText<"Set address of section">;
 
 def shared: F<"shared">, HelpText<"Build a shared object">;
 
 def soname: J<"soname=">, HelpText<"Set DT_SONAME">;

 def alias_hash_style_hash_style: J<"hash-style=">, Alias<hash_style>;
 def alias_init_init: J<"init=">, Alias<init>;
 def alias_l__library: J<"library=">, Alias<l>;
 def alias_omagic: Flag<["-"], "N">, Alias<omagic>;
 def alias_o_output: Joined<["--"], "output=">, Alias<o>;
 def alias_o_output2 : Separate<["--"], "output">, Alias<o>;
 def alias_pie_pic_executable: F<"pic-executable">, Alias<pie>;
 def alias_relocatable_r: Flag<["-"], "r">, Alias<relocatable>;
+def alias_retain_symbols_file: S<"retain-symbols-file">, Alias<retain_symbols_file>;
 def alias_rpath_R: JoinedOrSeparate<["-"], "R">, Alias<rpath>;
 def alias_rpath_rpath: J<"rpath=">, Alias<rpath>;
 def alias_script_T: JoinedOrSeparate<["-"], "T">, Alias<script>;
 def alias_shared_Bshareable: F<"Bshareable">, Alias<shared>;
 def alias_soname_h: JoinedOrSeparate<["-"], "h">, Alias<soname>;
 def alias_soname_soname: S<"soname">, Alias<soname>;
 def alias_sort_section: J<"sort-section=">, Alias<sort_section>;
 def alias_script: J<"script=">, Alias<script>;

lld/trunk/ELF/Writer.cpp

static bool shouldKeepInSymtab(InputSectionBase<ELFT> *Sec, StringRef SymName,
 
   return !Sec || !(Sec->Flags & SHF_MERGE);
 }
 
 template <class ELFT> static bool includeInSymtab(const SymbolBody &B) {
   if (!B.isLocal() && !B.symbol()->IsUsedInRegularObj)
     return false;
 
+  // If --retain-symbols-file is given, we'll keep only symbols listed in that
+  // file.
+  if (Config->Discard == DiscardPolicy::RetainFile &&
+      !Config->RetainSymbolsFile.count(B.getName()))
+    return false;
+
   if (auto *D = dyn_cast<DefinedRegular<ELFT>>(&B)) {
     // Always include absolute symbols.
     if (!D->Section)
       return true;
     // Exclude symbols pointing to garbage-collected sections.
     if (!D->Section->Live)
       return false;
     if (auto *S = dyn_cast<MergeInputSection<ELFT>>(D->Section))

lld/trunk/test/ELF/retain-symbols-file.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t
+# RUN: echo "bar" > %t_retain.txt
+# RUN: echo "foo" >> %t_retain.txt
+# RUN: ld.lld -shared --retain-symbols-file=%t_retain.txt %t -o %t2
+# RUN: llvm-readobj -s -sd -t %t2 | FileCheck %s
+
+## Check separate form.
+# RUN: ld.lld -shared --retain-symbols-file %t_retain.txt %t -o %t2
+# RUN: llvm-readobj -s -sd -t %t2 | FileCheck %s
+
+# CHECK:       Symbols [
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name:  (0)
+# CHECK:        Symbol {
+# CHECK-NEXT:     Name: bar
+# CHECK:        Symbol {
+# CHECK-NEXT:     Name: foo
+# CHECK-NOT:    Symbol
+
+.text
+.globl _start
+_start:
+call zed@PLT
+call und@PLT
+
+.globl foo
+.type foo,@function
+foo:
+retq
+
+.globl bar
+.type bar,@function
+bar:
+retq
+
+.globl zed
+.type zed,@function
+zed:
+retq
+
+.type loc,@function
+loc:
+retq