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

Thu Sep 19 17:14:27 PDT 2019

DiggerLin added inline comments.

================
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;
----------------
sfertile wrote:
> 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`).
changed without template 

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67008