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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast requested changes to this revision.
hubert.reinterpretcast added inline comments.
This revision now requires changes to proceed.


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


================
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;
----------------
s/instuction/instruction/;


================
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.
----------------
Add a blank line before the comment.


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


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:499
+
+  if (Index >= getLogicalNumberOfSymbolTableEntries32())
+    return errorCodeToError(object_error::invalid_symbol_index);
----------------
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.


================
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);
----------------
getWithOffset does not do any bounds checking. Please ensure bounds checking.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:660
+  return ArrayRef<XCOFFRelocation32>(StartReloc,
+                                     StartReloc + Sec.NumberOfRelocations);
+}
----------------
This should use the logical number of relocations, not the raw field value.


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



================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:119
+
+static const EnumEntry<XCOFF::RelocationType> ReloationTypeNameclass[] = {
+#define ECase(X)                                                               \
----------------
s/Reloation/Relocation/;


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:122
+  { #X, XCOFF::X }
+    ECase(R_POS),    ECase(R_NEG),    ECase(R_REL),  ECase(R_TOC),
+    ECase(R_TRL),    ECase(R_TRLA),   ECase(R_GL),   ECase(R_TCL),
----------------
The number of `ECase`s does not match the number of enumerators in the enumeration.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67008