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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:252
+  static constexpr uint64_t InvalidRelocOffset =
+      std::numeric_limits<uint64_t>::max();
+
----------------
hubert.reinterpretcast wrote:
> MaskRay wrote:
> > hubert.reinterpretcast wrote:
> > > `0xffffffff'ffffffffULL`
> > Will UINT64_C(-1) be clearer?
> You mean `-UINT64_C(1)`? I don't think so. My experience is that people are not familiar with these macros.
I would use std::numeric_limits<uint64_t>::max(), since it's clearer to me than a literal. It's meaning seems obvious.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:284-287
+  /// Returns the relocation offset with the base address of the containing
+  /// section as zero.
+  /// Returns InvalidRelocOffset on errors (such as a
+  /// relocation that does not refer to an address in any section).
----------------
Please reflow this comment so that it isn't weirdly spaced (especially the end of the third line).

I'd actually recommend another change too:

"\returns the relocation offset with the base address of the containing section as zero, or InvalidRelocOffset on error (for example for a relocation that does not refer to an address in any section)."

The "\returns" style is a doxygen thing.


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


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:18-20
+enum { RELOC_OVERFLOW = 65535 };
+enum : uint8_t { FUNCTION_SYM = 0x20, SYM_TYPE_MASK = 0x07 };
+enum : uint16_t { NO_REL_MASK = 0x0001 };
----------------
At this point would it just make more sense to make these static constant literals?

```
// TODO: Use appropriate naming convention - are these spec-defined, or just local names?
static const int RELOC_OVERFLOW = 65535; // Use an appropriate type here.
static const uint8_t FUNCTION_SYM = 0x20;
static const uint8_t SYM_TYPE_MASK = 0x07;
static const uint16_t NO_REL_MASK = 0x0001;
```



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:359
+    if (Sec32->VirtualAddress <= RelocAddress &&
+        RelocAddress < (Sec32->VirtualAddress + Sec32->SectionSize)) {
+      return RelocAddress - Sec32->VirtualAddress;
----------------
I'm pretty certain you don't need the parentheses here.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-all.test:1-5
+# RUN: llvm-objdump -D -r %p/Inputs/xcoff-section-headers.o | \
 # RUN: FileCheck %s
 
+# RUN: llvm-objdump -r %p/Inputs/xcoff-section-headers.o | \
+# RUN: FileCheck --check-prefix=RELOC %s
----------------
Please don't test -r + -D instead of -D on its own. -D should be tested on its own. -r should be tested on its own in a separate test. I'd also recommend -r + -d be tested there too (or that could even be a different test file if you want).


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

https://reviews.llvm.org/D75131