[PATCH] D67008: implement parsing relocation information for 32-bit xcoff objectfile

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:152
+// Relocation types, defined in `/usr/include/reloc.h`.
+enum RelocationType : uint8_t {
+  R_POS = 0x00,
----------------
We should be documenting what each of these relocation types represents. 


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:173
+// the instruction the relocation refers to.
+template <typename T> bool isFixupIndicated(T Reloc) {
+  return Reloc.Info & XR_FIXUP_INDICATOR_MASK;
----------------
DiggerLin wrote:
> sfertile wrote:
> > I imagine this (and `getRelocatedLength`) was templated to handle both `XCOFFRelocation32` and `XCOFFRelocation64` eventually? Lets just take an argument of ` XCOFFRelocation32` for now,  and template them once we land  64-bit object file relocation support. 
> yes, all template here will be used to support XCOFFRelocation32 and XCOFFRelocation64. we support 32bits now, we will support 64bits later.
The problem is that we are now reviewing these functions in a context that doesn't exist yet: ie for both 32-bit relocation representation (in this patch) and 64-bit relocation representation (which is yet to be added). I think we can make an educated guess what the 64-bit relocation struct will look like, and review in that context but it wont be so obvious to other readers of the file in the meantime (unitl 64-bit support lands). To be pedantic we should either add 64-bit support along side the 32-bit support (which would mean landing 64-bit symbol table support first), or stick to the existing concrete type. (Ditto on `XCOFFDumper::printRelocations`).


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:655
+XCOFFObjectFile::relocations(const XCOFFSectionHeader32 &Sec) const {
+  uintptr_t RelocAddr = getWithOffset(reinterpret_cast<uintptr_t>(FileHeader),
+                                      Sec.FileOffsetToRelocationInfo);
----------------
hubert.reinterpretcast wrote:
> getWithOffset does not do any bounds checking. Please ensure bounds checking.
report_fatal_error() for 64-bit object files.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:140
+    if (Relocations.empty())
+      continue;
+
----------------
Your section index will get out of sync, since we continue (to the next section) but don't increment the index.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67008