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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 14 inline comments as done.
DiggerLin added a comment.

In D81585#2163516 <https://reviews.llvm.org/D81585#2163516>, @hubert.reinterpretcast wrote:

> @DiggerLin, for context (since I will be on vacation), I've gotten through the functional code changes, but have not reviewed the testing.


OK, thanks hubert.



================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:443
+
+  bool isBackChainStored() const;
+  uint8_t getNumOfFPRsSaved() const;
----------------
hubert.reinterpretcast wrote:
> There's no query corresponding to `IsFixupMask`?
yes, it is a reserved  bit. I am not sure is there anyone to require a  reserved bit.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:877
+    return std::move(Err);
+  return std::move(TBT);
+}
----------------
jhenderson wrote:
> Do you actually need the `std::move` here?
yes. I think so. there is iImplicitly-declared move constructor for XCOFFTracebackTable , and there is there are several Optional type of data member, for example, SmallString has move constructor. 


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:887
+  uint64_t Offset = 0;
+  unsigned ParmNum = 0;
+
----------------
hubert.reinterpretcast wrote:
> Please reduce the scope of `ParmNum` by merging the adjacent blocks that set and use it.
thanks


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:911
+    if (!Err && NumOfCtlAnchors) {
+      SmallVector<uint32_t, 8> Disp;
+      for (uint32_t I = 0; I < NumOfCtlAnchors && !Err; I++)
----------------
hubert.reinterpretcast wrote:
> This should reserve the number of entries beforehand (since that number is known).
thanks


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:914
+        Disp.push_back(DE.getU32(&Offset, &Err));
+      ControlledStorageInfoDisp = Disp;
+    }
----------------
hubert.reinterpretcast wrote:
> Please use `std::move`.
thanks.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:974
+
+bool XCOFFTracebackTable::IsFloatingPointOperationLogOrAbortEnabled() const {
+  return GETBITWITHMASK(0, IsFloatingPointOperationLogOrAbortEnabledMask);
----------------
hubert.reinterpretcast wrote:
> The capitalization of this function name does not match that of the other functions.
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585