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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


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


================
Comment at: llvm/test/tools/llvm-objdump/ELF/private-headers.test:9
+# CHECK-NEXT: filesz 0x0000000000000020 memsz 0x0000000000000020 flags ---
+# CHECK: Dynamic Section:
+# CHECK-NEXT:  NEEDED bar
----------------
Nit: here and below, add additional spaces between CHECK: and "Dynamic Section"/"Version definitions" to make things nicely line up.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/private-headers.test:26-27
+    Address: 0x1000
+    Size:    0x38
+    Content: "0062617200666F6F0056455253494F4E5F320056455253494F4E5F3100"
+  - Name:    .dynamic
----------------
If memory serves me correctly, the combination of a size and content will tail-pad the content with null bytes to make the appropriate size. I'm guessing this isn't what you wanted?

It would be helpful to have a comment spelling out what this content represents.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/private-headers.test:30
+    Type:    SHT_DYNAMIC
+    Address: 0x1010
+    Link:    1
----------------
This looks awfully suspicious. Your previous section starts at address 0x1000 but is more than 0x10 bytes in size, so this address couldn't possibly be correct. You don't actually need to explicitly specify the address - yaml2obj will automatically increment it as necessary (but you'll need to add the SHF_ALLOC section flag, which you want anyway, since these are sections with addresses).


================
Comment at: llvm/test/tools/llvm-objdump/ELF/private-headers.test:31
+    Address: 0x1010
+    Link:    1
+    Entries:
----------------
I might be wrong, but I believe yaml2obj automatically assigns the Link value for .dynamic to .dynstr. Even if it doesn't, you should be able to specify the section name rather than the index.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/private-headers.test:39-40
+    Type:            SHT_GNU_verdef
+    Flags:           [ SHF_ALLOC ]
+    AddressAlign:    0x0000000000000004
+    Entries:
----------------
I think you probably don't need these fields?


================
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
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103974