[llvm-objcopy][COFF] Implement --redefine-sym and --redefine-syms

[MaskRay]

Summary

The bad usage tests in ELF/redefine-symbols.test are not specific to ELF.
Move them to redefine-symbols.test .

Also fix the documentation regarding --redefine-syms: the old and new
names are separated by whitespace, not an equals sign.

[MaskRay]

llvm/test/tools/llvm-objcopy/COFF/redefine-symbol.test

31

test/tools/llvm-objcopy/ELF/redefine-symbol.test contains tests for incorrect option usage (generic). I guess we probably don't need to duplicate that.

[jhenderson]

Looks okay to me, but I don't know enough about COFF to know whether there are any hidden gotchas with renaming symbols that might need accounting for.

llvm/test/tools/llvm-objcopy/COFF/redefine-symbol.test

6–7

Maybe add some leading whitespace to one of these two lines?

31

We don't need to duplicate it, but perhaps we should move that bit into test/tools/llvm-objcopy (i.e. out of the ELF layer)?

[MaskRay]

Improve tests

[MaskRay]

%t3 -> /dev/null

[jhenderson]

llvm/test/tools/llvm-objcopy/redefine-symbols.test

1 ↗

(On Diff #228741)

Maybe worth putting a comment saying why this test is not in the ELF specific test folder (i.e. it contains just the common parsing stuff).

[mstorsjo]

Looks good in general, with two minor comments

llvm/docs/CommandGuide/llvm-objcopy.rst

96

While this just moves the documentation, it seems to me that this documentation is wrong; in the text file, there's no equals sign between old and new names, they're just separated by whitespace.

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

138

One pretty relevant functional aspect of this, is the fact that a request to rename a symbol that doesn't exist passes through silently (which I presume matches GNU behaviour). It might be good to add a test of that aspect.

[MaskRay]

Address review comments

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

138

A non-existent symbol does not error. Added a test.

[mstorsjo]

LGTM, but please mention the documentation fix in the commit message, as it's not immediately obvious that the paragraph is edited while it's moved, and the edit is kind of unrelated to this change (other than the lines are moved).

[MaskRay]

Update description: fixed the documentation

llvm/docs/CommandGuide/llvm-objcopy.rst

 .. option:: --only-section <section>, -j
 
  Remove all sections from the output, except for sections named ``<section>``.
  Can be specified multiple times to keep multiple sections.
 
  For MachO objects, ``<section>`` must be formatted as
  ``<segment name>,<section name>``.
 
+.. option:: --redefine-sym <old>=<new>
+
+ Rename symbols called ``<old>`` to ``<new>`` in the output. Can be specified
+ multiple times to rename multiple symbols.
+
+.. option:: --redefine-syms <filename>
+
+ Rename symbols in the output as described in the file ``<filename>``. In the
+ file, each line represents a single symbol to rename, with the old name and new
+ name separated by an equals sign. Leading and trailing whitespace is ignored,
+ as is anything following a '#'. Can be specified multiple times to read names
+ from multiple files.
+
 .. option:: --regex
 
  If specified, symbol and section names specified by other switches are treated
  as extended POSIX regular expression patterns.
 
 .. option:: --remove-section <section>, -R
 
  Remove the specified section from the output. Can be specified multiple times

 .. option:: --prefix-symbols <prefix>
 
  Add ``<prefix>`` to the front of every symbol name in the output.
 
 .. option:: --preserve-dates, -p
 
  Preserve access and modification timestamps in the output.
 
-.. option:: --redefine-sym <old>=<new>
-
- Rename symbols called ``<old>`` to ``<new>`` in the output. Can be specified
- multiple times to rename multiple symbols.
-
-.. option:: --redefine-syms <filename>
-
- Rename symbols in the output as described in the file ``<filename>``. In the
- file, each line represents a single symbol to rename, with the old name and new
- name separated by an equals sign. Leading and trailing whitespace is ignored,
- as is anything following a '#'. Can be specified multiple times to read names
- from multiple files.
-
 .. option:: --rename-section <old>=<new>[,<flag>,...]
 
  Rename sections called ``<old>`` to ``<new>`` in the output, and apply any
  specified ``<flag>`` values. See :option:`--set-section-flags` for a list of
  supported flags. Can be specified multiple times to rename multiple sections.
 
 .. option:: --set-section-flags <section>=<flag>[,<flag>,...]
 

llvm/test/tools/llvm-objcopy/COFF/redefine-symbol.test

+# RUN: yaml2obj %s -o %t
+
+# RUN: llvm-objcopy --redefine-sym func=cnuf --redefine-sym foo=bar %t %t2
+# RUN: llvm-readobj --symbols %t2 | FileCheck %s
+
+# RUN: echo 'func cnuf #rename func' > %t.rename.txt
+# RUN: echo 'foo bar' >> %t.rename.txt
+# RUN: llvm-objcopy --redefine-syms %t.rename.txt %t %t3
+# RUN: cmp %t2 %t3
+
+# CHECK:      Symbol {
+# CHECK:      Symbol {
+# CHECK:      Symbol {
+# CHECK-NEXT:   Name: cnuf
+# CHECK-NEXT:   Value: 0
+# CHECK-NEXT:   Section: .text (1)
+# CHECK-NEXT:   BaseType: Null (0x0)
+# CHECK-NEXT:   ComplexType: Function (0x2)
+# CHECK-NEXT:   StorageClass: External (0x2)
+# CHECK-NEXT:   AuxSymbolCount: 0
+# CHECK-NEXT: }
+# CHECK-NEXT: Symbol {
+# CHECK-NEXT:   Name: bar
+# CHECK-NEXT:   Value: 0
+# CHECK-NEXT:   Section: .rdata (2)
+# CHECK-NEXT:   BaseType: Null (0x0)
+# CHECK-NEXT:   ComplexType: Null (0x0)
+# CHECK-NEXT:   StorageClass: External (0x2)
+# CHECK-NEXT:   AuxSymbolCount: 0
+# CHECK-NEXT: }
+
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+  - Name:            .rdata
+    Characteristics: [  ]
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+symbols:
+  - Name:            .text
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+  - Name:            .rdata
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+  - Name:            func
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            foo
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

for (Section &Sec : Obj.getMutableSections())
       Sec.Relocs.clear();
 
   // If we need to do per-symbol removals, initialize the Referenced field.
   if (Config.StripUnneeded || Config.DiscardMode == DiscardType::All ||
       !Config.SymbolsToRemove.empty())
     if (Error E = Obj.markSymbols())
       return E;
 
+  for (Symbol &Sym : Obj.getMutableSymbols()) {
+    auto I = Config.SymbolsToRename.find(Sym.Name);
+    if (I != Config.SymbolsToRename.end())
+      Sym.Name = I->getValue();
+  }
+
   // Actually do removals of symbols.
   Obj.removeSymbols([&](const Symbol &Sym) {
     // For StripAll, all relocations have been stripped and we remove all
     // symbols.
     if (Config.StripAll || Config.StripAllGNU)
       return true;
 
     if (Config.SymbolsToRemove.matches(Sym.Name)) {
if (Config.AllowBrokenLinks || !Config.BuildIdLinkDir.empty() ||
       Config.BuildIdLinkInput || Config.BuildIdLinkOutput ||
       !Config.SplitDWO.empty() || !Config.SymbolsPrefix.empty() ||
       !Config.AllocSectionsPrefix.empty() || !Config.DumpSection.empty() ||
       !Config.KeepSection.empty() || Config.NewSymbolVisibility ||
       !Config.SymbolsToGlobalize.empty() || !Config.SymbolsToKeep.empty() ||
       !Config.SymbolsToLocalize.empty() || !Config.SymbolsToWeaken.empty() ||
       !Config.SymbolsToKeepGlobal.empty() || !Config.SectionsToRename.empty() ||
       !Config.SetSectionAlignment.empty() || !Config.SetSectionFlags.empty() ||
-      !Config.SymbolsToRename.empty() || Config.ExtractDWO ||
-      Config.KeepFileSymbols || Config.LocalizeHidden || Config.PreserveDates ||
-      Config.StripDWO || Config.StripNonAlloc || Config.StripSections ||
-      Config.Weaken || Config.DecompressDebugSections ||
+      Config.ExtractDWO || Config.KeepFileSymbols || Config.LocalizeHidden ||
+      Config.PreserveDates || Config.StripDWO || Config.StripNonAlloc ||
+      Config.StripSections || Config.Weaken || Config.DecompressDebugSections ||
       Config.DiscardMode == DiscardType::Locals ||
       !Config.SymbolsToAdd.empty() || Config.EntryExpr) {
     return createStringError(llvm::errc::invalid_argument,
                              "option not supported by llvm-objcopy for COFF");
   }
 
   return Error::success();
 }