[PATCH] D75131: [llvm-objdump][XCOFF][AIX] Implement -r option

[Jason Liu via Phabricator via llvm-commits]

jasonliu marked 2 inline comments as done.
jasonliu added inline comments.


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


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


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

https://reviews.llvm.org/D75131