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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:346
+  static constexpr uint32_t ParmsOnStackMask = 0x0000'0001;
+  static constexpr uint8_t NumberOfFPParaShift = 01;
+
----------------
01 -> 1?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:396
+  const uint8_t *TBPtr;
+  const uint64_t Size;
+  Optional<std::string> ParaType;
----------------
Do you actually need the Size as data member?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:872
+Expected<XCOFFTracebackTable> XCOFFTracebackTable::create(const uint8_t *Ptr,
+                                                          const uint64_t S) {
+  Error Err = Error::success();
----------------
Remove `const` to match with declaration.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:884
+  ErrorAsOutParameter EAO(&Err);
+  DataExtractor DE(ArrayRef<uint8_t>(Ptr, S), false, 4);
+  uint64_t offset_ptr = 0;
----------------
Suggestion:
```
DataExtractor DE(ArrayRef<uint8_t>(Ptr, S), /* IsLittleEndian */ false, AddressSize);
```
AddressSize is not necessarily 4, it's 8 in 64 bit mode (although we don't actually use member functions in DataExtractor which uses AddressSize, in that case not sure if we could just pass in `/* AddressSize  */ 0` to avoid confusion).


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:904
+  if (!Err && isFuncNamePresent()) {
+    uint16_t Len = DE.getU16(&offset_ptr, &Err);
+    if (!Err)
----------------
Why do we need to declare a new variable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585