[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:419-421
+  ///    If XCOFF Traceback Table is not parsed successfully or there are extra
+  ///    bytes that are not recognized, \a Size will be updated to be the size
+  ///    up to the end of the last successfully parsed field of the table.
----------------
Minor nit: Add "the" before XCOFF.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:904
+  if (Cur && ParmNum > 0 && !hasVectorInfo())
+    ParmsType = parseParmsType(DE.getU32(Cur), ParmNum);
+
----------------
Even with `hasVectorInfo`, the field may be present. Should that happen, the field still needs to be read (even if not interpreted). I assume we are missing a test case where `hasVectorInfo` is true.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:919
+        Disp.push_back(DE.getU32(Cur));
+      ControlledStorageInfoDisp = std::move(Disp);
+    }
----------------
We should not update the field unless if `Cur` is still valid.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:997
+
+uint8_t XCOFFTracebackTable::getOnConditionDirective() const {
+  return GETBITWITHMASKSHIFT(0, OnConditionDirectiveMask,
----------------
Please rename the "get" functions to be "read" functions as indicated by previous comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:887
+  uint64_t Offset = 0;
+  unsigned ParmNum = 0;
+
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > Please reduce the scope of `ParmNum` by merging the adjacent blocks that set and use it.
> thanks
My request was to have the scope of `ParmNum` reduced. That is, please move the declaration of `ParmNum` to where it is set and used (and also arrange for it to be in a smaller block scope that does not extend much further than the point where `ParmNum` is used).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585