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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.
Herald added a subscriber: wuzish.


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:41
+  switch (Type) {
+    RELOC_CASE(R_POS)
+    RELOC_CASE(R_RL)
----------------
Most architectures use the names R_<ARCH>_... for their relocation names (e.g. R_X86_64_NONE). Is that not the case for XCOFF?


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:65
+  default:
+    report_fatal_error("Unhandled relocation type.");
+  }
----------------
This seems fishy. If I was to create a XCOFF object file with a relocation with an unknown type value, would I get this error when e.g. dumping relocations?

My feeling is that anything that uses `report_fatal_error` is a bug waiting to happen. Better would be to return some string indicating an unknown relocation type.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:322
 relocation_iterator XCOFFObjectFile::section_rel_begin(DataRefImpl Sec) const {
-  llvm_unreachable("Not yet implemented!");
-  return relocation_iterator(RelocationRef());
+  assert(!is64Bit() && "64-bit support not implemented yet.");
+  const XCOFFSectionHeader32* SectionEntPtr = toSection32(Sec);
----------------
Does other code further up the stack check to make sure the file is not 64 bits? If not, this isn't a valid assertion, since the program could reach this point. What would happen then if assertions are disabled?

Same comment goes for all the other instances of this.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:362
+  }
+  llvm_unreachable("Cannot find the containing section for this relocation.");
 }
----------------
Is this really unreachable? Could you get to here with an object with some invalid relocation or section property?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:369
+  const uint32_t Index = Reloc->SymbolIndex;
+  assert(Index < getLogicalNumberOfSymbolTableEntries32() && "Symbol index is out of bounds.");
+
----------------
clang-format. But should this really be an assertion rather than an llvm::Error or similar?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll:450
 ; DIS-NEXT:       84: 00 00 00 44                   <unknown>
+
+
----------------
Nit: remove the second blank line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75131