[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:298
 
+struct TracebackTable {
+  // Byte 1
----------------
I would suggest some renames if we decide not to add any doxygen comment for the masks, because some names here are not self-explanatory.
Here's some suggestion as examples:
GlobaLinkageMask -> GlobalLinkageMask
IsEprolMask -> IsOutOfLineEpilogOrPrologueMask
HasCodeLenMask -> HasTraceBackTableOffsetMask
...

Hope you could get the idea.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:310
+  static constexpr uint32_t IsEprolMask = 0x0000'4000;
+  static constexpr uint32_t HasCodeLenMask = 0x0000'2000;
+  static constexpr uint32_t IntProcMask = 0x0000'1000;
----------------
In AIX OS header "/usr/include/sys/debug.h", we have this field as 
```
unsigned has_tboff:1; /* Set if offset from start of proc stored */
```
It might be better to rename HasCodeLenMask sot that it has a bit association with the original name? So that people do not need to reason about if this is the correct field or not.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:334
+  // Byte 6
+  static constexpr uint32_t HasVecInfoMask = 0x0080'0000;
+  static constexpr uint32_t Spare4Mask = 0x0040'0000;
----------------
I find the 6th byte here is a bit different than what we have in the OS headers:
```
    /* Byte 6 */
  unsigned longtbtable:1; /* Set if xtbtable extension exists. */
  unsigned has_vec:1;   /* Set if optional vector info is present */
  unsigned gpr_saved:6;   /* Number of GPRs saved, max of 32 */
```


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:348
+
+  static constexpr uint32_t FixedParaTypeBit = 0x8000'0000;
+  static constexpr uint32_t FloatPointParaTypeBit = 0x4000'0000;
----------------
TODO: add a comment to say this is for other purpose?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:399
+  Optional<uint32_t> CodeLen;
+  Optional<uint32_t> HandMask;
+  Optional<uint32_t> CtlInfo;
----------------
HandMask -> HandlerMask



================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:400
+  Optional<uint32_t> HandMask;
+  Optional<uint32_t> CtlInfo;
+  Optional<uint16_t> FunctionNameLen;
----------------
TODO: no number of CTL anchors? or Displacement into stack of each anchor?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:406
+public:
+  static Expected<XCOFFTracebackTable> create(const uint8_t *Ptr, uint64_t S);
+  XCOFFTracebackTable(const uint8_t *Ptr, uint64_t S, Error &Err);
----------------
Rename parameter `S` as `Size`. `S` itself doesn't tell people what this parameter is meant to be without people looking at the function body for a while. 


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:428
+
+  bool doesBackChainStore();
+  uint8_t getNumOfFPRsSaved();
----------------
doesBackChainStore -> isBackChainStore


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:842
+bool doesXCOFFTracebackTableBegin(ArrayRef<uint8_t> Bytes) {
+  assert(Bytes.size() == 4 && "Traceback table started with 4 bytes zero.");
+  return support::endian::read32be(Bytes.data()) == 0;
----------------
How does callers know that they have to pass an ArrayRef that has exactly 4 bytes to this function?
What if they have an empty array? What if they pass in a 8 byte array?
I don't think it's callers' responsibility to ensure that. This function should still return a correct answer for the above situation.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:847
+static std::string parseParaType(uint32_t Value, unsigned int ParaNum) {
+  std::string ParaType;
+  for (unsigned I = 0; I < ParaNum; ++I) {
----------------
I think we could use a SmallString here when editing the string and return std::string in the end for better performance (especially when you know the ParaNum which have some implication on how large the string could be).



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:850
+    if (I != 0)
+      ParaType += ", ";
+    if ((Value & TracebackTable::FixedParaTypeBit) == 0) {
----------------
Consider doing ParaType += "i, " and ParaType += "f, " ...
and do a removal of ", " after parsing all parameters.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:891
+
+  if (!Err && ParaNum && !hasVecInfo())
+    ParaType = parseParaType(DE.getU32(&offset_ptr, &Err), ParaNum);
----------------
When (ParaNum && hasVecInfo()) returns true, and we do not parse this, would we have the wrong offset_ptr for the rest of the traceback table?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:902
+    CtlInfo = DE.getU32(&offset_ptr, &Err);
+
+  if (!Err && isFuncNamePresent()) {
----------------
Are we missing something between CtlInfo and FunctionNameLen?

```
 * ctl_info      exists if has_ctl bit is set.
 * ctl_info_disp exists if ctl_info exists.
 * name_len      exists if name_present bit is set.
```
i.e. the ctl_info_disp?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:912
+    AllocaRegister = DE.getU8(&offset_ptr, &Err);
+}
+
----------------
It seems this is missing the parsing of 
```
struct vec_ext vec_ext; /* Vector extension (if has_vec is set) */
unsigned char xtbtable; /* More tbtable fields, if longtbtable is set*/
```
Although it is Okay (or even preferable) that we do not handle these extra fields in this patch to keep the patch size reasonable, I think we should still return errors when these fields are present.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:916
+  (support::endian::read32be(TBPtr + P) & TracebackTable::X)
+#define GETBITWITHMASKSHIFT(P, X, S)                                           \
+  (support::endian::read32be(TBPtr + P) & TracebackTable::X) >>                \
----------------
Macros are missing undefs.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585