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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast accepted this revision.
hubert.reinterpretcast marked an inline comment as done.
hubert.reinterpretcast added a comment.

Just confirming that this LGTM (with the minor comment posted before). Please wait to see if @jhenderson agrees that his comments have been addressed.



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


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

https://reviews.llvm.org/D75131