[llvm-objcopy] Add few file processing directives

[jhenderson]

test/tools/llvm-objcopy/ELF/globalize.test

11 ↗

(On Diff #185712)

--regex here and in the other test cases you added below should be tested in addition to the basic behaviour of --globalize-symbols (i.e. --globalize-symbols should be the only switch for some tests). In particular, what you are testing is that --regex causes pattern matching, but not that no --regex doesn't cause pattern matching. That may actually be an issue with --regex in general, not specific to this test.

test/tools/llvm-objcopy/ELF/strip-symbol.test

9 ↗

(On Diff #185712)

Why is this tested under strip-symbol and not the other cases?

Related aside: this is a good example of where unit testing should be present because really we're testing the symbol reading from file function not the switch behaviour.

[evgeny777]

test/tools/llvm-objcopy/ELF/strip-symbol.test

9 ↗

(On Diff #185712)

Why is this tested under strip-symbol and not the other cases?

What's the point of doing this everywhere? We're using the same file parser for all cases

[jhenderson]

test/tools/llvm-objcopy/ELF/strip-symbol.test

9 ↗

(On Diff #185712)

My point is more that we are testing a detail of the parser in a test that doesn't obviously have anything to do with that parser. Tests should test one feature or behaviour or it becomes hard to determine at a glance why a test is failing, or what behaviours or features are under test. I would deem that the parser is itself a feature, and should be tested separately (with separate tests to show that clients use it correctly).

Let's imagine the (admittedly somewhat contrived case) that we decide that the behaviour of --strip-symbols should change, e.g. to not allow comments any more. That would imply that this particular case should be removed from this test. However, given the current state of this patch, that would mean that we don't have any testing for comments in the normal parser.

Two options would be either to add the comment behaviour testing to each user of the parser or to write a test that tests comment handling explicitly. As noted above, this would naturally be a unit test, but given the current state of llvm-objcopy that is not possible. An alternative would be one or more lit tests called something like "symbols-from-file.test" or whatever.

[evgeny777]

Better tests

[jhenderson]

test/tools/llvm-objcopy/ELF/globalize.test

14 ↗

(On Diff #185923)

This one still has --regex in, which I assume wasn't intentional?

[evgeny777]

Test case fix

[jhenderson]

LGTM.

llvm/trunk/test/tools/llvm-objcopy/ELF/globalize.test

 # RUN: yaml2obj %s > %t
 # RUN: llvm-objcopy --globalize-symbol Global \
 # RUN:   --globalize-symbol Local \
 # RUN:   --globalize-symbol Weak \
 # RUN:   --globalize-symbol WeakUndef %t %t2
 # RUN: llvm-readobj --symbols %t2 | FileCheck %s
 # RUN: llvm-objcopy --regex --globalize-symbol='.*' %t %t3
 # RUN: cmp %t2 %t3
+# RUN: echo " Global # global" > %t-list.txt
+# RUN: echo "Local" >> %t-list.txt
+# RUN: echo "Weak" >> %t-list.txt
+# RUN: echo "WeakUndef" >> %t-list.txt
+# RUN: echo " # comment " >> %t-list.txt
+# RUN: llvm-objcopy --globalize-symbols %t-list.txt %t %t4
+# RUN: cmp %t2 %t4
+# RUN: echo ".+ # .+ " > %t-list2.txt
+# RUN: llvm-objcopy --regex --globalize-symbols %t-list2.txt %t %t5
+# RUN: cmp %t2 %t5
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:

llvm/trunk/test/tools/llvm-objcopy/ELF/localize.test

 # RUN: yaml2obj %s > %t
 # RUN: llvm-objcopy \
 # RUN:     --localize-symbol Global \
 # RUN:     -L GlobalUndef \
 # RUN:     -L Local \
 # RUN:     -L Weak \
 # RUN:     -L GlobalCommon \
 # RUN:     %t %t2
 # RUN: llvm-readobj --symbols %t2 | FileCheck %s
 # RUN: llvm-objcopy --regex --localize-symbol='.*' %t %t3
 # RUN: cmp %t2 %t3
+# RUN: echo "  Global # comment  " > %t-list.txt
+# RUN: echo "GlobalUndef" >> %t-list.txt
+# RUN: echo "Local" >> %t-list.txt
+# RUN: echo "Weak" >> %t-list.txt
+# RUN: echo "GlobalCommon" >> %t-list.txt
+# RUN: echo " # comment " >> %t-list.txt
+# RUN: llvm-objcopy --localize-symbols %t-list.txt %t %t4
+# RUN: cmp %t2 %t4
+# RUN: echo "  .*al.* # Global + Local + GlobalCommon " > %t-list2.txt
+# RUN: echo "Weak" >> %t-list2.txt
+# RUN: llvm-objcopy --regex --localize-symbols %t-list2.txt %t %t5
+# RUN: cmp %t2 %t5
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:

llvm/trunk/test/tools/llvm-objcopy/ELF/strip-symbol.test

 # RUN: yaml2obj %s > %t
 # RUN: llvm-objcopy --strip-symbol baz -N bar %t %t2
 # RUN: llvm-readobj --symbols --sections %t2 | FileCheck %s
 # RUN: llvm-strip --strip-symbol baz -N bar %t -o %t3
 # RUN: cmp %t2 %t3
 # RUN: llvm-strip --regex --strip-symbol '^b.*' -N bar %t -o %t4
 # RUN: cmp %t3 %t4
+# RUN: echo " bar # bar" > %t-list.txt
+# RUN: echo " baz # baz" >> %t-list.txt
+# RUN: echo " # no symbol" >> %t-list.txt
+# RUN: llvm-objcopy --strip-symbols %t-list.txt %t %t5
+# RUN: cmp %t3 %t5
+# RUN: echo "b.* # bar & baz" > %t-list2.txt
+# RUN: llvm-objcopy --regex --strip-symbols %t-list2.txt %t %t6
+# RUN: cmp %t3 %t6
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:

llvm/trunk/test/tools/llvm-objcopy/ELF/weaken.test

 # RUN: yaml2obj %s > %t
 # RUN: llvm-objcopy --weaken-symbol Global -W Local -W Weak %t %t2
 # RUN: llvm-readobj --symbols %t2 | FileCheck %s
 # RUN: llvm-objcopy --regex --weaken-symbol='.*' %t %t3
 # RUN: cmp %t2 %t3
+# RUN: echo "  Global  # comment" > %t-list.txt
+# RUN: echo "Local" >> %t-list.txt
+# RUN: echo "Weak" >> %t-list.txt
+# RUN: echo " # comment" >> %t-list.txt
+# RUN: llvm-objcopy --weaken-symbols %t-list.txt %t %t4
+# RUN: cmp %t2 %t4
+# RUN: echo ".*al # Global + Local == .*al" > %t-list2.txt
+# RUN: echo "Weak # weak" >> %t-list2.txt
+# RUN: llvm-objcopy --regex --weaken-symbols %t-list2.txt %t %t5
+# RUN: cmp %t2 %t5
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:

llvm/trunk/tools/llvm-objcopy/CopyConfig.cpp

 
 static const MachineInfo &getOutputFormatMachineInfo(StringRef Format) {
   auto Iter = OutputFormatMap.find(Format);
   if (Iter == std::end(OutputFormatMap))
     error("Invalid output format: '" + Format + "'");
   return Iter->getValue();
 }
 
-static void addGlobalSymbolsFromFile(std::vector<NameOrRegex> &Symbols,
-                                     BumpPtrAllocator &Alloc,
-                                     StringRef Filename, bool UseRegex) {
+static void addSymbolsFromFile(std::vector<NameOrRegex> &Symbols,
+                               BumpPtrAllocator &Alloc, StringRef Filename,
+                               bool UseRegex) {
   StringSaver Saver(Alloc);
   SmallVector<StringRef, 16> Lines;
   auto BufOrErr = MemoryBuffer::getFile(Filename);
   if (!BufOrErr)
     reportError(Filename, BufOrErr.getError());
 
   BufOrErr.get()->getBuffer().split(Lines, '\n');
   for (StringRef Line : Lines) {
Config.DiscardMode =
             ? DiscardType::All
             : DiscardType::Locals;
   Config.OnlyKeepDebug = InputArgs.hasArg(OBJCOPY_only_keep_debug);
   Config.KeepFileSymbols = InputArgs.hasArg(OBJCOPY_keep_file_symbols);
   Config.DecompressDebugSections =
       InputArgs.hasArg(OBJCOPY_decompress_debug_sections);
   for (auto Arg : InputArgs.filtered(OBJCOPY_localize_symbol))
     Config.SymbolsToLocalize.emplace_back(Arg->getValue(), UseRegex);
+  for (auto Arg : InputArgs.filtered(OBJCOPY_localize_symbols))
+    addSymbolsFromFile(Config.SymbolsToLocalize, DC.Alloc, Arg->getValue(),
+                       UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_global_symbol))
     Config.SymbolsToKeepGlobal.emplace_back(Arg->getValue(), UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_global_symbols))
-    addGlobalSymbolsFromFile(Config.SymbolsToKeepGlobal, DC.Alloc,
-                             Arg->getValue(), UseRegex);
+    addSymbolsFromFile(Config.SymbolsToKeepGlobal, DC.Alloc, Arg->getValue(),
+                       UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_globalize_symbol))
     Config.SymbolsToGlobalize.emplace_back(Arg->getValue(), UseRegex);
+  for (auto Arg : InputArgs.filtered(OBJCOPY_globalize_symbols))
+    addSymbolsFromFile(Config.SymbolsToGlobalize, DC.Alloc, Arg->getValue(),
+                       UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_weaken_symbol))
     Config.SymbolsToWeaken.emplace_back(Arg->getValue(), UseRegex);
+  for (auto Arg : InputArgs.filtered(OBJCOPY_weaken_symbols))
+    addSymbolsFromFile(Config.SymbolsToWeaken, DC.Alloc, Arg->getValue(),
+                       UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_strip_symbol))
     Config.SymbolsToRemove.emplace_back(Arg->getValue(), UseRegex);
+  for (auto Arg : InputArgs.filtered(OBJCOPY_strip_symbols))
+    addSymbolsFromFile(Config.SymbolsToRemove, DC.Alloc, Arg->getValue(),
+                       UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_symbol))
     Config.SymbolsToKeep.emplace_back(Arg->getValue(), UseRegex);
 
   Config.DeterministicArchives = InputArgs.hasFlag(
       OBJCOPY_enable_deterministic_archives,
       OBJCOPY_disable_deterministic_archives, /*default=*/true);
 
   Config.PreserveDates = InputArgs.hasArg(OBJCOPY_preserve_dates);

llvm/trunk/tools/llvm-objcopy/ObjcopyOpts.td

: Flag<["-", "--"], "extract-dwo">,
           "Remove all sections that are not DWARF .dwo sections from file">;
 
 def localize_hidden
     : Flag<["-", "--"], "localize-hidden">,
       HelpText<
           "Mark all symbols that have hidden or internal visibility as local">;
 defm localize_symbol : Eq<"localize-symbol", "Mark <symbol> as local">,
                        MetaVarName<"symbol">;
+defm localize_symbols
+    : Eq<"localize-symbols",
+         "Reads a list of symbols from <filename> and marks them local.">,
+      MetaVarName<"filename">;
+
 def L : JoinedOrSeparate<["-"], "L">, Alias<localize_symbol>;
 
 defm globalize_symbol : Eq<"globalize-symbol", "Mark <symbol> as global">,
                         MetaVarName<"symbol">;
+
+defm globalize_symbols
+    : Eq<"globalize-symbols",
+         "Reads a list of symbols from <filename> and marks them global.">,
+      MetaVarName<"filename">;
+
 defm keep_global_symbol
     : Eq<"keep-global-symbol",
          "Convert all symbols except <symbol> to local. May be repeated to "
          "convert all except a set of symbols to local.">,
       MetaVarName<"symbol">;
 def G : JoinedOrSeparate<["-"], "G">, Alias<keep_global_symbol>;
 
 defm keep_global_symbols
     : Eq<"keep-global-symbols",
          "Reads a list of symbols from <filename> and runs as if "
          "--keep-global-symbol=<symbol> is set for each one. <filename> "
          "contains one symbol per line and may contain comments beginning with "
          "'#'. Leading and trailing whitespace is stripped from each line. May "
          "be repeated to read symbols from many files.">,
       MetaVarName<"filename">;
 
 defm weaken_symbol : Eq<"weaken-symbol", "Mark <symbol> as weak">,
                      MetaVarName<"symbol">;
+defm weaken_symbols
+    : Eq<"weaken-symbols",
+         "Reads a list of symbols from <filename> and marks them weak.">,
+      MetaVarName<"filename">;
+
 def W : JoinedOrSeparate<["-"], "W">, Alias<weaken_symbol>;
 def weaken : Flag<["-", "--"], "weaken">,
              HelpText<"Mark all global symbols as weak">;
 
 def discard_locals : Flag<["-", "--"], "discard-locals">,
                      HelpText<"Remove compiler-generated local symbols, (e.g. "
                               "symbols starting with .L)">;
 def X : Flag<["-"], "X">, Alias<discard_locals>;
 
 def discard_all
     : Flag<["-", "--"], "discard-all">,
       HelpText<"Remove all local symbols except file and section symbols">;
 def x : Flag<["-"], "x">, Alias<discard_all>;
 defm strip_symbol : Eq<"strip-symbol", "Remove symbol <symbol>">,
                     MetaVarName<"symbol">;
+defm strip_symbols
+    : Eq<"strip-symbols",
+         "Reads a list of symbols from <filename> and removes them.">,
+      MetaVarName<"filename">;
+
 def N : JoinedOrSeparate<["-"], "N">, Alias<strip_symbol>;
 defm keep_symbol : Eq<"keep-symbol", "Do not remove symbol <symbol>">,
                    MetaVarName<"symbol">;
 def K : JoinedOrSeparate<["-"], "K">, Alias<keep_symbol>;
 def only_keep_debug
     : Flag<["-", "--"], "only-keep-debug">,
       HelpText<"Currently ignored. Only for compatibility with GNU objcopy.">;
 def keep_file_symbols : Flag<["-", "--"], "keep-file-symbols">,