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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 4 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:158
+  R_TOC = 0x03,
+  R_TRL = 0x12,
+  R_TRLA = 0x13,
----------------
sfertile wrote:
> DiggerLin wrote:
> > sfertile wrote:
> > > This is 0x4 in the [[ https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/filesreference/XCOFF.html#XCOFF__sua3i125jbau | XCOFF Object Format Docs]], do they need to be updated or is the patch wrong?
> > I think it is Doc wrong, The value in the patch come from aix OS header file . I  opened ticket to confirm it.
> Thanks.
I have confirmed from the aix OS developer, the value in the doc is wrong , they will fix it. 


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:660
+  return ArrayRef<XCOFFRelocation32>(StartReloc,
+                                     StartReloc + Sec.NumberOfRelocations);
+}
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > sfertile wrote:
> > > DiggerLin wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > DiggerLin wrote:
> > > > > > hubert.reinterpretcast wrote:
> > > > > > > This should use the logical number of relocations, not the raw field value.
> > > > > > there is no logical number of relocation , it defined as 
> > > > > > support::ubig16_t NumberOfRelocations; in the struct XCOFFSectionHeader32 {
> > > > > "In an XCOFF32 file, if more than 65,534 relocation entries are required, the field value will be 65535, and an STYP_OVRFLO section header will contain the actual count of relocation entries in the s_paddr field."
> > > > for the relocation entry or line number entry overflow. I think we need to add a new patch for it. for the interpret of fields of Overflow section also be changed. We need to  display Overflow section as separation(other than use current display source code). 
> > > > s_nreloc (Overflow section)
> > > > Specifies the file section number of the section header that overflowed; that is, the section header containing a value of 65535 in its s_nreloc and s_nlnno fields. This value provides a reference to the primary section header. This field must have the same value as the s_nlnno field.
> > > > 
> > > > s_paddr (Overflow section)
> > > > Specifies the number of relocation entries actually required. This field is used instead of the s_nreloc field of the section header that overflowed.
> > > > s_vaddr (Overflow section)
> > > > Specifies the number of line-number entries actually required. This field is used instead of the s_nlnno field of the section header that overflowed.
> > > > 
> > > > I added a TODO:  in the code.
> > > Would it be worth while to loop over the section header table after parsing it and emit a fatal error if any sections have overflowed (either their relocation or their line number infos). I agree any handling of  overflow sections must be added in a separate patch. The error I am suggesting would also need to be landed in a separate patch from this, but it should be a very quick patch to review and commit.
> > if a relocation is overflow , I think we still can print out part of the relocation information which is under 65535 (even if we do not know exactly number of the relocation entry at this moment)
> We don't know the number, and it is not immediately obvious (because the relocation might not be the field whose logical value didn't fit) that the value would be at least 65535, so the patch Sean suggested makes sense.
I have added some new code to deal with the relocation entries overflow.


================
Comment at: llvm/test/tools/llvm-readobj/xcoff-basic.test:128
+
+# RELOCSEXP:           Virtual Address: 0x110
+# RELOCSEXP-NEXT:      Symbol: J (96)
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > hubert.reinterpretcast wrote:
> > > This reduction is odd. Two kinds of TLS-related relocations are represented by the `.o` file, but we don't check for them. Please also add in the relocation referencing the unnamed `[RO]` csect.
> > changed as suggestion.
> To be more clear: There are two TLS-related relocation kinds represented in the object file, and neither are checked in this test. Please check for them.
added, thanks


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:140
+
+    Index++;
+    if (Relocations.empty())
----------------
hubert.reinterpretcast wrote:
> https://llvm.org/docs/CodingStandards.html#prefer-preincrement: `++Index`.
changed as suggestion


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67008