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

[Xing Xue via Phabricator via llvm-commits]

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


================
Comment at: libunwind/src/AddressSpace.hpp:599
 
-
 inline bool LocalAddressSpace::findOtherFDE(pint_t targetAddr, pint_t &fde) {
----------------
sfertile wrote:
> minor nit: whitespace change.
There is an extra empty line here and the change is from `clang-format`. I am leaning towards to keeping the change.


================
Comment at: libunwind/src/UnwindCursor.hpp:2020
+      // Must find the correct frame pointer register
+      if (TBTable->tb.name_present) {
+        const uint16_t name_len = *((uint16_t *)p);
----------------
sfertile wrote:
> minot nit: I think its cleaner to pull this out of the if block and increment past the name whether we need to or not.
Changed as suggested, thanks!


================
Comment at: libunwind/src/UnwindCursor.hpp:2061
+          if (stateTablePersonality == NULL) {
+            _LIBUNWIND_TRACE_UNWINDING("dlsym() failed with errno=%d\n", errno);
+          }
----------------
sfertile wrote:
> Should failing to load the library, or failing to resolve `__xlcxx_personality_v0` be a fatal failure?
The dynamic resolving of symbol `__xlcxx_personality_v0` is used for stack frames from code generated by the legacy compiler. According to the compiler packaging, they should not fail. In a unlikely event they do fail, the unwinder will ignore the legacy frames and it is still able to process `CLANG` generated frames. The debugging trace has the info if it happened.  Added asserts to help debug.


================
Comment at: libunwind/src/UnwindCursor.hpp:2105
+      assert(*(uint32_t *)ehInfo == 0 &&
+             "XLC++ EH: ehInfo version other than 0 is not supported");
+
----------------
sfertile wrote:
> This is the EH implementation for both new XLC++ and clang so we should drop the `XLC++` from the assert message.
Good point! Replaced `XLC++` with `libunwind`.


================
Comment at: libunwind/src/UnwindCursor.hpp:2349
+    // of a function on AIX.
+    pc -= 4;
+#else
----------------
sfertile wrote:
> Do we need this on AIX? I think we have 2 cases that could happen when we throw as the last instruction in a function and I don't think we need to backtrack the instruction pointer in either. The first case is a normal call to __cxa_throw, the call instruction will be followed by a toc-restore which is what the program counter will be pointing at. The second case is a call to _cxa_throw with no toc-restore, which might come up if we _cxa_throw is statically linked in and we use LTO or the call is written in asm, but in this case the program counter will be pointing to the word after the call instruction which is the zero word we are looking for (so no need to backtrack).
`PC` is needed for checking if the location is within a range of addresses. 


================
Comment at: libunwind/src/Unwind_AIXExtras.cpp:31
+  // Get to the name of the function.
+  if (TBTable->tb.name_present) {
+    p = (uint32_t *)&TBTable->tb_ext;
----------------
sfertile wrote:
> Switch the order here and early return if the name is not present.
> 
> ie 
> ```
> if (!TBTable->tb.name_present)
>   return NULL
> 
> // Get the name of the function.
> ...
> ```
Changed as suggested, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100132