[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 ↗

(On Diff #268333)

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

5 ↗

(On Diff #268333)

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 ↗

(On Diff #268333)

NEW -> newly

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

14 ↗

(On Diff #268333)

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 ↗

(On Diff #268333)

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

5

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-section-before-add-remove.test

+# RUN: yaml2obj %s -o %t
+
+## Verify that section is dumped before it is removed.
+# RUN: llvm-objcopy --dump-section __TEXT,__text=%t1.dump --remove-section __TEXT,__text %t %t1
+# RUN: FileCheck %s --input-file=%t1.dump --check-prefix=CONTENTS --implicit-check-not={{.}}
+
+# CONTENTS: abcd
+
+## Verify that the newly added section is not dumped.
+# RUN: echo CAFE > %t2.txt
+# RUN: not llvm-objcopy --dump-section __TEXT,__const=%t2.dump --add-section __TEXT,__const=%t2.txt %t %t2 2>&1 | \
+# RUN:     FileCheck %s --check-prefix=NODUMP -DINPUT=%t
+
+# NODUMP: error: '[[INPUT]]': section '__TEXT,__const' 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:    '61626364'
+        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");
   }
 
+  // Dump sections before add/remove for compatibility with GNU objcopy.
+  for (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;