[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:320
+  static constexpr uint32_t HasFunctionNamePresentMask = 0x0000'0040;
+  static constexpr uint32_t UsedAllocaMask = 0x0000'0020;
+  static constexpr uint32_t OnConditionDirectiveMask = 0x0000'001C;
----------------
For boolean mask, let's make them consistent as above name and use "Is" or "Has" to start which people could look at the name and know it's boolean. 
This also applies to the masks below.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:399
+  Optional<SmallString<32>> ParaType;
+  Optional<uint32_t> CodeLen;
+  Optional<uint32_t> HandlerMask;
----------------
Please match the name of the mask, the same for the getter function of it.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:426
+  bool hasFuncNamePresent() const;
+  bool usedAlloca() const;
+  uint8_t getClDisInv() const;
----------------
Please make sure the masks and getter function have matching names. With `is` or `has` if it returns bool. 


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:882
+
+XCOFFTracebackTable::XCOFFTracebackTable(const uint8_t *Ptr, uint64_t Size,
+                                         Error &Err)
----------------
We passed in a pointer and a size to this constructor function and create function, so caller knows how big the traceback table is. 
I think in general we need to have a way to tell caller some of the content is not parsed when that happens. 
i.e. everything in the traceback looks good, but it has some extra bytes in the end we do not recognized. 
So that caller maybe able to just dump the raw bytes out if they want, and print out a general message saying:" Some of the traceback table content is not recognized and here are the raw bytes."


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:922
+
+  // TODO: Need to parse vector Info and extension table if there is.
+}
----------------
I'm Okay with these TODOs if we are actually going to provide a patch for it right after we land this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585