Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[llvm-objcopy] Add --remove-symbol-prefix #79415

Merged
merged 8 commits into from
Feb 7, 2024
Merged

Conversation

kongy
Copy link
Collaborator

@kongy kongy commented Jan 25, 2024

llvm-objcopy has functionality to add prefixes to symbols (--prefix-symbols). This adds an inverse action to undo the modification and return to the original.

@kongy kongy requested a review from MaskRay January 25, 2024 07:33
@kongy kongy requested review from jh7370 and MaskRay and removed request for MaskRay January 25, 2024 07:33
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 25, 2024

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

Author: Yi Kong (kongy)

Changes

llvm-objcopy has functionality to add prefixes to symbols (--prefix-symbols). This adds an inverse action to undo the modification and return to the original.


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

5 Files Affected:

  • (modified) llvm/include/llvm/ObjCopy/CommonConfig.h (+1)
  • (modified) llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp (+4)
  • (added) llvm/test/tools/llvm-objcopy/ELF/prefix-symbols-remove.test (+78)
  • (modified) llvm/tools/llvm-objcopy/ObjcopyOptions.cpp (+7)
  • (modified) llvm/tools/llvm-objcopy/ObjcopyOpts.td (+5)
diff --git a/llvm/include/llvm/ObjCopy/CommonConfig.h b/llvm/include/llvm/ObjCopy/CommonConfig.h
index 386c20aec184ded..0d9320ec2efd71b 100644
--- a/llvm/include/llvm/ObjCopy/CommonConfig.h
+++ b/llvm/include/llvm/ObjCopy/CommonConfig.h
@@ -218,6 +218,7 @@ struct CommonConfig {
   uint64_t PadTo = 0;
   StringRef SplitDWO;
   StringRef SymbolsPrefix;
+  StringRef SymbolsPrefixRemove;
   StringRef AllocSectionsPrefix;
   DiscardType DiscardMode = DiscardType::None;
 
diff --git a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
index b6d77d17bae36c5..a5ebc99b42ea84c 100644
--- a/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
+++ b/llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp
@@ -331,6 +331,10 @@ static Error updateAndRemoveSymbols(const CommonConfig &Config,
 
     if (!Config.SymbolsPrefix.empty() && Sym.Type != STT_SECTION)
       Sym.Name = (Config.SymbolsPrefix + Sym.Name).str();
+
+    if (!Config.SymbolsPrefixRemove.empty() && Sym.Type != STT_SECTION)
+      if (Sym.Name.rfind(Config.SymbolsPrefixRemove, 0) == 0)
+        Sym.Name = Sym.Name.substr(Config.SymbolsPrefixRemove.size());
   });
 
   // The purpose of this loop is to mark symbols referenced by sections
diff --git a/llvm/test/tools/llvm-objcopy/ELF/prefix-symbols-remove.test b/llvm/test/tools/llvm-objcopy/ELF/prefix-symbols-remove.test
new file mode 100644
index 000000000000000..ad9b77d59f1b1cd
--- /dev/null
+++ b/llvm/test/tools/llvm-objcopy/ELF/prefix-symbols-remove.test
@@ -0,0 +1,78 @@
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --prefix-symbols-remove __pf_ %t %t2
+# RUN: llvm-readobj --symbols %t2 | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x1000
+    AddressAlign:    0x0000000000000010
+    Size:            64
+Symbols:
+  - Name:     __pf_foo
+    Type:     STT_SECTION
+    Section:  .text
+  - Name:     __pf_bar
+    Type:     STT_FILE
+    Section:  .text
+  - Name:     foobar
+    Type:     STT_FUNC
+    Section:  .text
+    Binding:  STB_GLOBAL
+  - Name:     undef
+    Binding:  STB_GLOBAL
+
+# CHECK:  Symbols [
+# NEXT:        Symbol {
+# NEXT:          Name:
+# NEXT:          Value: 0x0
+# NEXT:          Size: 0
+# NEXT:          Binding: Local
+# NEXT:          Type: None
+# NEXT:          Other: 0
+# NEXT:          Section: Undefined
+# NEXT:        }
+# NEXT:        Symbol {
+# NEXT:          Name: foo
+# NEXT:          Value: 0x0
+# NEXT:          Size: 0
+# NEXT:          Binding: Local
+# NEXT:          Type: Section
+# NEXT:          Other: 0
+# NEXT:          Section: .text
+# NEXT:        }
+# NEXT:        Symbol {
+# NEXT:          Name: bar
+# NEXT:          Value: 0x0
+# NEXT:          Size: 0
+# NEXT:          Binding: Local
+# NEXT:          Type: File
+# NEXT:          Other: 0
+# NEXT:          Section: .text
+# NEXT:        }
+# NEXT:        Symbol {
+# NEXT:          Name: foobar
+# NEXT:          Value: 0x0
+# NEXT:          Size: 0
+# NEXT:          Binding: Global
+# NEXT:          Type: Function
+# NEXT:          Other: 0
+# NEXT:          Section: .text
+# NEXT:        }
+# NEXT:        Symbol {
+# NEXT:          Name: undef
+# NEXT:          Value: 0x0
+# NEXT:          Size: 0
+# NEXT:          Binding: Global
+# NEXT:          Type: None
+# NEXT:          Other: 0
+# NEXT:          Section: Undefined
+# NEXT:        }
+# NEXT:      ]
diff --git a/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp b/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
index f15307181fad618..a6ebf24ecb15763 100644
--- a/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
+++ b/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
@@ -731,7 +731,14 @@ objcopy::parseObjcopyOptions(ArrayRef<const char *> RawArgsArr,
         llvm::crc32(arrayRefFromStringRef(Debug->getBuffer()));
   }
   Config.SplitDWO = InputArgs.getLastArgValue(OBJCOPY_split_dwo);
+
   Config.SymbolsPrefix = InputArgs.getLastArgValue(OBJCOPY_prefix_symbols);
+  Config.SymbolsPrefixRemove = InputArgs.getLastArgValue(OBJCOPY_prefix_symbols_remove);
+  if (!Config.SymbolsPrefix.empty() && !Config.SymbolsPrefixRemove.empty())
+    return createStringError(
+        errc::invalid_argument,
+        "--prefix-symbols and --prefix-symbols-remove are mutualy exclusive");
+
   Config.AllocSectionsPrefix =
       InputArgs.getLastArgValue(OBJCOPY_prefix_alloc_sections);
   if (auto Arg = InputArgs.getLastArg(OBJCOPY_extract_partition))
diff --git a/llvm/tools/llvm-objcopy/ObjcopyOpts.td b/llvm/tools/llvm-objcopy/ObjcopyOpts.td
index ead8cd28d387791..6c64e5ecf52e4a7 100644
--- a/llvm/tools/llvm-objcopy/ObjcopyOpts.td
+++ b/llvm/tools/llvm-objcopy/ObjcopyOpts.td
@@ -203,6 +203,11 @@ defm dump_section
 defm prefix_symbols
     : Eq<"prefix-symbols", "Add <prefix> to the start of every symbol name">,
       MetaVarName<"prefix">;
+defm prefix_symbols_remove
+    : Eq<"prefix-symbols-remove",
+         "Remove <prefix> from the start of every symbol name. No-op for symbols that do not start "
+         "with <prefix>">,
+      MetaVarName<"prefix">;
 
 defm prefix_alloc_sections
     : Eq<"prefix-alloc-sections", "Add <prefix> to the start of every allocated section name">,

Copy link

github-actions bot commented Jan 25, 2024

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

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for the patch! What's the motivation behind being able to remove prefixes from symbol names? Note that removing prefixes could lead to various issues, e.g. multiple symbols with the same name in the same file, resulting in the linker not knowing the right symbol to pick, so adding this option needs to be done carefully. Does GNU objcopy have any prior art for this option?

In addition to your code changes, please update the llvm-objcopy CommandGuide doc (llvm/docs/CommandGuide/llvm-objcopy.rst) to describe the new option.

llvm/tools/llvm-objcopy/ObjcopyOpts.td Outdated Show resolved Hide resolved
llvm/tools/llvm-objcopy/ObjcopyOptions.cpp Outdated Show resolved Hide resolved
llvm/include/llvm/ObjCopy/CommonConfig.h Show resolved Hide resolved
@MaskRay
Copy link
Member

MaskRay commented Jan 25, 2024

GNU objcopy does not support the option. I filed https://sourceware.org/bugzilla/show_bug.cgi?id=16773

If an object file has a and za, llvm-objcopy --prefix-symbols-remove=z would leave two a. If either one is STB_LOCAL, it should be fine. If both are STB_GLOBAL, the linker will likely report a duplicate definition error.

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an object file has a and za, llvm-objcopy --prefix-symbols-remove=z would leave two a. If either one is STB_LOCAL, it should be fine. If both are STB_GLOBAL, the linker will likely report a duplicate definition error.

Thinking more about this, I think it's fair to allow it (it's not the first option where people can break their object files after all).

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp Outdated Show resolved Hide resolved
@kongy kongy force-pushed the objcopy-remove-prefix branch 3 times, most recently from 766ab3d to 1743d78 Compare January 29, 2024 06:22
Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kongy, per LLVM review guidance, please don't do rebase and force push on an in-progress review, because this prevents diffing against the version that was originally reviewed, making it harder to see what's changed between the original and current. Prefer fixup commits, or if a rebase is really necessary, do a merge from main instead. Also, please don't click "Resolve Conversation" on threads I'm involved in, as I use this functionality to confirm that I have reviewed any requested changes and am happy with them. I will assume, unless stated otherwise, that you have attempted to resolve all requested changes without you needing to mark the conversation as resolved whenver you update a PR (and if you haven't, per LLVM guidance, you need to say so).

llvm-objcopy has functionality to add prefixes to symbols
(--prefix-symbols). This adds an inverse action to undo the modification
and return to the original.
@kongy
Copy link
Collaborator Author

kongy commented Jan 29, 2024

@kongy, per LLVM review guidance, please don't do rebase and force push on an in-progress review, because this prevents diffing against the version that was originally reviewed, making it harder to see what's changed between the original and current. Prefer fixup commits, or if a rebase is really necessary, do a merge from main instead. Also, please don't click "Resolve Conversation" on threads I'm involved in, as I use this functionality to confirm that I have reviewed any requested changes and am happy with them. I will assume, unless stated otherwise, that you have attempted to resolve all requested changes without you needing to mark the conversation as resolved whenver you update a PR (and if you haven't, per LLVM guidance, you need to say so).

Got it. Sorry this is my first time uploading an LLVM change after the Github switch.

@MaskRay
Copy link
Member

MaskRay commented Feb 1, 2024

Can you update llvm/docs/CommandGuide/llvm-objcopy.rst?

# RUN: cmp %t2 %t3

## When both options are present, llvm-objcopy should remove
## prefixes first, before adding prefixes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you forget --prefix-symbols as a test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@MaskRay
Copy link
Member

MaskRay commented Feb 1, 2024

title: Add functionality to llvm-objcopy to remove prefixes

Perhaps [llvm-objcopy] Add --remove-symbol-prefix ? (ensure that the option name is included in the commit message)

@kongy kongy changed the title Add functionality to llvm-objcopy to remove prefixes [llvm-objcopy] Add --remove-symbol-prefix Feb 2, 2024
Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM when jh7370 is happy

@kongy
Copy link
Collaborator Author

kongy commented Feb 6, 2024

I'm going to merge this as no objection is raised for three days. Happy to address any further comments in follow-up CLs.

@jh7370
Copy link
Collaborator

jh7370 commented Feb 6, 2024

GNU objcopy does not support the option. I filed https://sourceware.org/bugzilla/show_bug.cgi?id=16773

That bug reference seems incorrect @MaskRay?

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to merge this as no objection is raised for three days. Happy to address any further comments in follow-up CLs.

In general, I don't think this is within LLVM policy. @MaskRay explicitly said his approval was conditional on my approval, so landing it without my approval or a revised approval from @MaskRay would effectively mean landing this without any approval at all. I've been a) off sick and b) have a high load of reviews that need to be tackled, so responding to updates takes time (even if I hadn't been off sick).

llvm/docs/CommandGuide/llvm-objcopy.rst Outdated Show resolved Hide resolved
kongy and others added 5 commits February 6, 2024 18:56
Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks. Do you have commit access?

@kongy kongy merged commit 1b87ebc into llvm:main Feb 7, 2024
4 of 5 checks passed
@kongy kongy deleted the objcopy-remove-prefix branch February 7, 2024 08:38
@kongy
Copy link
Collaborator Author

kongy commented Feb 7, 2024

LGTM, thanks. Do you have commit access?

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants