[PATCH] D78805: [llvm-readobj][test] - Stop using binaries in gnu-phdrs.test and refine it.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test:308
+
+## Check/document how we dump Arm specific program headers.
+# RUN: yaml2obj -DBITS=64 -DMACHINE=EM_ARM %s -o %tarm.elf
----------------
grimar wrote:
> jhenderson wrote:
> > Arm -> ARM
> > 
> > Could we just fix the ARM header printing so that you don't need "document" in the comment?
> To fix ARM we need to add/change a test. This patch introduces a proper place for it (`gnu-phdrs.test`)
Right, what I meant was fix it in a child commit, and not bother with the "document" part of the comment. Sorry I wasn't clear.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test:325
+
+## Check the output when an object has no section headers.
+## RUN: llvm-objcopy --strip-sections %t64.elf %tno-shdrs.o
----------------
grimar wrote:
> jhenderson wrote:
> > What's the reasoning from removing this from its separate test? I don't think that gives us anything.
> The original test uses `phdrs-elf.exe-x86_64` which is removed with this patch. 
> Also I probably see no reason for testing it separatelly either: it is not a large/complex feature.
> 
> Want me to restore `phdrs-elf.exe-x86_64` and keep `gnu-section-mapping-no-shdrs.test` unchanged?
I guess it would be okay to fold it into a dedicated test for `--section-mapping`, but I think there should be a minimum of one test per user-visible option, unless two options are intricately connected. Whilst related, the `--section-mapping` option is not intricately tied into the `--program-header` option, so it makes sense to test them separately.


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

https://reviews.llvm.org/D78805