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

[Chris Bowler via Phabricator via libcxx-commits]

cebowleratibm added a comment.

I haven't completed my review.  In particular I need to look at the longtbtable, has_vec mix up with more scrutiny but I wanted to submit my comments thus far.



================
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);
----------------
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
```


================
Comment at: libunwind/src/Registers.hpp:611
   void      setIP(uint32_t value) { _registers.__srr0 = value; }
+  uint64_t getCR() const { return _registers.__cr; }
+  void setCR(uint32_t value) { _registers.__cr = value; }
----------------
formatting is not consistent with surrounding code.


================
Comment at: libunwind/src/Registers.hpp:1179
   void      setIP(uint64_t value) { _registers.__srr0 = value; }
+  uint64_t getCR() const { return _registers.__cr; }
+  void setCR(uint64_t value) { _registers.__cr = value; }
----------------
formatting is not consistent with surrounding code.


================
Comment at: libunwind/src/UnwindCursor.hpp:1947
+
+enum frameType : unw_word_t { oldFrame = 0, clangFrame = 1 };
+
----------------
I think "oldFrame" could be more descriptive.  The term is meant to encompass xlC and xlclang++ compiled EH aware functions.

Suggest "legacyXLFrame" or "classicXLFrame".  I think a comment in libunwind wrt limitations of interaction with such frames is warranted, because those frames contain calls to the legacy libC system unwinder, which has potential to foul up state with libunwind.


================
Comment at: libunwind/src/UnwindCursor.hpp:1955
+__attribute__((__weak__)) _Unwind_Reason_Code
+__xlcxx_personality_v0(int, _Unwind_Action, uint64_t, _Unwind_Exception *,
+                       struct _Unwind_Context *);
----------------
Don't you want to use the __xlcxx_personality_v0_t typedef you just defined to declare __xlcxx_personality_v0?


================
Comment at: libunwind/src/UnwindCursor.hpp:2017
+    // p points to the offset of the state table into the stack.
+    pint_t stateOffset = *p++;
+
----------------
suggestion to your discretion: there are so many offsets in this code I would be inclined to name stateOffset => stateFrameOffset


================
Comment at: libunwind/src/UnwindCursor.hpp:2022
+    // Skip fields name_len and name if exist.
+    if (TBTable->tb.name_present) {
+      const uint16_t name_len = *((uint16_t *)p);
----------------
Curiously the IBM legacy XL code only scans past the name if the traceback table uses_alloca flag is set.

What you've written makes intuitive sense but I'd like to confirm the semantic here is intended.


================
Comment at: libunwind/src/UnwindCursor.hpp:2044
+      if (xlcPersonalityV0 == NULL) {
+        // If libc++abi is statically linked in, symbol __xlcxx_personality_v0
+        // has been resolved at the link time.
----------------
I think a commented is warranted here that you're trying to prevent libunwind from having a dependency on libc++abi.  If a legacy XLC++ frame is encountered, then the loading of libc++abi is warranted.


================
Comment at: libunwind/src/assembly.h:71
+#if defined(__powerpc64__) && (!defined(_CALL_ELF) || _CALL_ELF == 1) &&       \
+    !defined(_AIX)
 #define PPC64_OPD1 .section .opd,"aw", at progbits SEPARATOR
----------------
What is defining _CALL_ELF that it's necessary to mask this off with ~defined(_AIX) ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100132