[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:396
 
+class XCOFFTracebackTable {
+  const uint8_t *TBPtr;
----------------
Add Doxygen-style comment for the class to explain that this class provides methods to extract traceback table data from a buffer and that the various accessors may continue to reference the buffer provided via the constructor.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:397
+class XCOFFTracebackTable {
+  const uint8_t *TBPtr;
+  Optional<SmallString<32>> ParmsType;
----------------
I don't think this should be modified following initialization in the constructor.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:892-905
+  unsigned ParmNum = 0;
+  if (Cur)
+    ParmNum = getNumberOfFixedParms() + getNumberOfFPParms();
+
+  // As long as there are no "fixed-point" or floating-point parameters, this
+  // field remains not present even when hasVectorInfo gives true and indicates
+  // the presence of vector parameters.
----------------
The scope of `ParmNum` can be further reduced, which I want because `ParmNum` does not encompass the number of vector parameters. Additionally, "Params" is not consistent with the use of "Parms" in most places of this patch.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:942
+#define GETBITWITHMASK(P, X)                                                   \
+  (support::endian::read32be(TBPtr + P) & TracebackTable::X)
+#define GETBITWITHMASKSHIFT(P, X, S)                                           \
----------------
Use defensive parentheses for references to macro parameters.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:944-945
+#define GETBITWITHMASKSHIFT(P, X, S)                                           \
+  (support::endian::read32be(TBPtr + P) & TracebackTable::X) >>                \
+      TracebackTable::S
+
----------------
Use defensive parentheses for references to macro parameters and for the definition of the macro.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:997
+
+uint8_t XCOFFTracebackTable::getOnConditionDirective() const {
+  return GETBITWITHMASKSHIFT(0, OnConditionDirectiveMask,
----------------
hubert.reinterpretcast wrote:
> Please rename the "get" functions to be "read" functions as indicated by previous comments.
Sorry for the churn. I think we can go with just naming these "get" and documenting the class better, especially considering the awkwardness of renaming the "is", etc. functions as well (since those functions also read from the buffer).

Please update for the other "read" functions as well (so we don't leave a mix of "read" and "get" functions that are different in naming but not really different in nature).


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:81
+
+TEST(XCOFFObjectFileTest, XCOFFTracebackTableAPIParamsType) {
+  uint8_t V[] = {0x01, 0x02, 0xA2, 0x40, 0x80, 0x00, 0x02, 0x07, 0x2B, 0x00,
----------------
s/Params/Parms;


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:171
+
+TEST(XCOFFObjectFileTest, XCOFFTracebackTableTruncatedAtParamsType) {
+  uint64_t Size = 9;
----------------
s/Params/Parms;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585