[PATCH] D75131: [llvm-objdump][XCOFF][AIX] Implement -r option
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 25 02:08:16 PDT 2020
jhenderson added a comment.
Thanks for the effort. This is nearly there.
================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-all.test:7-8
+
+# RUN: llvm-objdump -d -r %p/Inputs/xcoff-section-headers.o | \
+# RUN: FileCheck --check-prefix=CHECK-d-r %s
----------------
`-d -r` should be tested in a test for the `--disassemble` option, not `--disassemble-all`. Arguably in fact, you don't need it tested at all, as the `-D -r` combination is probably sufficient.
================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-all.test:23
+CHECK: Inputs/xcoff-section-headers.o: file format aixcoff-rs6000
+CHECK: Disassembly of section .text:
+CHECK: 00000000 <.text>:
----------------
hubert.reinterpretcast wrote:
> jasonliu wrote:
> > hubert.reinterpretcast wrote:
> > > jasonliu wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > I think additional prefixes can be added for the `.text` part to common up the `-D -r` and the `-d -r`.
> > > > Common up for the two cases make sense as they are identical for 95% of the time right now, and does not need much effort to parse both of the them.
> > > > Common up for all three cases and adding more prefix actually decrease the readability of the test case in my mind.
> > > > So if people are not feeling strong about it, I'd prefer to leave it as it is.
> > > Just for sketching out the effect, the end result would be using:
> > > `TEXT,ALL,ALL-NO-R`
> > > `TEXT,ALL,TEXT-R,ALL-R`
> > > `TEXT,TEXT-R`
> > >
> > > With:
> > > ```
> > > TEXT: Inputs/xcoff-section-headers.o: file format aixcoff-rs6000
> > > TEXT: Disassembly of section .text:
> > > TEXT: 00000000 <.text>:
> > > TEXT-NEXT: 0: 80 62 00 04 lwz 3, 4(2)
> > > TEXT-R-NEXT: 00000002: R_TOC a
> > > TEXT-NEXT: 4: 80 63 00 00 lwz 3, 0(3)
> > > TEXT-NEXT: 8: 4e 80 00 20 blr
> > > TEXT-NEXT: c: 00 00 00 00 <unknown>
> > > TEXT-NEXT: 10: 00 00 20 40 <unknown>
> > > TEXT-NEXT: 14: 00 00 00 01 <unknown>
> > > TEXT-NEXT: 18: 00 00 00 0c <unknown>
> > > TEXT-NEXT: 1c: 00 04 66 75 <unknown>
> > > TEXT-NEXT: 20: 6e 63 00 00 xoris 3, 19, 0
> > > TEXT-NEXT: ...
> > > ALL: Disassembly of section .data:
> > > ALL: 00000080 <func>:
> > > ALL-NEXT: 80: 00 00 00 94 <unknown>
> > > ALL: 00000084 <a>:
> > > ALL-NEXT: 84: 00 00 00 a4 <unknown>
> > > ALL: 00000088 <b>:
> > > ALL-NEXT: 88: 00 00 00 a0 <unknown>
> > > ALL: 0000008c <c>:
> > > ALL-NEXT: 8c: 00 00 00 08 <unknown>
> > > ALL: 00000090 <d>:
> > > ALL-NO-R-NEXT: 90: 00 00 00 00 <unknown>
> > > ALL-R-NEXT: ...
> > > ALL: 00000094 <func>:
> > > ALL-NEXT: 94: 00 00 00 00 <unknown>
> > > ALL-NEXT: 98: 00 00 00 80 <unknown>
> > > ALL-NEXT: 9c: 00 00 00 00 <unknown>
> > > ALL: 000000a0 <b>:
> > > ALL-NEXT: a0: 00 00 30 39 <unknown>
> > > ALL: Disassembly of section .bss:
> > > ALL: 000000a4 <a>:
> > > ALL-NEXT: ...
> > > ALL: Disassembly of section .tdata:
> > > ALL: 00000000 <d>:
> > > ALL-NEXT: 0: 40 09 21 f9 bdnzfl 9, .+8696
> > > ALL-NEXT: 4: f0 1b 86 6e <unknown>
> > > ALL: Disassembly of section .tbss:
> > > ALL: 00000008 <c>:
> > > ALL-NEXT: ...
> > > ```
> > Thanks for generating the proposed result.
> > Comparing it side by side confirms my worries... It took extra effort to understand what each test expects, comparing the current one we have right now.
> Okay, thanks for looking.
So I think the question is really, what is the purpose of the test? Is it to allow easy comparison of two outputs or something else? As things stand, it isn't easy to compare said two outputs.
This is of course slightly moot, since there really should only be two test cases in this file anyway (see my comment above).
================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/print-reloc.test:2
+# RUN: llvm-objdump -r %p/Inputs/xcoff-section-headers.o | \
+# RUN: FileCheck --check-prefix=RELOC %s
+
----------------
jasonliu wrote:
> hubert.reinterpretcast wrote:
> > Minor nit: No need to use a `--check-prefix` now that this is a separate file.
> Thanks. I will address it in the commit if I don't receive other comments.
You should probably add --match-full-lines and --strict-whitespace to this FileCheck call, especially the former of these. This might require you to do some extra indentation stuff to make things look okay (see below - it ensures that all output lines up):
```
RELOC:Inputs/xcoff-section-headers.o: file format aixcoff-rs6000
RELOC:RELOCATION RECORDS FOR [.text]:
RELOC-NEXT:OFFSET TYPE VALUE
RELOC-NEXT:00000002 R_TOC a
RELOC:RELOCATION RECORDS FOR [.data]:
RELOC-NEXT:OFFSET TYPE VALUE
...
```
================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/print-reloc.test:4-14
+# xcoff-section-headers.o Compiled with IBM XL C/C++ for AIX, V16.1.0
+# compiler command: xlc -qtls -o xcoff-section-headers.o -c test.c
+# test.c:
+# int a;
+# int b = 12345;
+# __thread int c;
+# __thread double d = 3.14159;
----------------
Use '##' for comments.
================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/print-reloc.test:16
+
+; REQUIRES: powerpc-registered-target
+RELOC: Inputs/xcoff-section-headers.o: file format aixcoff-rs6000
----------------
';' -> '#' for consistency with other lit directives. Also, this should be at the top of the file as is common for REQUIRES directives.
================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/print-reloc.test:20
+RELOC-NEXT: OFFSET TYPE VALUE
+RELOC-NEXT: 00000002 R_TOC a
+RELOC: RELOCATION RECORDS FOR [.data]:
----------------
Add `RELOC-EMPTY` here and after the last relocation in the other block to show that all relocations have been printed.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75131/new/
https://reviews.llvm.org/D75131
More information about the llvm-commits
mailing list