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

[Dave Bozier via Phabricator via llvm-commits]

davidb added a comment.

This looks great, thanks Jason!



================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:41
+  switch (Type) {
+    RELOC_CASE(R_POS)
+    RELOC_CASE(R_RL)
----------------
jhenderson wrote:
> Most architectures use the names R_<ARCH>_... for their relocation names (e.g. R_X86_64_NONE). Is that not the case for XCOFF?
according this document these names are correct as they currently are. https://www.ibm.com/support/knowledgecenter/ssw_aix_72/filesreference/XCOFF.html#XCOFF__sua3i125jbau


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:65
+  default:
+    report_fatal_error("Unhandled relocation type.");
+  }
----------------
jhenderson wrote:
> 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.
ELF returns the string "Unknown". Will make it less painful in case this code falls out of sync with new relocs.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75131