[PATCH] D81097: [llvm-objcopy] Reorder --dump-section before --remove-section for ELF. Fixes PR44283

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/dump-before-add-remove.test:1
+# RUN: yaml2obj %s -o %t
+
----------------
Perhaps `dump-section-before-add-remove`



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/dump-before-add-remove.test:3
+
+# Verify that section is dumped before remove.
+# RUN: llvm-objcopy --dump-section .test2=%t1 -R .test2 %t %t2
----------------
`## `

For new tests we use `## ` to make comments stand out from RUN/CHECK lines.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/dump-before-add-remove.test:6
+# RUN: od -t x1 %t1 | FileCheck %s --ignore-case
+# RUN: wc -c %t1 | FileCheck %s --check-prefix=SIZE
+
----------------
You can dump the second line of `od` output (it prints the size) and delete `wc -c`


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


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/dump-before-add-remove.test:11
+# RUN: not llvm-objcopy --dump-section .test3=%t3.txt --add-section .test3=%t1.txt %t %t4 2>&1 | \
+# RUN:     FileCheck %s --check-prefix=NODUMP -DINPUT=%t -DSECTION=.test3
+
----------------
alexshap wrote:
> nit: it looks like it's the only place where these macros (INPUT, SECTION) are used. In this case we don't really need them, simply use the corresponding values directly (on the line 37)
I think we typically place CHECK lines before the tests. Some earlier tests do not follow the convention, but newer tests can.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/dump-before-add-remove.test:18
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
----------------
Delete excess indentation. Keep one just after `Machine: `, the longest key.

ditto below


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:620
 
+  // Section must be dumped before add/remove for
+  // compatibility with GNU objcopy.
----------------
Section must be dumped -> Dump sections


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81097