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

[Sean Fertile via Phabricator via llvm-commits]

sfertile marked an inline comment as done.
sfertile added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:168
+
+bool isRelocationSigned(XCOFFRelocation32 &Reloc);
+
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > hubert.reinterpretcast wrote:
> > > Do these need to be declared in the header? Are they called only in one `.cpp` file? If so, they can be made `static` in the `.cpp` file. Otherwise, it seems odd that these aren't `const` member functions of `XCOFFRelocation32`.
> > the llvm-readobj is using those function and obj2yaml will use them too.
> It is still odd to me that these aren't `const` non-static member functions of `XCOFFRelocation32`.
I think were these originally templated to work with both 32-bit and 64-bit relocations, which explains why they aren't member functions.


================
Comment at: llvm/test/tools/llvm-readobj/reloc_overflow.ll:1
+# RUN: llvm-readobj --sections %p/Inputs/xcoff-reloc-overflow.o | \
+# RUN: FileCheck --check-prefix=SECOVERFLOW %s
----------------
DiggerLin wrote:
> sfertile wrote:
> > The `.ll` suffix implies the test is written in LLVM IR. Use `.test` instead. 
> I will change the name
I still see the `.ll` suffix.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:140
+    // Only the .text, .data, .tdata, and STYP_DWARF sections have relocation.
+    if (Sec.Flags != XCOFF::STYP_TEXT && Sec.Flags != XCOFF::STYP_DATA &&
+        Sec.Flags != XCOFF::STYP_TDATA && Sec.Flags != XCOFF::STYP_DWARF)
----------------
DiggerLin wrote:
> sfertile wrote:
> > Is this specified in the docs? I wasn't able to find it specified anywhere. What about the exception section?  I don't know anything about the exception implementation on AIX so I could be wrong, but I suspect it might contain relocations. 
> > 
> > I did find the specification of the special relocations in the loader table, and that they are a different format from the 'normal' relocations implemented in this patch. Does the loader section use the relocation pointer and relocation count in the section header table for these different relocations, or do we find them through fields defined in the loader section  itself?
> from the xcoff document.
> s_relptr	Recognized for the .text, .data, .tdata, and STYP_DWARF sections only.
Thanks.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67008