[PATCH] D81123: [llvm-objcopy] Reorder --dump-section for MachO. Fixes PR44283

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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).



================
Comment at: llvm/test/tools/llvm-objcopy/MachO/dump-before-add-remove.test:3
+
+# Verify that section is dumped before remove.
+# RUN: llvm-objcopy --dump-section __TEXT,__text=%t1.dump.text --remove-section __TEXT,__text %t %t2
----------------
We prefer to use '##' for comments in newer tools tests, to make them stand out from check and run lines. Please update.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/dump-before-add-remove.test:5
+# 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
----------------
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```


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/dump-before-add-remove.test:11
+
+# Verify that a NEW added section is NOT dumped.
+# RUN: echo CAFE > %t1.txt
----------------
NEW -> newly

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


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/dump-before-add-remove.test:14
+# 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
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81123/new/

https://reviews.llvm.org/D81123