[PATCH] D103974: [llvm-objdump] Add testing for --print-imm-hex, --headers, --section-headers and --private-headers

[Owen Reynolds via Phabricator via llvm-commits]

gbreynoo added a comment.

Thanks for your patience Maskray.



================
Comment at: llvm/test/tools/llvm-objdump/ELF/private-headers.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objdump --private-headers %t | FileCheck %s
----------------
MaskRay wrote:
> jhenderson wrote:
> > Add a comment to the start of the test briefly outlining the test purpose.
> > 
> > A quick skim suggests this is essentially the only testing for dumping this information for llvm-objdump for ELF. I'm guessing the code path is completely unrelated to the Mach-O equivalent behaviour, so it looks like it needs far more thorough testing, showing that all the various properties can be dumped correctly (e.g. all the different flags). Take a look at the equivalent llvm-readobj testing for an idea of what would be best.
> > 
> > Assuming more widespread testing is added, I'd split this up into separate testing for each of the different things (program header/version info/dynamic section) and then have a very minimal test that shows that --private-headers dumps all three.
> `{,llvm-|objdump -p / --private-headers dumps` program headers, the dynamic section, and GNU symbol versioning sections.
> 
> `dynamic-section.test` tests `-p` but there is no test in this directory testing `--private-headers`.
As MaskRay says there is already `dynamic-section.test` for the ELF dynamic section. There is `verdef.test` for version definitions and I’ve added `program-headers.test` for program header output. I saw there was no ELF specific test for use of `–private-headers` (rather than `-p`) except for `invalid-phdr.test` so a test to ensure output was as expected would be useful even if the output was partially covered in other tests. Although I originally was only going to check for the strings Program Header, Dynamic Section and Version definitions, similar to how `–headers` is tested in `llvm-readobj`, I thought extra coverage like this wouldn’t hurt?


================
Comment at: llvm/test/tools/llvm-objdump/ELF/private-headers.test:11
+# CHECK-NEXT:  NEEDED bar
+# CHECK:  Version definitions:
+# CHECK-NEXT:1 0x01 0x075bcd15 foo
----------------
MaskRay wrote:
> For versions. `llvm-readobj/ELF/` has some `llvm-readelf -V` tests which may be used as a reference.
Is this coverage sufficient considering we already have `verdef.test`? I am happy to make changes here if not.


================
Comment at: llvm/test/tools/llvm-objdump/X86/print-imm-hex.s:6
+# RUN: llvm-objdump -d --no-print-imm-hex --print-imm-hex %t | FileCheck %s --check-prefix=PRINT
+
+.text
----------------
jhenderson wrote:
> gbreynoo wrote:
> > The assembly here was taken from another test, I'm happy to change it if a better sample can be suggested.
> Which other test was this taken from and why did you pick it?
I took this from `disassemble-align.s` as it was an existing example of disassembled output that had values that can be converted to hex. 


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

https://reviews.llvm.org/D103974