[llvm-objcopy] Reorder --dump-section for MachO.

[sameerarora101]

Summary

Reorder DumpSection under handleArgs in
file MachOObjcopy.cpp. The operation to dump
a section is now performed before both add and remove
section operations for MachO file format.

Change for the ELF format at D81097. Together fixes https://bugs.llvm.org/show_bug.cgi?id=44283

[jhenderson]

When I originally filed the bug, this was an issue in ELF too. Could you check the other formats, specifically ELF, but other formats too if they support --dump-section, to make sure they behave the right way, please? If not, they'll want fixing, but you can do that in a separate patch (but the bug isn't resolved until it works for all formats).

llvm/test/tools/llvm-objcopy/MachO/dump-before-add-remove.test

3

We prefer to use '##' for comments in newer tools tests, to make them stand out from check and run lines. Please update.

5

If you make the section content something ASCII, you could use FileCheck directly on the file to avoid the need for od and wc:

# FileCheck %s --input-file=%t1.dump.txt --check-prefix=CONTENTS --implicit-check-not={{.}}
# CONTENTS: abcd

11

NEW -> newly

Also, I don't think you need the extra emphasis of captialisation of NEW/NOT.

14

As you're not reusing the NODUMP prefix, you can hard code the value of SECTION into the check directly rather than passing it in as a variable.

[sameerarora101]

In D81123#2072964, @jhenderson wrote:

When I originally filed the bug, this was an issue in ELF too. Could you check the other formats, specifically ELF, but other formats too if they support --dump-section, to make sure they behave the right way, please? If not, they'll want fixing, but you can do that in a separate patch (but the bug isn't resolved until it works for all formats).

Yup, I have a patch for ELF at D81097
For COFF: DumpSection options isn't supported and
for wasm: DumpSection is already being performed before add/remove.

So I think D81123 (MachO) and D81097 (ELF) together fix the bug?

[sameerarora101]

llvm/test/tools/llvm-objcopy/MachO/dump-before-add-remove.test

5

Really cool, thanks 😊 .

[sameerarora101]

Incorporating inline comments above and from D81097.

[MaskRay]

Mostly good

llvm/test/tools/llvm-objcopy/MachO/dump-section-before-add-remove.test

4 ↗

(On Diff #268484)

There is no need to use different numbering (%t1 %t2) for the same command.
Actually, it is preferred to use the same numbering for associated output (%t1.dump.text and %t2) in this case.

.dump.text can be simplified to .dump . You don't need two extensions.

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

190

The two lines should be joined.

clang-format can format that for you.

195

'='

[MaskRay]

. Fixes PR44283

You can move this part from the subject to the body.

[sameerarora101]

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

190

Do you mean on the same line like // Dump sections before add/remove for compatibility with GNU objcopy. ?

[sameerarora101]

Updating %ts to use the same number for a single command.

[sameerarora101]

Fixing previous update

[jhenderson]

LGTM.

[jhenderson]

Actually, one request for the commit message: could you change "PR44283" to an actual URL, please (i.e. https://bugs.llvm.org/show_bug.cgi?id=44283). It makes it easier to look up the bug in the future. Same goes in the ELF case.

[sameerarora101]

Updating comment: "before it is removed".

llvm/test/tools/llvm-objcopy/MachO/dump-before-add-remove.test

+# RUN: yaml2obj %s -o %t
+
+# Verify that section is dumped before remove.
+# RUN: llvm-objcopy --dump-section __TEXT,__text=%t1.dump.text --remove-section __TEXT,__text %t %t2
+# RUN: od -t x1 %t1.dump.text | FileCheck %s --ignore-case
+# RUN: wc -c %t1.dump.text | FileCheck %s --check-prefix=SIZE
+
+# CHECK: 0000000 aa bb cc dd
+# SIZE: 4
+
+# Verify that a NEW added section is NOT dumped.
+# RUN: echo CAFE > %t1.txt
+# RUN: not llvm-objcopy --dump-section __TEXT,__const=%t3.txt --add-section __TEXT,__const=%t1.txt %t %t4 2>&1 | \
+# RUN:     FileCheck %s --check-prefix=NODUMP -DINPUT=%t -DSECTION=__TEXT,__const
+
+# NODUMP: error: '[[INPUT]]': section '[[SECTION]]' not found
+
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x00000003
+  filetype:        0x00000001
+  ncmds:           1
+  sizeofcmds:      312
+  flags:           0x00002000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         312
+    segname:         ''
+    vmaddr:          0
+    vmsize:          12
+    fileoff:         344
+    filesize:        12
+    maxprot:         7
+    initprot:        7
+    nsects:          3
+    flags:           0
+    Sections:
+      - sectname:        __text
+        segname:         __TEXT
+        addr:            0x0000000000000000
+        content:         'AABBCCDD'
+        size:            4
+        offset:          344
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x80000400
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+      - sectname:        __data
+        segname:         __DATA
+        addr:            0x0000000000000004
+        content:         'EEFFEEFF'
+        size:            4
+        offset:          348
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x00000000
+        reserved1:       0x00000000
+        reserved2:       0x00000000

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

if (Config.AllowBrokenLinks || !Config.BuildIdLinkDir.empty() ||
       Config.StripSections || Config.Weaken || Config.DecompressDebugSections ||
       Config.StripNonAlloc || Config.StripSections || Config.StripUnneeded ||
       Config.DiscardMode == DiscardType::Locals ||
       !Config.SymbolsToAdd.empty() || Config.EntryExpr) {
     return createStringError(llvm::errc::invalid_argument,
                              "option not supported by llvm-objcopy for MachO");
   }
 
+  // Section must be dumped before add/remove for
+  // compatibility with GNU objcopy.
+  for (const StringRef &Flag : Config.DumpSection) {
+    StringRef SectionName;
+    StringRef FileName;
+    std::tie(SectionName, FileName) = Flag.split("=");
+    if (Error E = dumpSectionToFile(SectionName, FileName, Obj))
+      return E;
+  }
+
   if (Error E = removeSections(Config, Obj))
     return E;
 
   // Mark symbols to determine which symbols are still needed.
   if (Config.StripAll)
     markSymbols(Config, Obj);
 
   updateAndRemoveSymbols(Config, Obj);
 
   if (Config.StripAll)
     for (LoadCommand &LC : Obj.LoadCommands)
       for (std::unique_ptr<Section> &Sec : LC.Sections)
         Sec->Relocations.clear();
 
-  for (const StringRef &Flag : Config.DumpSection) {
-    std::pair<StringRef, StringRef> SecPair = Flag.split("=");
-    StringRef SecName = SecPair.first;
-    StringRef File = SecPair.second;
-    if (Error E = dumpSectionToFile(SecName, File, Obj))
-      return E;
-  }
-
   for (const auto &Flag : Config.AddSection) {
     std::pair<StringRef, StringRef> SecPair = Flag.split("=");
     StringRef SecName = SecPair.first;
     StringRef File = SecPair.second;
     if (Error E = isValidMachOCannonicalName(SecName))
       return E;
     if (Error E = addSection(SecName, File, Obj))
       return E;