[llvm-objcopy] Add --set-symbol-visibility and --set-symbols-visibility options #80872

kuilpd · 2024-02-06T16:52:43Z

Add options --set-symbol-visibility and --set-symbols-visibility to manually change the visibility of symbols.

There is already an option to set the visibility of newly added symbols via --add-symbol and --new-symbol-visibility. This option will allow to change the visibility of already existing symbols.

llvmbot · 2024-02-06T16:53:14Z

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

Author: Ilia Kuklin (kuilpd)

Changes

Add an option to manually set the visibility of symbols.

There is already an option to set the visibility of newly added symbols via --add-symbol and --new-symbol-visibility. This option will allow to change the visibility of already existing symbols.

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

5 Files Affected:

(modified) llvm/include/llvm/ObjCopy/ELF/ELFConfig.h (+4)
(modified) llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp (+3)
(added) llvm/test/tools/llvm-objcopy/ELF/set-visibility.test (+139)
(modified) llvm/tools/llvm-objcopy/ObjcopyOptions.cpp (+43)
(modified) llvm/tools/llvm-objcopy/ObjcopyOpts.td (+11)

diff --git a/llvm/include/llvm/ObjCopy/ELF/ELFConfig.h b/llvm/include/llvm/ObjCopy/ELF/ELFConfig.h
index d77cb69b159db..b3efa3205e376 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,9 @@ namespace objcopy {
 struct ELFConfig {
   uint8_t NewSymbolVisibility = (uint8_t)ELF::STV_DEFAULT;
 
+  NameMatcher SymbolsToSetVisibility;
+  uint8_t SetVisibilityType = ELF::STV_DEFAULT;
+
   // 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..b768b7c5d05d7 100644
--- a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
+++ b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
@@ -290,6 +290,9 @@ static Error updateAndRemoveSymbols(const CommonConfig &Config,
     return Error::success();
 
   Obj.SymbolTable->updateSymbols([&](Symbol &Sym) {
+    if (ELFConfig.SymbolsToSetVisibility.matches(Sym.Name) &&
+        Sym.getShndx() != SHN_UNDEF)
+      Sym.Visibility = ELFConfig.SetVisibilityType;
     // 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/set-visibility.test b/llvm/test/tools/llvm-objcopy/ELF/set-visibility.test
new file mode 100644
index 0000000000000..bc1c92021c4fe
--- /dev/null
+++ b/llvm/test/tools/llvm-objcopy/ELF/set-visibility.test
@@ -0,0 +1,139 @@
+
+# RUN: yaml2obj %s -o %t.o
+# RUN: echo '.*' > %t.symbols.regex
+
+# RUN: cp %t.o %t0.o
+# RUN: llvm-objcopy %t0.o --set-visibility-syms=%t.symbols.regex=default --regex
+# RUN: llvm-readelf -s %t0.o | FileCheck %s --check-prefix=DEF
+# DEF-DAG: LOCAL  DEFAULT     1 default_local
+# DEF-DAG: LOCAL  DEFAULT     1 internal_local
+# DEF-DAG: LOCAL  DEFAULT     1 hidden_local
+# DEF-DAG: LOCAL  DEFAULT     1 protected_local
+# DEF-DAG: GLOBAL DEFAULT     1 default_global
+# DEF-DAG: WEAK   DEFAULT     1 default_weak
+# DEF-DAG: GLOBAL DEFAULT     1 internal_global
+# DEF-DAG: WEAK   DEFAULT     1 internal_weak
+# DEF-DAG: GLOBAL DEFAULT     1 hidden_global
+# DEF-DAG: WEAK   DEFAULT     1 hidden_weak
+# DEF-DAG: GLOBAL DEFAULT     1 protected_global
+# DEF-DAG: WEAK   DEFAULT     1 protected_weak
+
+# RUN: cp %t.o %t0.o
+# RUN: llvm-objcopy %t0.o --set-visibility-syms=%t.symbols.regex=hidden --regex
+# RUN: llvm-readelf -s %t0.o | FileCheck %s --check-prefix=HID
+# HID-DAG: LOCAL  HIDDEN      1 default_local
+# HID-DAG: LOCAL  HIDDEN      1 internal_local
+# HID-DAG: LOCAL  HIDDEN      1 hidden_local
+# HID-DAG: LOCAL  HIDDEN      1 protected_local
+# HID-DAG: GLOBAL HIDDEN      1 default_global
+# HID-DAG: WEAK   HIDDEN      1 default_weak
+# HID-DAG: GLOBAL HIDDEN      1 internal_global
+# HID-DAG: WEAK   HIDDEN      1 internal_weak
+# HID-DAG: GLOBAL HIDDEN      1 hidden_global
+# HID-DAG: WEAK   HIDDEN      1 hidden_weak
+# HID-DAG: GLOBAL HIDDEN      1 protected_global
+# HID-DAG: WEAK   HIDDEN      1 protected_weak
+
+# RUN: cp %t.o %t0.o
+# RUN: llvm-objcopy %t0.o --set-visibility-syms=%t.symbols.regex=protected --regex
+# RUN: llvm-readelf -s %t0.o | FileCheck %s --check-prefix=PRO
+# PRO-DAG: LOCAL  PROTECTED   1 default_local
+# PRO-DAG: LOCAL  PROTECTED   1 internal_local
+# PRO-DAG: LOCAL  PROTECTED   1 hidden_local
+# PRO-DAG: LOCAL  PROTECTED   1 protected_local
+# PRO-DAG: GLOBAL PROTECTED   1 default_global
+# PRO-DAG: WEAK   PROTECTED   1 default_weak
+# PRO-DAG: GLOBAL PROTECTED   1 internal_global
+# PRO-DAG: WEAK   PROTECTED   1 internal_weak
+# PRO-DAG: GLOBAL PROTECTED   1 hidden_global
+# PRO-DAG: WEAK   PROTECTED   1 hidden_weak
+# PRO-DAG: GLOBAL PROTECTED   1 protected_global
+# PRO-DAG: WEAK   PROTECTED   1 protected_weak
+
+# RUN: cp %t.o %t0.o
+# RUN: llvm-objcopy %t0.o --set-visibility-syms=%t.symbols.regex=internal --regex
+# RUN: llvm-readelf -s %t0.o | FileCheck %s --check-prefix=INT
+# INT-DAG: LOCAL  INTERNAL    1 default_local
+# INT-DAG: LOCAL  INTERNAL    1 internal_local
+# INT-DAG: LOCAL  INTERNAL    1 hidden_local
+# INT-DAG: LOCAL  INTERNAL    1 protected_local
+# INT-DAG: GLOBAL INTERNAL    1 default_global
+# INT-DAG: WEAK   INTERNAL    1 default_weak
+# INT-DAG: GLOBAL INTERNAL    1 internal_global
+# INT-DAG: WEAK   INTERNAL    1 internal_weak
+# INT-DAG: GLOBAL INTERNAL    1 hidden_global
+# INT-DAG: WEAK   INTERNAL    1 hidden_weak
+# INT-DAG: GLOBAL INTERNAL    1 protected_global
+# INT-DAG: WEAK   INTERNAL    1 protected_weak
+
+# RUN: cp %t.o %t0.o
+# RUN: llvm-objcopy %t0.o --set-visibility-sym=hidden_global=default
+# RUN: llvm-objcopy %t0.o --set-visibility-sym=default_global=hidden
+# RUN: llvm-objcopy %t0.o --set-visibility-sym=default_local=protected
+# RUN: llvm-objcopy %t0.o --set-visibility-sym=protected_global=internal
+# RUN: llvm-readelf -s %t0.o | FileCheck %s --check-prefix=SYM
+# SYM-DAG: LOCAL  PROTECTED   1 default_local
+# SYM-DAG: GLOBAL HIDDEN      1 default_global
+# SYM-DAG: GLOBAL DEFAULT     1 hidden_global
+# SYM-DAG: GLOBAL INTERNAL    1 protected_global
+
+!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:    default_local
+    Section: .text
+    Binding:  STB_LOCAL
+  - Name:    default_global
+    Section: .text
+    Binding:  STB_GLOBAL
+  - Name:    default_weak
+    Section: .text
+    Binding:  STB_WEAK
+  - Name:    internal_local
+    Section: .text
+    Binding:  STB_LOCAL
+    Other:    [ STV_INTERNAL ]
+  - Name:    internal_global
+    Section: .text
+    Binding:  STB_GLOBAL
+    Other:    [ STV_INTERNAL ]
+  - Name:    internal_weak
+    Section: .text
+    Binding:  STB_WEAK
+    Other:    [ STV_INTERNAL ]
+  - Name:    hidden_local
+    Section: .text
+    Binding:  STB_LOCAL
+    Other:    [ STV_HIDDEN ]
+  - Name:    hidden_global
+    Section: .text
+    Binding:  STB_GLOBAL
+    Other:    [ STV_HIDDEN ]
+  - Name:    hidden_weak
+    Section: .text
+    Binding:  STB_WEAK
+    Other:    [ STV_HIDDEN ]
+  - Name:    protected_local
+    Section: .text
+    Binding:  STB_LOCAL
+    Other:    [ STV_PROTECTED ]
+  - Name:    protected_global
+    Section: .text
+    Binding:  STB_GLOBAL
+    Other:    [ STV_PROTECTED ]
+  - Name:    protected_weak
+    Section: .text
+    Binding:  STB_WEAK
+    Other:    [ STV_PROTECTED ]
+  - Name:    ignored_name
+    Section: .text
+    Binding:  STB_GLOBAL
+    Other:    [ STV_INTERNAL ]
diff --git a/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp b/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
index f15307181fad6..9fb7d3997039e 100644
--- a/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
+++ b/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
@@ -254,6 +254,21 @@ parseSetSectionFlagValue(StringRef FlagValue) {
   return SFU;
 }
 
+static Expected<uint8_t> parseVisibilityType(StringRef VisType) {
+  const uint8_t Invalid = 0xff;
+  uint8_t type = StringSwitch<uint8_t>(VisType)
+                     .Case("default", ELF::STV_DEFAULT)
+                     .Case("hidden", ELF::STV_HIDDEN)
+                     .Case("internal", ELF::STV_INTERNAL)
+                     .Case("protected", ELF::STV_PROTECTED)
+                     .Default(Invalid);
+  if (type == Invalid)
+    return createStringError(errc::invalid_argument,
+                             "'%s' is not a valid symbol visibility",
+                             VisType.str().c_str());
+  return type;
+}
+
 namespace {
 struct TargetInfo {
   FileFormat Format;
@@ -962,6 +977,34 @@ objcopy::parseObjcopyOptions(ArrayRef<const char *> RawArgsArr,
 
     Config.SymbolsToAdd.push_back(*SymInfo);
   }
+  for (auto *Arg : InputArgs.filtered(OBJCOPY_set_visibility_sym)) {
+    if (!StringRef(Arg->getValue()).contains('='))
+      return createStringError(errc::invalid_argument,
+                               "bad format for --set-visibility-sym");
+    auto SymAndVis = StringRef(Arg->getValue()).split('=');
+    Expected<uint8_t> Type = parseVisibilityType(SymAndVis.second);
+    if (!Type)
+      return Type.takeError();
+    ELFConfig.SetVisibilityType = Type.get();
+    if (Error E =
+            ELFConfig.SymbolsToSetVisibility.addMatcher(NameOrPattern::create(
+                SymAndVis.first, SymbolMatchStyle, ErrorCallback)))
+      return std::move(E);
+  }
+  for (auto *Arg : InputArgs.filtered(OBJCOPY_set_visibility_syms)) {
+    if (!StringRef(Arg->getValue()).contains('='))
+      return createStringError(errc::invalid_argument,
+                               "bad format for --set-visibility-syms");
+    auto FileAndVis = StringRef(Arg->getValue()).split('=');
+    Expected<uint8_t> Type = parseVisibilityType(FileAndVis.second);
+    if (!Type)
+      return Type.takeError();
+    ELFConfig.SetVisibilityType = Type.get();
+    if (Error E = addSymbolsFromFile(ELFConfig.SymbolsToSetVisibility, DC.Alloc,
+                                     FileAndVis.first, SymbolMatchStyle,
+                                     ErrorCallback))
+      return std::move(E);
+  }
 
   ELFConfig.AllowBrokenLinks = InputArgs.hasArg(OBJCOPY_allow_broken_links);
 
diff --git a/llvm/tools/llvm-objcopy/ObjcopyOpts.td b/llvm/tools/llvm-objcopy/ObjcopyOpts.td
index ead8cd28d3877..d872316df5190 100644
--- a/llvm/tools/llvm-objcopy/ObjcopyOpts.td
+++ b/llvm/tools/llvm-objcopy/ObjcopyOpts.td
@@ -93,6 +93,17 @@ defm set_section_type
          "Set the type of section <section> to the integer <type>">,
       MetaVarName<"section=type">;
 
+defm set_visibility_sym
+    : Eq<"set-visibility-sym",
+         "Change the visibility of a symbol to the specified type">,
+      MetaVarName<"symbol=visibility_type">;
+defm set_visibility_syms
+    : Eq<"set-visibility-syms",
+         "Reads a list of symbols from <filename> and changes their "
+         "visibility to the specified type. Visibility types: default, "
+         "internal, hidden, protected">,
+      MetaVarName<"filename=visibility_type">;
+
 def S : Flag<["-"], "S">,
         Alias<strip_all>,
         HelpText<"Alias for --strip-all">;

jh7370

Thanks for the patch! The new option seems reasonable to me, but could you outline what the motivation is for you to have this option? You could after all remove a symbol and readd it with the new visibility as a workaround.

Have you looked at whether GNU objcopy supports this sort of option? If they do, it's important that the option name matches. If they don't, I think the option name should be --set-symbol-visibility which would line up better with the --new-symbol-visibility option name apart from anything else.

Finally, please update the llvm-objcopy documentation at llvm/docs/CommandGuide/llvm-objcopy.rst to describe the new option.

jh7370 · 2024-02-08T07:57:42Z

llvm/include/llvm/ObjCopy/ELF/ELFConfig.h

@@ -18,6 +19,9 @@ namespace objcopy {
 struct ELFConfig {
  uint8_t NewSymbolVisibility = (uint8_t)ELF::STV_DEFAULT;

+  NameMatcher SymbolsToSetVisibility;
+  uint8_t SetVisibilityType = ELF::STV_DEFAULT;


I'm slightly surprised that you don't need a cast here when NewSymbolVisibility uses a cast immediately above. It could be the on above is unnecessary of course.

I looked it up and found that "values of unscoped enumeration type can be promoted or converted to integral types", so this is just an implicit conversion.
I can make a PR later that removes that cast in NewSymbolVisibility.

Thanks. Up to you whether you do or not, I don't mind.

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp

llvm/test/tools/llvm-objcopy/ELF/set-visibility.test

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

github-actions · 2024-02-08T17:34:11Z

✅ With the latest revision this PR passed the C/C++ code formatter.

kuilpd · 2024-02-08T18:07:48Z

Thanks for the patch! The new option seems reasonable to me, but could you outline what the motivation is for you to have this option? You could after all remove a symbol and readd it with the new visibility as a workaround.

The idea was to have a straightforward option instead of the weird workaround :) Plus it allows using patterns and even reading them from a file.

Have you looked at whether GNU objcopy supports this sort of option? If they do, it's important that the option name matches. If they don't, I think the option name should be --set-symbol-visibility which would line up better with the --new-symbol-visibility option name apart from anything else.

From what I've seen, it doesn't have anything that changes the visibility of symbols.

MaskRay · 2024-02-09T06:50:16Z

FWIW I filed https://sourceware.org/bugzilla/show_bug.cgi?id=31000 ("objcopy: add support for changing ELF symbol visibility") in 2023-10. While I think they will likely take an option to set the visibility (especially if someone sends a patch), there is some design space here. If --set-symbol-flags is present, then we might not need a special-purpose --set-symbol-visibility.

It appears that objcopy does not provide a direct way to set symbol visibility, but I have found a workaround:

    objcopy --strip-symbol __udivmodti4 --add-symbol __udivmodti4=.text.__udivmodti4:0,weak udivmodti4.c.o

Running this command on an archive will add __udivmodti4 to every archive member, which is not desired. So, I needed to determine which archive member defines __udivmodti4. I ended up implementing something like this in Bazel:

    """$(AR) x --output=$(@D)/lib $(OUTS)
    for o in $(@D)/lib/*.o; do
      $(NM) -gU $$o | grep -qw __udivmodti4 && $(OBJCOPY) --strip-symbol __udivmodti4 --add-symbol __udivmodti4=.text.__udivmodti4:0,weak $$o
    done
    $(AR) r $(OUTS) $(@D)/lib/*.o
    rm -rf $(@D)/lib"""

This is cumbersome.

jh7370

Please update the PR title to reflect the revised option name.

@MaskRay, llvm-objcopy (and I presume GNU objcopy) have options to set the binding of a symbol already (although they aren't unconditional, I'll admit). Aside from binding and visibility, what other properties did you have in mind for a possible --set-symbol-flags?

@kuilpd, please don't mark items as resolved that I've started, as I use these items to track whether the requested changes have actually been made as I intended. See https://discourse.llvm.org/t/rfc-github-pr-resolve-conversation-button/73178 for more details.

Picking up the thread about multiple values, which is now hidden as "Outdated", I think it makes sense to support this case. If a symbol matches multiple patterns, just have each pattern set it in turn, in the order they appear in the vector. The end result is that the symbol's visibility will match the last visibility that matches. "Last one wins" is a fairly common pattern in option parsing, and I think it therefore generalises to this quite nicely.

jh7370 · 2024-02-14T08:22:17Z

llvm/include/llvm/ObjCopy/ELF/ELFConfig.h

@@ -18,6 +19,9 @@ namespace objcopy {
 struct ELFConfig {
  uint8_t NewSymbolVisibility = (uint8_t)ELF::STV_DEFAULT;

+  NameMatcher SymbolsToSetVisibility;
+  uint8_t SetVisibilityType = ELF::STV_DEFAULT;


Thanks. Up to you whether you do or not, I don't mind.

jh7370 · 2024-02-14T08:25:45Z

llvm/test/tools/llvm-objcopy/ELF/set-visibility-check-errors.test

@@ -0,0 +1,43 @@
+


The norm for new options is to have a single test that test the different cases rather than lots of individual test files. This makes it easier to understand the option's full behaviour because you don't have to look through multiple files to understand everything. If you need different input files for different cases, you can use the yaml2obj --docnum option or possibly the -D option to set macro values, whichever is more appropriate. Sometimes you can get away with only one input and have multiple symbols in it with appropriate properties.

Also nit: please don't start your file with a blank line :)

I didn't quite understand, should I merge all 4 tests into one file or just set-visibility-symbol.test and set-visibility-symbols.test?

globalize.test tests both the globalize-symbol and globalize-symbols option, so I think they probably want to be in the same file. It's not necessarily clear-cut, but is probably a good idea.

llvm/test/tools/llvm-objcopy/ELF/set-visibility-check-errors.test

llvm/test/tools/llvm-objcopy/ELF/set-visibility-symbols.test

llvm/test/tools/llvm-objcopy/ELF/set-visibility-undef-symbol.test

jh7370

@MaskRay, any further comments from you?

llvm/test/tools/llvm-objcopy/ELF/set-symbol-visibility-check-errors.test

jh7370

Approving, to clear my "changes requested" blocker, but I'd still like to hear from @MaskRay to see if he has any further opinions before this gets merged.

MaskRay · 2024-02-22T08:43:14Z

llvm/test/tools/llvm-objcopy/ELF/set-symbol-visibility.test

+
+## Check that the visibility of symbols specified by a wildcard are set correctly,
+## and that no other symbols are affected.
+# RUN: llvm-objcopy %t.o %t7.o --set-symbol-visibility=*_local=hidden --wildcard


Use single quotes when * is used to avoid potential issues with some shell configurations.

MaskRay · 2024-02-22T08:45:39Z

llvm/tools/llvm-objcopy/ObjcopyOpts.td

+      MetaVarName<"symbol=visibility_type">;
+defm set_symbols_visibility
+    : Eq<"set-symbols-visibility",
+         "Reads a list of symbols from <filename> and changes their "


Read ... change

Use an imperative sentence to match the convention.

I copied this description from other options, localize- globalize- strip- weaken- keep- symbols all use this wording. Should I change it?

MaskRay

Since --set-symbols-visibility is added, it should be mentioned in the description (first comment) as well.

MaskRay · 2024-02-22T08:48:15Z

llvm/tools/llvm-objcopy/ObjcopyOpts.td

+defm set_symbols_visibility
+    : Eq<"set-symbols-visibility",
+         "Reads a list of symbols from <filename> and changes their "
+         "visibility to the specified type. Visibility types: default, "


We don't use "visibility type". The generic ABI (official spec) uses "visibility attribute" (https://www.sco.com/developers/gabi/latest/ch4.symtab.html) . Though, perhaps we can just use "value".

MaskRay · 2024-02-22T08:53:17Z

I think GNU objcopy is likely willing to accept --set-symbol-visibility.--set-symbols-visibility filename=visibility seems to match the convention that uses filename for an option named *symbols*, but the infix instead of suffix use of symbols is a new convention. I do like --set-symbols-visibility and cannot find a better alternative, though.

MaskRay · 2024-02-22T08:57:06Z

llvm/test/tools/llvm-objcopy/ELF/set-symbol-visibility.test

+    Other:    [ STV_HIDDEN ]
+  - Name:    protected_local
+    Section: .text
+    Binding:  STB_LOCAL


STB_LOCAL after non-local is technically invalid.
"In each symbol table, all symbols with STB_LOCAL binding precede the weak and global symbols. "

This should be easy to fix and does not change the generality of the test.

MaskRay · 2024-02-22T08:57:42Z

llvm/test/tools/llvm-objcopy/ELF/set-symbol-visibility.test

+    Section: .text
+    Binding:  STB_WEAK
+    Other:    [ STV_HIDDEN ]
+  - Name:    protected_local


This is technically invalid "A symbol with STB_LOCAL binding may not have STV_PROTECTED visibility. " but we can probably ignore the generic ABI wording.

kuilpd · 2024-02-23T15:40:49Z

I think GNU objcopy is likely willing to accept --set-symbol-visibility.--set-symbols-visibility filename=visibility seems to match the convention that uses filename for an option named *symbols*, but the infix instead of suffix use of symbols is a new convention. I do like --set-symbols-visibility and cannot find a better alternative, though.

So as I understood, please correct me if I'm wrong, that for now we leave the option names and format as is.

MaskRay · 2024-02-23T19:07:38Z

llvm/docs/CommandGuide/llvm-objcopy.rst

+
+.. option:: --set-symbols-visibility <filename>=<visibility>
+
+ Reads a list of symbols from <filename> and changes their visibility to the


Imperative sentence like other options?

I replied in another comment:

I copied this description from other options, localize- globalize- strip- weaken- keep- symbols all use this wording. Should I change it?

Yes, there are ~8 options that are inconsistent with others. Actually there is a larger problem that many help messages are slightly wrong, not synced with the clearer semantics by @jh7370 in https://reviews.llvm.org/D63820

I am working on a patch to fix these help messages.

For the new options, definitely use imperative sentences as llvm/docs/CommandGuide/llvm-objcopy.rst uses.

MaskRay · 2024-02-23T19:09:54Z

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp

@@ -298,6 +298,10 @@ static Error updateAndRemoveSymbols(const CommonConfig &Config,
         Config.SymbolsToLocalize.matches(Sym.Name)))
      Sym.Binding = STB_LOCAL;

+    for (auto &[Matcher, VisibilityType] : ELFConfig.SymbolsToSetVisibility)
+      if (Matcher.matches(Sym.Name) && Sym.getShndx() != SHN_UNDEF)


Why is there a special case for undefined symbol? A protected/hidden undefined should be allowed.

The following code generates a hidden undefined symbol.

#pragma GCC visibility push(hidden) void bar(); void foo() { bar(); }

The option should have the ability to change their visibility.

MaskRay · 2024-02-23T19:10:30Z

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp

@@ -298,6 +298,10 @@ static Error updateAndRemoveSymbols(const CommonConfig &Config,
         Config.SymbolsToLocalize.matches(Sym.Name)))
      Sym.Binding = STB_LOCAL;

+    for (auto &[Matcher, VisibilityType] : ELFConfig.SymbolsToSetVisibility)


Change VisibilityType to Visibility like elsewhere?

MaskRay · 2024-02-23T19:10:56Z

I think GNU objcopy is likely willing to accept --set-symbol-visibility.--set-symbols-visibility filename=visibility seems to match the convention that uses filename for an option named *symbols*, but the infix instead of suffix use of symbols is a new convention. I do like --set-symbols-visibility and cannot find a better alternative, though.

So as I understood, please correct me if I'm wrong, that for now we leave the option names and format as is.

Yes. The option names are good as is.

MaskRay · 2024-02-23T19:14:17Z

Add an option to manually set the visibility of symbols.

There is already an option to set the visibility of newly added symbols via --add-symbol and --new-symbol-visibility. This option will allow to change the visibility of already existing symbols.

The first comment will be used as the default commit message. It needs to mention --set-symbols-visibility, so that people just searching for the option name can find this commit.

[llvm-objcopy] Add llvm-objcopy option --set-symbol-visibility

llvm-objcopy is redundant. Perhaps [llvm-objcopy] Add --set-symbol-visibility/ and --set-symbols-visibility

jh7370

The new option probably deserves mentioning in the release notes?

kuilpd · 2024-02-27T18:24:36Z

@MaskRay @jh7370
Added the release note, please tell me if I need to adjust it.

MaskRay

release note LGTM

jh7370

LGTM too.

Add llvm-objcopy option --set-visibility-sym

af5f188

kuilpd requested review from MaskRay, jh7370 and avl-llvm February 6, 2024 16:52

kuilpd self-assigned this Feb 6, 2024

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

kuilpd changed the title ~~Add llvm-objcopy option --set-visibility-sym~~ [llvm-objcopy] Add llvm-objcopy option --set-visibility-sym 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

Change --set-visibility-sym test check order and variable

cc05728

jh7370 requested changes Feb 8, 2024

View reviewed changes

jh7370 mentioned this pull request Feb 8, 2024

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

Merged

kuilpd added 7 commits February 8, 2024 22:23

Move setting the visibility to after localizing hidden symbols

97b1e2d

Change code formatting in when parsing the options

d3104d1

Add checks for multiple uses of setting visibility

524b13f

Adjust and separate tests of setting visibility functionality

9ec88f2

Add a test for checking various error cases

450e1f6

Add a test for ingoring SHN_UNDEF symbols

20a8c43

Update the llvm-objcopy documentation

f6b659b

kuilpd added 2 commits February 8, 2024 22:52

Rename --set-visibility-sym to --set-symbol-visibility

c6faec2

Apply clang-format

105e335

jh7370 reviewed Feb 14, 2024

View reviewed changes

kuilpd added 4 commits February 14, 2024 21:46

Remove checks for multiple uses of setting visibility

cf0c1e5

Allow each symbol pattern to have a separate visibility

88c3cf2

Adjust formatting and wording in tests

78cd496

Remove unnecessary --regex option in set-visibility-check-errors.test

4ab67a1

kuilpd added 3 commits February 16, 2024 17:18

Add a test case with a special character without pattern matching

2d14e71

Add a test case that checks visibility overwriting with multiple options

7cd45b4

Adjust test formatting

98e127f

jh7370 reviewed Feb 20, 2024

View reviewed changes

llvm/test/tools/llvm-objcopy/ELF/set-symbol-visibility-check-errors.test Outdated Show resolved Hide resolved

Merge all tests into a single file

287cf46

jh7370 approved these changes Feb 21, 2024

View reviewed changes

MaskRay reviewed Feb 22, 2024

View reviewed changes

kuilpd added 2 commits February 23, 2024 20:30

Adjust test formatting

fe8a2d0

Reword option description

d25332c

MaskRay reviewed Feb 23, 2024

View reviewed changes

MaskRay approved these changes Feb 23, 2024

View reviewed changes

kuilpd added 3 commits February 26, 2024 21:56

Adjust description as suggested in llvm#82830

72022ae

Allow the option to change the visibility of hidden symbols

ca26144

Rename VisibilityType to Visibility

26da9e3

kuilpd changed the title ~~[llvm-objcopy] Add llvm-objcopy option --set-symbol-visibility~~ [llvm-objcopy] Add --set-symbol-visibility and --set-symbols-visibility options Feb 26, 2024

MaskRay approved these changes Feb 26, 2024

View reviewed changes

jh7370 reviewed Feb 27, 2024

View reviewed changes

Added the options to release notes

5df01eb

MaskRay approved these changes Feb 27, 2024

View reviewed changes

jh7370 approved these changes Feb 28, 2024

View reviewed changes

kuilpd merged commit 07d8a45 into llvm:main Feb 28, 2024
4 of 5 checks passed


		.. option:: --set-symbols-visibility <filename>=<visibility>

		Reads a list of symbols from <filename> and changes their visibility to the

[llvm-objcopy] Add --set-symbol-visibility and --set-symbols-visibility options #80872

[llvm-objcopy] Add --set-symbol-visibility and --set-symbols-visibility options #80872

Conversation

kuilpd commented Feb 6, 2024 • edited

llvmbot commented Feb 6, 2024

jh7370 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 8, 2024 • edited

kuilpd commented Feb 8, 2024 • edited

MaskRay commented Feb 9, 2024 • edited

jh7370 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jh7370 left a comment

Choose a reason for hiding this comment

jh7370 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaskRay left a comment

Choose a reason for hiding this comment

MaskRay Feb 22, 2024 • edited

Choose a reason for hiding this comment

MaskRay commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuilpd commented Feb 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaskRay Feb 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaskRay Feb 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaskRay commented Feb 23, 2024

MaskRay commented Feb 23, 2024 • edited

jh7370 left a comment

Choose a reason for hiding this comment

kuilpd commented Feb 27, 2024

MaskRay left a comment

Choose a reason for hiding this comment

jh7370 left a comment

Choose a reason for hiding this comment

kuilpd commented Feb 6, 2024 •

edited

github-actions bot commented Feb 8, 2024 •

edited

kuilpd commented Feb 8, 2024 •

edited

MaskRay commented Feb 9, 2024 •

edited

MaskRay Feb 22, 2024 •

edited

MaskRay Feb 23, 2024 •

edited

MaskRay Feb 23, 2024 •

edited

MaskRay commented Feb 23, 2024 •

edited