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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:160
+                ///< of the referenced symbol.
+  R_REL = 0x02, ///< Relative to self relocation. Provides a dispacement value
+                ///< between the address of the referenced symbol and the
----------------
s/dispacement/displacement/;


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:173
+            ///< relocation. Compilers are not permitted to generate this
+            ///< relocation type. it is the result of a reversible
+            ///< transformation by the linker of an R_TOC relation that turned a
----------------
s/it/It/;


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:185
+                ///< from garbage collecting a csect for which another csect has
+                ///< an implicit dependency.
+
----------------
Add "(such as code used for dynamic initialization of non-local statics)" before "for which".


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:188
+  R_BA = 0x08, ///< Branch absolute relocation. Provides the address of the
+               ///< referenced symbol. References a non modifiable instruction.
+  R_BR = 0x0a, ///< Branch relative to self relocation. Provides the
----------------
s/non modifiable/non-modifiable/;


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:191
+               ///< displacement that is the difference between the address of
+               ///< the refrenced symbol and the address of the refrenced branch
+               ///< instruction.  References a non modifiable instruction.
----------------
s/refrenced/referenced/g;


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:192
+               ///< the refrenced symbol and the address of the refrenced branch
+               ///< instruction.  References a non modifiable instruction.
+  R_RBA = 0x18, ///< Branch absolute relocation. Similar to R_BA but refrences a
----------------
`s/  / /;`
`s/non modifiable/non-modifiable/;`


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:193
+               ///< instruction.  References a non modifiable instruction.
+  R_RBA = 0x18, ///< Branch absolute relocation. Similar to R_BA but refrences a
+                ///< modifiable instruction.
----------------
s/refrences/references/;


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:195
+                ///< modifiable instruction.
+  R_RBR = 0x1a, ///< Branch relative to self relocation. Simialr to R_BR
+                ///< relocation type, but references a modifiable instruction.
----------------
s/Simialr/Similar/;
s/R_BR/the R_BR/;


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:158
+// Masks for packing/unpacking the r_rsize field of relocations.
+// The msb is used to indicate if the bits being relocated are signed or
+// unsigned.
----------------
Insert
```
//
```
between the group description and the specific description.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:161
+static constexpr uint8_t XR_SIGN_INDICATOR_MASK = 0x80;
+// The 2nd msb is used to indicate that the binder has replaced/modified the
+// original instruction.
----------------
Insert
```
//
```
before the next specific description to extend the group while maintaining spacing.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:164
+static constexpr uint8_t XR_FIXUP_INDICATOR_MASK = 0x40;
+// The remaining bits specify the bit length of the relocatable reference
+// minus one.
----------------
Same here.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:264
                              SmallVectorImpl<char> &Result) const override;
-
   section_iterator section_begin() const override;
   section_iterator section_end() const override;
----------------
Leave the blank line that was there before this patch.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:319
   void checkSymbolEntryPointer(uintptr_t SymbolEntPtr) const;
+  // Relocation-related interfaces.
+  uint32_t getLogicalNumberOfRelocationEntries(const XCOFFSectionHeader32 &Sec,
----------------
Insert a blank line above the comment.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:560
+// to the discussion of overflow headers in "Sections and Section Headers".
+uint32_t XCOFFObjectFile::getLogicalNumberOfRelocationEntries(
+    const XCOFFSectionHeader32 &Sec, uint16_t SectionIndex) const {
----------------
@sfertile, suggested that this be a separate patch. Could we land that first (with an update to how `STYP_OVRFLO` section headers are printed)?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67008