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

[Xing Xue via Phabricator via llvm-commits]

xingxue marked an inline comment as done.
xingxue 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);
----------------
cebowleratibm wrote:
> 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?  
Good question.  We can allow the declaration of `_Unwind_GetTextRelBase()` on AIX, and have the stub routine print "_Unwind_GetTextRelBase() not implemented" and abort like others.


================
Comment at: libunwind/src/AddressSpace.hpp:50
+namespace libunwind {
+char *getFuncName(uintptr_t pc, uint16_t &NameLen, unw_word_t *offset);
+}
----------------
cebowleratibm wrote:
> 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?
Renamed `getFuncName` to `getFuncNameFromTBTable`.


================
Comment at: libunwind/src/UnwindCursor.hpp:1949
+
+enum frameType : unw_word_t { classicXLFrame = 0, clangFrame = 1 };
+
----------------
cebowleratibm wrote:
> 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.
Renamed `clangFrame` to `frameWithEHInfo` and renamed `classicFrame` to `frameWithXLEHStateTable`.


================
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.
+
----------------
cebowleratibm wrote:
> 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.
Added comments as suggested.  Thanks!


================
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
----------------
cebowleratibm wrote:
> 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.
Added comments at the beginning of the branch to describe the erroneous xlC frame case. Updated the comment here accordingly.


================
Comment at: libunwind/src/UnwindCursor.hpp:2129
+      // Mark this frame has the new EH info.
+      flags = frameType::clangFrame;
+
----------------
cebowleratibm wrote:
> 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.
Moved the statement to the top of the block as suggested, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100132