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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:324
+  if (is64Bit())
+    report_fatal_error("64-bit support not implemented yet.");
+  const XCOFFSectionHeader32* SectionEntPtr = toSection32(Sec);
----------------
hubert.reinterpretcast wrote:
> jasonliu wrote:
> > MaskRay wrote:
> > > hubert.reinterpretcast wrote:
> > > > jhenderson wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > jhenderson wrote:
> > > > > > > Here and all report_fatal_error calls: remove the trailing full stop.
> > > > > > We have been trying to full stops for XCOFF work for invocations of report_fatal_error, llvm_unreachable, and assert in order to be consistent within our patches and after having observed that such uses are not uncommon.
> > > > > > 
> > > > > > For example:
> > > > > > ```
> > > > > > llvm/lib/Object/COFFObjectFile.cpp:    report_fatal_error("Section was outside of section table.");
> > > > > > llvm/lib/Object/COFFObjectFile.cpp:      report_fatal_error("Aux Symbol data was outside of symbol table.");
> > > > > > llvm/lib/Object/MachOObjectFile.cpp:    report_fatal_error("Malformed MachO file.");
> > > > > > llvm/lib/Object/MachOObjectFile.cpp:    report_fatal_error("Requested symbol index is out of range.");
> > > > > > ```
> > > > > They may not be uncommon, because people haven't been particularly careful with how they write error messages in the past. Whereever possible, I've tried to encourage people to adopt a single consistent style - note that most (all?) of our error messages in clang, LLD and various tools like llvm-readobj and llvm-objdump use lower-case and no trailing full stop. report_fatal_error calls should be no different.
> > > > @jhenderson, I agree that a single consistent style would be great. I note that the Coding Standards do not document this style. It is also unfortunate that this style is not consistent, in respect to the capitalization of the first word, with the examples in the Coding Standards for strings in `assert` and `llvm_unreachable`. If you find consensus to update the Coding Standards to document this style, it would probably help to improve the consistency in the project.
> > > Please stop capitalizing and stop using a trailing full stop. For the common infrastructure of LLVMObject and binary utilities where James and several other contributors spending a lot of time on, we have made lots of efforts improving the diagnostics and their consistency. As a relatively new user of these infrastructure, it'd be much appreciated if XCOFF could follow suite. I hope this XCOFF development in Object won't be sort of code dump.
> > We are good with the consistent style. And I will make the change. We are just asking if it make sense to update Coding Standards to document the style as right now it's not all consistent through out the project, and it's easy to follow the wrong precedent. 
> > >  I hope this XCOFF development in Object won't be sort of code dump.
> > I don't find this comment to be very constructive. 
> > 
> > 
> I believe that this is the first patch for which we were told about this convention. No one had said we were not changing this after James indicated that he believes it to be an important point. I find the insinuation that a "code dump" is being attempted to be rather extreme.
Thanks for making the change. I'll post a question on llvm-dev about whether we should update the coding standard. I guess `assert` and `llvm_unreachable` are a little different, because of how they are printed. I'll do some investigation and write a full proposal up.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-all.test:2
 # RUN: llvm-objdump -D %p/Inputs/xcoff-section-headers.o | \
 # RUN: FileCheck %s
 
----------------
Indent continuation lines by two extra spaces to show they are clearly continuations:

```
# RUN: llvm-objdump -D %p/Inputs/xcoff-section-headers.o | \
# RUN:   FileCheck %s
```

I'm suggesting it for all the checks.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-all.test:10-11
+
+# RUN: llvm-objdump -r %p/Inputs/xcoff-section-headers.o | \
+# RUN: FileCheck --check-prefix=RELOC %s
+
----------------
As mentioned, I think this case should be moved to a different test file, as it's really testing something unrelated.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-all.test:66
+
+CHECK-D-r: Inputs/xcoff-section-headers.o:	file format aixcoff-rs6000
+CHECK-D-r: Disassembly of section .text:
----------------
MaskRay wrote:
> It may look slightly better if you add enough spaces after `:` to make the first letter on the right of `CHECK-D-r-NEXT:`
I'd recommended using FileCheck's `--check-prefixes` option to fold these two sequences together, as it will more clearly show the differences.

Example:
```
# RUN: llvm-objdump -D %p/Inputs/xcoff-section-headers.o | \
# RUN:   FileCheck %s

# RUN: llvm-objdump -D -r %p/Inputs/xcoff-section-headers.o | \
# RUN:   FileCheck %s --check-prefixes=CHECK,WITH-R

CHECK:       Inputs/xcoff-section-headers.o:  file format aixcoff-rs6000
CHECK:       Disassembly of section .text:
CHECK:       00000000 <.text>:
CHECK-NEXT:         0: 80 62 00 04                    lwz 3, 4(2)
WITH-R-NEXT:                         00000002:  R_TOC       
CHECK-NEXT:         4: 80 63 00 00                    lwz 3, 0(3) 
```


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

https://reviews.llvm.org/D75131