[PATCH] D75131: [XCOFF][AIX] Enable -r option for llvm-objdump
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 28 01:04:34 PST 2020
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
More information about the llvm-commits
mailing list