[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

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

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

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

I agree it was confusing. What about RetainFile ?

76

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

Done. Thanks.

[ruiu]

ELF/Config.h

76

Sort.

ELF/Driver.cpp

477

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

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

498–499

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

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

[grimar]

• Addressed review comments.

ELF/Config.h

76

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

Done.

498–499

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

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

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

[grimar]

• Addressed review comments.

ELF/Driver.cpp

498–499

It would assert, you right. Fixed, thanks !

[ruiu]

ELF/Driver.cpp

480–483

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

Looks so..

[ruiu]

ELF/Options.td

4–5

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

ELF/Writer.cpp

6

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

8

Does this compile? You are still using CachedHashString.

[grimar]

ELF/Writer.cpp

8

Unusual, but it was. I think because of

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

I'll fix.

[grimar]

• Addressed review comments.

ELF/Options.td

4–5

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

6

Done.

[grimar]

• clang-formatted comment.

[ruiu]

LGTM with a nit.

ELF/Writer.cpp

427–428

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.

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/CachedHashString.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.

 
 // This struct contains the global configuration for the linker.
 // Most fields are direct mapping from the command line options
 // and such fields have the same name as the corresponding options.
 // Most fields are initialized by the driver.
 struct Configuration {
   InputFile *FirstElf = nullptr;
   uint8_t OSABI = 0;
+  llvm::StringSet<> RetainSymbolsFile;
   llvm::DenseMap<llvm::CachedHashStringRef, unsigned> SymbolOrderingFile;
   llvm::StringMap<uint64_t> SectionStartMap;
   llvm::StringRef DynamicLinker;
   llvm::StringRef Entry;
   llvm::StringRef Emulation;
   llvm::StringRef Fini;
   llvm::StringRef Init;
   llvm::StringRef LTOAAPipeline;

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 SmallVector<StringRef, 0> getLines(MemoryBufferRef MB) {
+  SmallVector<StringRef, 0> Arr;
+  MB.getBuffer().split(Arr, '\n', INT32_MAX, false);
+  return Arr;
+}
+
 // 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)
+  for (StringRef S : getLines(MB))
     Config->SymbolOrderingFile.insert({CachedHashStringRef(S.trim()), 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.trim());
+}
+
 // 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;
   for (auto *Arg : Args.filtered(OPT_rpath))
     RPaths.push_back(Arg->getValue());
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)) {
+    if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue())) {
+      Config->Discard = DiscardPolicy::RetainFile;
+      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)) {

ELF/Options.td

 include "llvm/Option/OptParser.td"
 
 // For options whose names are multiple letters, either one dash or
 // two can precede the option name except those that start with 'o'.
 class F<string name>: Flag<["--", "-"], name>;
 class J<string name>: Joined<["--", "-"], name>;
 class S<string name>: Separate<["--", "-"], name>;
 class JS<string name>: JoinedOrSeparate<["--", "-"], name>;
 
 def auxiliary: S<"auxiliary">, HelpText<"Set DT_AUXILIARY field to the specified name">;
 
 def Bsymbolic: F<"Bsymbolic">, HelpText<"Bind defined symbols locally">;
 

 
 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">;

ELF/Writer.cpp

 //===- Writer.cpp ---------------------------------------------------------===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Writer.h"
 #include "Config.h"
 #include "LinkerScript.h"
 #include "OutputSections.h"
 #include "Relocations.h"
 #include "Strings.h"
 #include "SymbolTable.h"
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 used, we do not keep include unlisted symbols.
+  if (Config->Discard == DiscardPolicy::RetainFile)
+    if (!Config->RetainSymbolsFile.count(CachedHashString(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))

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:       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