[PATCH] D81585: [AIX][XCOFF][Patch1] Provide decoding trace back table information API for xcoff object file for llvm-objdump -d

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:414
+  /// \param[in] Ptr
+  ///    A pointer that points to the beginning of an XCOFF Traceback Table
+  ///    (right after the initial 4 bytes of zeros).
----------------
Let's be consistent with the value of the offset field that the beginning of a traceback table is the initial 4 bytes of zeros:
A pointer that points just past the initial 4 bytes of zeros at the beginning of an XCOFF Traceback Table.



================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:421
+  ///    last successfully parsed field
+  ///  of the XCOFF Traceback Table.
+  static Expected<XCOFFTracebackTable> create(const uint8_t *Ptr,
----------------
Nit: It seems the previous line ended prematurely?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:888
+
+  DE.getU64(&Offset, &Err);
+
----------------
Please add a comment to indicate that we are skipping the required initial 8 bytes here.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:890
+
+  unsigned ParmNum = getNumberOfFixedParms() + getNumberOfFPParms();
+
----------------
I see no attempt here to verify that the read of the initial 8 bytes has not failed.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:894
+  // parsing parameter type for vector info.
+  if (!Err && ParmNum && !hasVectorInfo())
+    ParmsType = parseParmsType(DE.getU32(&Offset, &Err), ParmNum);
----------------
When checking values that are not semantically boolean values, I would suggest explicitly writing the comparison against zero.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:933
+
+uint8_t XCOFFTracebackTable::getVersion() const {
+  return GETBITWITHMASKSHIFT(0, VersionMask, VersionShift);
----------------
I am not sure that functions that perform a read via a pointer to non-owned memory should be named "get" functions as opposed to "read" functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585