[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,
----------------
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.


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67008