[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

3

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

3

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

28

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

3

I agree it was confusing. What about RetainFile ?

3

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

28

Done. Thanks.

[ruiu]

ELF/Config.h

3

Sort.

ELF/Driver.cpp

28

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.

31

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

32

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

33–34

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

[grimar]

• Addressed review comments.

ELF/Config.h

3

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

28

Done.

32

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

33–34

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'));
}

[ruiu]

ELF/Driver.cpp

33–34

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

[grimar]

• Addressed review comments.

ELF/Driver.cpp

33–34

It would assert, you right. Fixed, thanks !

[ruiu]

ELF/Driver.cpp

31–34

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

31–34

Looks so..

[ruiu]

ELF/Options.td

179–180

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

ELF/Writer.cpp

425

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

427

Does this compile? You are still using CachedHashString.

[grimar]

ELF/Writer.cpp

427

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

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

Done.

[grimar]

• clang-formatted comment.

[ruiu]

LGTM with a nit.

ELF/Writer.cpp

8–9

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

ELF/Driver.cpp

 #include "Threads.h"
 #include "Writer.h"
 #include "lld/Config/Version.h"
 #include "lld/Driver/Driver.h"
 #include "lld/Support/Memory.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cstdlib>
 #include <utility>
 
 using namespace llvm;
 using namespace llvm::ELF;
 using namespace llvm::object;
 using namespace llvm::sys;
 
 using namespace lld;
 using namespace lld::elf;
 
 Configuration *elf::Config;
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({CachedHashStringRef(S.trim()), I++});
+  for (StringRef S : getLines(MB))
+    Config->SymbolOrderingFile.insert({CachedHashStringRef(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)) {

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

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"
 #include "SyntheticSections.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