[libcxx-commits] [PATCH] D100132: [libunwind][AIX] implementation of the unwinder for AIX

[Chris Bowler via Phabricator via libcxx-commits]

cebowleratibm added inline comments.


================
Comment at: libunwind/include/unwind.h:164
+// are unimplemented. AIX supports data-rel addressing.
+#if defined(_AIX)
+extern uintptr_t _Unwind_GetDataRelBase(struct _Unwind_Context *context);
----------------
xingxue wrote:
> cebowleratibm wrote:
> > This declaration of _Unwind_GetDataRelBase is the same as the existing one.  Isn't this identical to the cleaner:
> > 
> > 
> > ```
> > extern uintptr_t _Unwind_GetDataRelBase(struct _Unwind_Context *context)
> >     LIBUNWIND_UNAVAIL;
> > #if !defined(_AIX)
> > extern uintptr_t _Unwind_GetTextRelBase(struct _Unwind_Context *context)
> >     LIBUNWIND_UNAVAIL;
> > #endif
> > ```
> I agree, thanks!
What is the reason for declaring _Unwind_GetDataRelBase on all platforms but hiding _Unwind_GetTextRelBase on AIX?  


================
Comment at: libunwind/src/AddressSpace.hpp:50
+namespace libunwind {
+char *getFuncName(uintptr_t pc, uint16_t &NameLen, unw_word_t *offset);
+}
----------------
getFuncName doesn't sound AIX specific so it's odd to see it guarded by _AIX.  Perhaps a more specific name to describe why it's unique to AIX?


================
Comment at: libunwind/src/UnwindCursor.hpp:1949
+
+enum frameType : unw_word_t { classicXLFrame = 0, clangFrame = 1 };
+
----------------
clangFrame probably isn't an appropriate name either as we shouldn't imply that libunwind will only work with clang frames.  Although clang is the only current emitter of such frames, other compilers may follow suit.


================
Comment at: libunwind/src/UnwindCursor.hpp:2081
+  } else if (TBTable->tb.longtbtable) {
+    // If the longtbtable bit is set, the range table info may be available.
+
----------------
I suggest a comment to explicitly state that we're no handling either a "new" frame or a classic XL frame that is not eh-aware.  I think it also good to outline the mix up of the has_vec and longtbtable bits at this point.  The reader should understand that there's a case where the longtbtable is set but there isn't really a xtbtable and the code that follows deals with this complexity.


================
Comment at: libunwind/src/UnwindCursor.hpp:2103
+    // Also check if the reserved bit of the extended traceback table field
+    // xtbtable is set. If it is, the traceback table was incorrectly generated
+    // by an XL compiler that uses the wrong ordering of longtbtable and
----------------
I think it's useful to spell out the erroneous xlC frame case.  What situation exposes it and in that case what does reservedBit actually refer to?

Here's my understanding:
-xlC compiled non eh aware frame which has vector registers to restore but nothing that otherwise requires the longtbtable bit to be set (otherwise both bits are set and the mix up is moot.)  In this case, the current charPtr will actually be pointing into the vec_ext struct of the tbtable because we failed to scan past the vec_ext struct because the has_vec bit was erroneously false.

As LLVM devs don't have access to internal XL source/docs I feel it's necessary we go above and beyond to explain this clearly in the comments.  Even for me it took some time to reason through the purpose of the reservedBit check.


================
Comment at: libunwind/src/UnwindCursor.hpp:2129
+      // Mark this frame has the new EH info.
+      flags = frameType::clangFrame;
+
----------------
Suggest moving this to the top of the block because it helps the reader identify that we've deduced that frame must be a non classic XL frame.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100132