[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:403
+  Optional<SmallVector<uint32_t, 8>> ControlledStorageInfoDisp;
+  Optional<uint16_t> FunctionNameLen;
+  Optional<StringRef> FunctionName;
----------------
Why a separate field for the length of the function name? The `StringRef` for the function name has a length field.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:416
+  /// \param[in] Ptr
+  ///    A pointer points to the beginning of XCOFF Traceback Table (right after
+  ///    the initial 4 bytes of zeros).
----------------
s/pointer points/pointer that points/;
s/of XCOFF/of an XCOFF/;


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:419
+  /// \param[in, out] Size
+  ///    A pointer points to the length of the XCOFF Traceback Table.
+  ///    If XCOFF Traceback Table is not parsed successfully or there are extra
----------------
s/pointer points/pointer that points/;


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:422
+  ///    bytes that are not recognized, \a Size will be updated to reflect the
+  ///    actual length of the XCOFF Traceback Table that get parsed.
+  static Expected<XCOFFTracebackTable> create(const uint8_t *Ptr,
----------------
s/get/got/;

Is the value of `Size` modified or not when `create` reports an error? If modified, does `Size` represent the last successfully parsed field?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:434
+  bool hasTOC() const;
+  bool isFloatPointPresent() const;
+  bool isLogAbort() const;
----------------
Note: More instances of "FloatPoint" here.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:435
+  bool isFloatPointPresent() const;
+  bool isLogAbort() const;
+
----------------
The function names have fallen out of sync with the mask naming.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:440
+  bool isAllocaUsed() const;
+  uint8_t getClDisInv() const;
+  bool isCRSaved() const;
----------------
This name is very different from the name we are using for the associated mask. Please go through the names and resync.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:451
+
+  uint8_t getNumberOfFixedPara() const;
+
----------------
Same comments here as for the mask names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585