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

[Xing Xue via Phabricator via libcxx-commits]

xingxue added inline comments.


================
Comment at: libunwind/src/UnwindCursor.hpp:2080
+    if (TBTable->tb.name_present)
+      charPtr = charPtr + *((uint16_t *)charPtr) + sizeof(uint16_t);
+
----------------
cebowleratibm wrote:
> Why is the logic for skipping the name here not the same as above?  The handling of XL frames bumps the pointer by the name_len.
Changed to use the same logic to skip the `name_len` and `name` fields.


================
Comment at: libunwind/src/UnwindCursor.hpp:2088
+    if (TBTable->tb.has_vec)
+      // Note struct vec_ext does exist at this point because whether the
+      // ordering of longtbtable and has_vec bits is correct or not, both
----------------
cebowleratibm wrote:
> I don't think this comment is necessary because this branch is for non XL tbtables, which should not mix up the has_vec and longtbtable bits.
It is possible to enter this branch for a classic XL frame as well. The possible scenarios of entering this branch are: 1) `tb.lang == TB_CPLUSPLUS` but `TBTable->tb.has_ctl` is not set, which means it is either a classic XL frame from `xlc++`/`xlclang++` but not EH aware, or a Clang frame. 2) `tb.lang != TB_CPLUSPLUS`, then it is a frame of other languages, such as `C`, which can be generated by `xlc`, `xlclang`, or `clang`. 


================
Comment at: libunwind/src/UnwindCursor.hpp:2099
+    if ((*charPtr & xTBTableMask::ehInfoBit) &&
+        !(*charPtr & xTBTableMask::reservedBit)) {
+      // eh_info is available.
----------------
cebowleratibm wrote:
> I think it's unnecessary to check the reservedBit here because this code should only be handling clang frames at this point that don't mix up the vec_ext and longtbtable bits.
> 
> The details of the has_vec and longtbtable bit mix up by the XL compiler can then be pushed into the v0 XL personality routine where the complexity belongs.  I don't think the check or comments are necessary here.
> I think it's unnecessary to check the reservedBit here because this code should only be handling clang frames at this point that don't mix up the vec_ext and longtbtable bits.

This code needs to handle classic XL frames as well.  See the comment above.


================
Comment at: libunwind/src/UnwindCursor.hpp:2324
+                               (void *)newRegisters.getRegister(2));
+    if (firstInstruction == loadTOCRegInst) {
+      _LIBUNWIND_TRACE_UNWINDING("Set gpr2=%p from frame\n",
----------------
cebowleratibm wrote:
> Just for my understanding, is this the first instruction of the glink forwarding stub that the linker inserts for toc save/restore?
Yes, the compiler generates a 'nop' after a call and the linker replaces it with the instruction to load the TOC saved by the glink stub.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100132