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

[Xing Xue via Phabricator via libcxx-commits]

xingxue marked 5 inline comments as done.
xingxue added inline comments.


================
Comment at: libunwind/src/AddressSpace.hpp:48
 
+#if defined(_AIX)
+namespace libunwind {
----------------
cebowleratibm wrote:
> 
It is guarded by `defined(_AIX)` instead of `defined(_LIBUNWIND_SUPPORT_TBTAB_UNWIND)` because `getFuncNameFromTBTable()` would also be used for GCC on AIX but GCC does not use the traceback table for unwinding.


================
Comment at: libunwind/src/AddressSpace.hpp:630
   }
+#elif defined(_AIX)
+  uint16_t nameLen;
----------------
cebowleratibm wrote:
> 
It is guarded by `defined(_AIX)` instead of `defined(_LIBUNWIND_SUPPORT_TBTAB_UNWIND)` because `findFunctionName()` would also be used for GCC on AIX but GCC does not use the traceback table for unwinding.


================
Comment at: libunwind/src/UnwindCursor.hpp:2088
+    // is not EH aware; or, 3) a frame of other languages. We need to figure out
+    // if the traceback table extension containds the 'eh_info' structure.
+    //
----------------
cebowleratibm wrote:
> 
Good catch! Fixed.


================
Comment at: libunwind/src/UnwindCursor.hpp:2097
+    // In </usr/include/sys/debug.h>, there is the following definition of
+    // 'struct tbtable_ext'. It is not really a struction but just a dummy to
+    // collect the description of the optional parts of the traceback table.
----------------
cebowleratibm wrote:
> 
Good catch! Fixed.


================
Comment at: libunwind/src/Unwind_AIXExtras.cpp:20
+// Get the function name from its traceback table.
+char *getFuncNameFromTBTable(uintptr_t Pc, uint16_t &NameLen,
+                             unw_word_t *Offset) {
----------------
cebowleratibm wrote:
> I suggested guarding this function with _LIBUNWIND_SUPPORT_TBTAB_UNWIND so perhaps an asserting stub is appropriate on non-AIX targets.
This source file `Unwind_AIXExtras.cpp` won't be included for non-AIX targets as specified in file `CMakeLists.txt`.  Also, the code inside the file is guarded by `#if defined(_AIX)` in line 11.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100132