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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 2 inline comments as done.
DiggerLin 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,
----------------
sfertile wrote:
> DiggerLin wrote:
> > sfertile wrote:
> > > We should be documenting what each of these relocation types represents. 
> > added
> Thanks for commenting these Digger. I see you used the descriptions from `reloc.h`. Those work. In some cases I think we can do a bit better on the descriptions if we crib [[ https://www.ibm.com/support/knowledgecenter/en/ssw_aix_72/filesreference/XCOFF.html#XCOFF__sua3i125jbau | from here ]]. I can write up my suggestions and then run them by you before we attempt to update any though.
OK, thanks please  share with suggestions.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:660
+  return ArrayRef<XCOFFRelocation32>(StartReloc,
+                                     StartReloc + Sec.NumberOfRelocations);
+}
----------------
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)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67008