[llvm-objcopy] Add --skip-symbol and --skip-symbols options #80873

kuilpd · 2024-02-06T17:01:31Z

Add --skip-symbol and --skip-symbols options that allow to skip symbols when executing other options that can change the symbol's name, binding or visibility, similar to an existing option --keep-symbol that keeps a symbol from being removed by other options.

llvmbot · 2024-02-06T17:02:07Z

@llvm/pr-subscribers-llvm-binary-utilities

Author: Ilia Kuklin (kuilpd)

Changes

Add an option to ignore (exclude) symbols when executing other options that can change the symbol's name, binding or visibility, similar to an existing option --keep-symbol that keeps a symbol from being removed by other options.

Full diff: https://github.com/llvm/llvm-project/pull/80873.diff

5 Files Affected:

(modified) llvm/include/llvm/ObjCopy/ELF/ELFConfig.h (+3)
(modified) llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp (+2)
(added) llvm/test/tools/llvm-objcopy/ELF/ignore-symbols.test (+41)
(modified) llvm/tools/llvm-objcopy/ObjcopyOptions.cpp (+9)
(modified) llvm/tools/llvm-objcopy/ObjcopyOpts.td (+14)

diff --git a/llvm/include/llvm/ObjCopy/ELF/ELFConfig.h b/llvm/include/llvm/ObjCopy/ELF/ELFConfig.h
index d77cb69b159db..52874ea55414d 100644
--- a/llvm/include/llvm/ObjCopy/ELF/ELFConfig.h
+++ b/llvm/include/llvm/ObjCopy/ELF/ELFConfig.h
@@ -9,6 +9,7 @@
 #ifndef LLVM_OBJCOPY_ELF_ELFCONFIG_H
 #define LLVM_OBJCOPY_ELF_ELFCONFIG_H
 
+#include "llvm/ObjCopy/CommonConfig.h"
 #include "llvm/Object/ELFTypes.h"
 
 namespace llvm {
@@ -18,6 +19,8 @@ namespace objcopy {
 struct ELFConfig {
   uint8_t NewSymbolVisibility = (uint8_t)ELF::STV_DEFAULT;
 
+  NameMatcher SymbolsToIgnore;
+
   // ELF entry point address expression. The input parameter is an entry point
   // address in the input ELF file. The entry address in the output file is
   // calculated with EntryExpr(input_address), when either --set-start or
diff --git a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
index b6d77d17bae36..f8c0413684f22 100644
--- a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
+++ b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
@@ -290,6 +290,8 @@ static Error updateAndRemoveSymbols(const CommonConfig &Config,
     return Error::success();
 
   Obj.SymbolTable->updateSymbols([&](Symbol &Sym) {
+    if (ELFConfig.SymbolsToIgnore.matches(Sym.Name))
+      return;
     // Common and undefined symbols don't make sense as local symbols, and can
     // even cause crashes if we localize those, so skip them.
     if (!Sym.isCommon() && Sym.getShndx() != SHN_UNDEF &&
diff --git a/llvm/test/tools/llvm-objcopy/ELF/ignore-symbols.test b/llvm/test/tools/llvm-objcopy/ELF/ignore-symbols.test
new file mode 100644
index 0000000000000..13a15b46038ee
--- /dev/null
+++ b/llvm/test/tools/llvm-objcopy/ELF/ignore-symbols.test
@@ -0,0 +1,41 @@
+
+# RUN: yaml2obj %s -o %t.o
+# RUN: echo 'foo[2-3]' > %t.ignore.regex
+
+# RUN: cp  %t.o  %t1.o
+# RUN: llvm-objcopy  %t1.o --localize-hidden --ignore-symbols=%t.ignore.regex --regex
+# RUN: llvm-readelf -s  %t1.o | FileCheck %s --check-prefix=SYMS
+# SYMS-DAG: LOCAL  HIDDEN      1 foo1
+# SYMS-DAG: GLOBAL HIDDEN      1 foo2
+# SYMS-DAG: GLOBAL HIDDEN      1 foo3
+
+# RUN: cp  %t.o  %t1.o
+# RUN: llvm-objcopy  %t1.o --localize-hidden --ignore-symbol=foo3
+# RUN: llvm-readelf -s  %t1.o | FileCheck %s --check-prefix=SYM
+# SYM-DAG: LOCAL  HIDDEN      1 foo1
+# SYM-DAG: LOCAL  HIDDEN      1 foo2
+# SYM-DAG: GLOBAL HIDDEN      1 foo3
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+Symbols:
+  - Name:    foo1
+    Section: .text
+    Binding:  STB_GLOBAL
+    Other:    [ STV_HIDDEN ]
+  - Name:    foo2
+    Section: .text
+    Binding:  STB_GLOBAL
+    Other:    [ STV_HIDDEN ]
+  - Name:    foo3
+    Section: .text
+    Binding:  STB_GLOBAL
+    Other:    [ STV_HIDDEN ]
diff --git a/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp b/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
index f15307181fad6..0238839b3221d 100644
--- a/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
+++ b/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
@@ -955,6 +955,15 @@ objcopy::parseObjcopyOptions(ArrayRef<const char *> RawArgsArr,
             addSymbolsFromFile(Config.SymbolsToKeep, DC.Alloc, Arg->getValue(),
                                SymbolMatchStyle, ErrorCallback))
       return std::move(E);
+  for (auto *Arg : InputArgs.filtered(OBJCOPY_ignore_symbol))
+    if (Error E = ELFConfig.SymbolsToIgnore.addMatcher(NameOrPattern::create(
+            Arg->getValue(), SymbolMatchStyle, ErrorCallback)))
+      return std::move(E);
+  for (auto *Arg : InputArgs.filtered(OBJCOPY_ignore_symbols))
+    if (Error E = addSymbolsFromFile(ELFConfig.SymbolsToIgnore, DC.Alloc,
+                                     Arg->getValue(), SymbolMatchStyle,
+                                     ErrorCallback))
+      return std::move(E);
   for (auto *Arg : InputArgs.filtered(OBJCOPY_add_symbol)) {
     Expected<NewSymbolInfo> SymInfo = parseNewSymbolInfo(Arg->getValue());
     if (!SymInfo)
diff --git a/llvm/tools/llvm-objcopy/ObjcopyOpts.td b/llvm/tools/llvm-objcopy/ObjcopyOpts.td
index ead8cd28d3877..8f6dd912a4fd5 100644
--- a/llvm/tools/llvm-objcopy/ObjcopyOpts.td
+++ b/llvm/tools/llvm-objcopy/ObjcopyOpts.td
@@ -196,6 +196,20 @@ defm keep_symbols
          "be repeated to read symbols from many files">,
       MetaVarName<"filename">;
 
+defm ignore_symbol : Eq<"ignore-symbol", "Do not change parameters of symbol <symbol> "
+                        "when executing other options that can change the symbol's "
+                        "name, binding or visibility">,
+                     MetaVarName<"symbol">;
+
+defm ignore_symbols
+    : Eq<"ignore-symbols",
+         "Reads a list of symbols from <filename> and runs as if "
+         "--ignore-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 dump_section
     : Eq<"dump-section",
          "Dump contents of section named <section> into file <file>">,

jh7370

I think many of the same comments I made in #80872 apply equally well to this PR, so I'm not going to list them all out here too. Please address them in this PR, and then I'll come back and review again

MaskRay · 2024-02-09T06:46:19Z

This needs motivation. I think Binutils maintainers will likely have a strong opinion about the name if they add a similar option. I suspect that --ignore-symbol may not be a name they like. If this option is indeed useful, you may file a feature request https://sourceware.org/bugzilla/enter_bug.cgi

jh7370 · 2024-02-16T09:14:56Z

This needs motivation. I think Binutils maintainers will likely have a strong opinion about the name if they add a similar option. I suspect that --ignore-symbol may not be a name they like. If this option is indeed useful, you may file a feature request https://sourceware.org/bugzilla/enter_bug.cgi

Before reviewing this further, I'd like the motivation explained as per this comment.

kuilpd · 2024-02-16T19:28:52Z

This needs motivation. I think Binutils maintainers will likely have a strong opinion about the name if they add a similar option. I suspect that --ignore-symbol may not be a name they like. If this option is indeed useful, you may file a feature request https://sourceware.org/bugzilla/enter_bug.cgi

Before reviewing this further, I'd like the motivation explained as per this comment.

This option is needed mostly to support cases where there is a need to change parameters of a vast number of symbols with exception of a few specific symbols. Writing a negative pattern is much simpler than many positive patterns that avoid a certain case. Plus, since there is an option --keep-symbol that does exactly this but for removing symbols, there should also be one that keeps symbols' parameters from being changed.

jh7370 · 2024-02-20T08:32:00Z

This needs motivation. I think Binutils maintainers will likely have a strong opinion about the name if they add a similar option. I suspect that --ignore-symbol may not be a name they like. If this option is indeed useful, you may file a feature request https://sourceware.org/bugzilla/enter_bug.cgi

Before reviewing this further, I'd like the motivation explained as per this comment.

This option is needed mostly to support cases where there is a need to change parameters of a vast number of symbols with exception of a few specific symbols. Writing a negative pattern is much simpler than many positive patterns that avoid a certain case. Plus, since there is an option --keep-symbol that does exactly this but for removing symbols, there should also be one that keeps symbols' parameters from being changed.

Thanks. This doesn't seem unreasonable to me. However, a) I don't like the name (people may think that --ignore-symbol can be used to prevent stripping of a symbol, for example, or similar), and b) you really should reach out to the binutils folk to get their opinions on the feature and its name.

@MaskRay, any comments on the motivation?

kuilpd · 2024-02-22T17:59:02Z

This option is needed mostly to support cases where there is a need to change parameters of a vast number of symbols with exception of a few specific symbols. Writing a negative pattern is much simpler than many positive patterns that avoid a certain case. Plus, since there is an option --keep-symbol that does exactly this but for removing symbols, there should also be one that keeps symbols' parameters from being changed.

Thanks. This doesn't seem unreasonable to me. However, a) I don't like the name (people may think that --ignore-symbol can be used to prevent stripping of a symbol, for example, or similar), and b) you really should reach out to the binutils folk to get their opinions on the feature and its name.

@MaskRay, any comments on the motivation?

I made a request to create an account on sourceware.org last Monday, but still haven't gotten any reply.
So now I wrote an email to binutils mailing list instead, hopefully we can get some feedback.

kuilpd · 2024-02-26T18:45:19Z

Thanks. This doesn't seem unreasonable to me. However, a) I don't like the name (people may think that --ignore-symbol can be used to prevent stripping of a symbol, for example, or similar), and b) you really should reach out to the binutils folk to get their opinions on the feature and its name.

So far got a reply suggesting to rename it to "--skip-symbols". Seems fair to me.

jh7370 · 2024-02-27T08:20:54Z

Thanks. This doesn't seem unreasonable to me. However, a) I don't like the name (people may think that --ignore-symbol can be used to prevent stripping of a symbol, for example, or similar), and b) you really should reach out to the binutils folk to get their opinions on the feature and its name.

So far got a reply suggesting to rename it to "--skip-symbols". Seems fair to me.

Thanks, --skip-symbols definitely feels like an improvement to me, as I think the meaning is subtly different and won't be as likely to be confused with an option to prevent stripping.

jh7370

Implementation basically looks good. You need to update the PR title and description for the new option names, since that is used in the final commit message.

As --skip-symbol is something you might expect to work for other formats (it might be implemented later), it should be added to the list of options that generate an error when a non-ELF format is detected in the (I think) config manager code.

You also should have a test case that tests multiple --skip-symbol options specified at the same time. Finally, I think there's value to test --skip-symbol + --set-symbol-visibility explicitly, to demonstrate their order.

llvm/docs/CommandGuide/llvm-objcopy.rst

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp

MaskRay · 2024-02-29T04:53:09Z

https://sourceware.org/bugzilla/show_bug.cgi?id=31000#c3 Nick has a comment whether we really need a filename variant option, since we can just use @xxx

kuilpd · 2024-02-29T18:45:56Z

https://sourceware.org/bugzilla/show_bug.cgi?id=31000#c3 Nick has a comment whether we really need a filename variant option, since we can just use @xxx

I added it to make it consistent with other options, pretty much all of them have a separate option to read symbols from a file. If someone wants, they can certainly use response files, but I don't know if it warrants the removal of these file reading options.

jh7370

https://sourceware.org/bugzilla/show_bug.cgi?id=31000#c3 Nick has a comment whether we really need a filename variant option, since we can just use @xxx

This thread refers to the --set-symbol-visibility option, not the --skip-symbols option in this ticket. Here, I think --skip-symbols makes sense, since it's not something you need any further flexibility with.

llvm/test/tools/llvm-objcopy/ELF/skip-symbols.test

llvm/docs/CommandGuide/llvm-objcopy.rst

llvm/test/tools/llvm-objcopy/ELF/skip-symbols.test

…sibility

… symbols

MaskRay · 2024-03-19T17:56:29Z

llvm/docs/CommandGuide/llvm-objcopy.rst

@@ -464,6 +464,19 @@ them.
 Read a list of symbols from <filename> and change their visibility to the
 specified value. Visibility values: default, internal, hidden, protected.

+.. option:: --skip-symbol <symbol>
+
+ Do not change the parameters of symbol ``<symbol>`` when executing other


Should this mention that this does not prevent removing symbols?

I don't think it needs to: it explicitly states the things that it impacts (name/binding/visibility). It's probably harmless to add it though.

MaskRay · 2024-03-19T17:58:17Z

The latest updated looks good

jh7370

According to the pre-merge checks, your test is failing.

jh7370 · 2024-03-20T09:04:31Z

llvm/docs/CommandGuide/llvm-objcopy.rst

@@ -464,6 +464,19 @@ them.
 Read a list of symbols from <filename> and change their visibility to the
 specified value. Visibility values: default, internal, hidden, protected.

+.. option:: --skip-symbol <symbol>
+
+ Do not change the parameters of symbol ``<symbol>`` when executing other


I don't think it needs to: it explicitly states the things that it impacts (name/binding/visibility). It's probably harmless to add it though.

jh7370 · 2024-03-20T09:13:38Z

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

+# RS-SYM-DAG: foo5
+
+## Check --skip-symbols functionality when changing symbol names.
+# RUN: echo -e "foo1 bar1\nfoo2 bar2" > %t.renames.list


This needs moving earlier (it might be why the test is failing...).

This needs moving earlier (it might be why the test is failing...).

You're right, the test didn't fail on my machine because the file still remained from previous runs.

jh7370

LGTM, thanks. The buildkite failures appear to be unrelated, so feel free to merge.

Add --skip-symbol and --skip-symbols options that allow to skip symbols when executing other options that can change the symbol's name, binding or visibility, similar to an existing option --keep-symbol that keeps a symbol from being removed by other options.

kuilpd requested review from MaskRay, jh7370 and avl-llvm February 6, 2024 17:01

kuilpd self-assigned this Feb 6, 2024

llvmbot added the llvm:binary-utilities label Feb 6, 2024

kuilpd added the tools:llvm-objcopy/strip label Feb 6, 2024

kuilpd requested a review from igorkudrin February 6, 2024 19:04

jh7370 requested changes Feb 8, 2024

View reviewed changes

kuilpd force-pushed the add-objcopy-ignore-symbol branch from ee12133 to c94c1f6 Compare February 12, 2024 15:34

jh7370 reviewed Feb 28, 2024

View reviewed changes

llvm/docs/CommandGuide/llvm-objcopy.rst Outdated Show resolved Hide resolved

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp Show resolved Hide resolved

kuilpd force-pushed the add-objcopy-ignore-symbol branch from 62a4b6f to ac06362 Compare February 28, 2024 17:49

kuilpd changed the title ~~[llvm-objcopy] Add llvm-objcopy option --ignore-symbol~~ [llvm-objcopy] Add --skip-symbol and --skip-symbols options Feb 28, 2024

kuilpd requested a review from jh7370 March 12, 2024 12:57

jh7370 reviewed Mar 14, 2024

View reviewed changes

kuilpd added 6 commits March 19, 2024 22:48

Add llvm-objcopy option --ignore-symbol

da84199

Adjust and expand ignore-symbols.test

a7b0edb

Update the llvm-objcopy documentation

85f031e

Add a test for checking various error cases

e9b8a8b

Adjust formatting and wording in tests

6491013

Use %errc_ENOENT for 'No such file or directory' message

55af35c

kuilpd added 13 commits March 19, 2024 22:48

Use a wildcard instead of a regex in one of the test cases

e25e5f2

Adjust test formatting

151029c

Rename the options to --skip-symbols(s)

7f65088

Merge tests into one file

55d3fd8

Added the options to release notes

4006d11

Address nits

10f6e33

Move the option to common config and add checks in config manager

da81eb3

Add a test to check both multiple uses and order with --set-symbol-vi…

6dd0135

…sibility

Rename skip-symbols.test

a808003

Adjust description and formatting

a470b13

Adjust the test to use multiple options and expand others to use more…

251e820

… symbols

Adjust the doc description, make it similar to --keep-symbols

5d48d70

Change test order

f997fd9

kuilpd force-pushed the add-objcopy-ignore-symbol branch from ac06362 to f997fd9 Compare March 19, 2024 17:50

MaskRay reviewed Mar 19, 2024

View reviewed changes

jh7370 reviewed Mar 20, 2024

View reviewed changes

Move creation of %t.renames.list earlier

4163575

jh7370 approved these changes Mar 21, 2024

View reviewed changes

kuilpd merged commit 4946cc3 into llvm:main Mar 21, 2024
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[llvm-objcopy] Add --skip-symbol and --skip-symbols options #80873

[llvm-objcopy] Add --skip-symbol and --skip-symbols options #80873

kuilpd commented Feb 6, 2024 •

edited

llvmbot commented Feb 6, 2024

jh7370 left a comment

MaskRay commented Feb 9, 2024 •

edited

jh7370 commented Feb 16, 2024

kuilpd commented Feb 16, 2024

jh7370 commented Feb 20, 2024

kuilpd commented Feb 22, 2024

kuilpd commented Feb 26, 2024

jh7370 commented Feb 27, 2024

jh7370 left a comment

MaskRay commented Feb 29, 2024

kuilpd commented Feb 29, 2024

jh7370 left a comment

MaskRay Mar 19, 2024

jh7370 Mar 20, 2024

MaskRay commented Mar 19, 2024

jh7370 left a comment

jh7370 Mar 20, 2024

jh7370 Mar 20, 2024

kuilpd Mar 20, 2024

jh7370 left a comment

[llvm-objcopy] Add --skip-symbol and --skip-symbols options #80873

[llvm-objcopy] Add --skip-symbol and --skip-symbols options #80873

Conversation

kuilpd commented Feb 6, 2024 • edited

llvmbot commented Feb 6, 2024

jh7370 left a comment

Choose a reason for hiding this comment

MaskRay commented Feb 9, 2024 • edited

jh7370 commented Feb 16, 2024

kuilpd commented Feb 16, 2024

jh7370 commented Feb 20, 2024

kuilpd commented Feb 22, 2024

kuilpd commented Feb 26, 2024

jh7370 commented Feb 27, 2024

jh7370 left a comment

Choose a reason for hiding this comment

MaskRay commented Feb 29, 2024

kuilpd commented Feb 29, 2024

jh7370 left a comment

Choose a reason for hiding this comment

MaskRay Mar 19, 2024

Choose a reason for hiding this comment

jh7370 Mar 20, 2024

Choose a reason for hiding this comment

MaskRay commented Mar 19, 2024

jh7370 left a comment

Choose a reason for hiding this comment

jh7370 Mar 20, 2024

Choose a reason for hiding this comment

jh7370 Mar 20, 2024

Choose a reason for hiding this comment

kuilpd Mar 20, 2024

Choose a reason for hiding this comment

jh7370 left a comment

Choose a reason for hiding this comment

kuilpd commented Feb 6, 2024 •

edited

MaskRay commented Feb 9, 2024 •

edited