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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 11 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:
> We should be documenting what each of these relocation types represents. 
added


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:170
+  R_BA = 0x08,
+  R_BR = 0x0A;
+  R_RBA = 0x18,
----------------
hubert.reinterpretcast wrote:
> Please be consistent on whether to use lowercase hex digits or uppercase ones.
fixed


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:165
+  R_RL = 0x0c,
+  R_RLA = 0x0d,
+  R_REF = 0x0f,
----------------
sfertile wrote:
> DiggerLin wrote:
> > sfertile wrote:
> > > Minor nit: IIUC `R_RL` and 'R_RLA` are handled the same as `R_POS`, maybe we should keep them together.
> > the enum is keep in value order, I am prefer to keep them as now. 
> There are already relocations grouped together by functionality as opposed to value order: eg `R_TOC', 'R_TRL','R_TRLA` or 'R_BA`,'R_RBA','R_RBR' . Why is important to keep these grouped in value order as opposed these others that are grouped by functionality? 
changed as suggestion


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:162
+// The 2nd msb is used to indicate that the binder has replaced/modified the
+// original instuction.
+static constexpr uint8_t XR_FIXUP_INDICATOR_MASK = 0x40;
----------------
hubert.reinterpretcast wrote:
> s/instuction/instruction/;
changed , thanks


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:171
+}
+// If the Fixup bit is set, it indicates that the linker has modified
+// the instruction the relocation refers to.
----------------
hubert.reinterpretcast wrote:
> Add a blank line before the comment.
added


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:326
   void checkSymbolEntryPointer(uintptr_t SymbolEntPtr) const;
+  // Relocation related interfaces.
+  ArrayRef<XCOFFRelocation32> relocations(const XCOFFSectionHeader32 &) const;
----------------
hubert.reinterpretcast wrote:
> Add a hyphen: "Relocation-related".
added


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:499
+
+  if (Index >= getLogicalNumberOfSymbolTableEntries32())
+    return errorCodeToError(object_error::invalid_symbol_index);
----------------
hubert.reinterpretcast wrote:
> Is this value verified elsewhere against the offset of the symbol table and the buffer size? If not, then this check is not a substitute for bounds checking on the pointer arithmetic below.
yes, We already check in XCOFFObjectFile::create as

  // Parse symbol table.
  CurOffset = Obj->fileHeader32()->SymbolTableOffset;
  uint64_t SymbolTableSize = (uint64_t)(sizeof(XCOFFSymbolEntry)) *
                             Obj->getLogicalNumberOfSymbolTableEntries32();
  auto SymTableOrErr =
      getObject<XCOFFSymbolEntry>(Data, Base + CurOffset, SymbolTableSize);
  if (Error E = SymTableOrErr.takeError())
    return std::move(E);
  Obj->SymbolTblPtr = SymTableOrErr.get();
  CurOffset += SymbolTableSize;


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:655
+XCOFFObjectFile::relocations(const XCOFFSectionHeader32 &Sec) const {
+  uintptr_t RelocAddr = getWithOffset(reinterpret_cast<uintptr_t>(FileHeader),
+                                      Sec.FileOffsetToRelocationInfo);
----------------
sfertile wrote:
> hubert.reinterpretcast wrote:
> > getWithOffset does not do any bounds checking. Please ensure bounds checking.
> report_fatal_error() for 64-bit object files.
I will do as suggestion


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


================
Comment at: llvm/test/tools/llvm-readobj/xcoff-basic.test:128
+
+# RELOCSEXP:           Virtual Address: 0x110
+# RELOCSEXP-NEXT:      Symbol: J (96)
----------------
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.


================
Comment at: llvm/test/tools/llvm-readobj/xcoff-basic.test:92
+# RELOCSEXP-NEXT: Relocations [
+# RELOCSEXP-NEXT:  Section (1) .text {
+# RELOCSEXP-NEXT:    Relocation {
----------------
hubert.reinterpretcast wrote:
> I would prefer:
> ```
> Section (index: 1) .text {
> ```
> 
added


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:140
+    if (Relocations.empty())
+      continue;
+
----------------
sfertile wrote:
> Your section index will get out of sync, since we continue (to the next section) but don't increment the index.
index has been increased before W.unindent();


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67008