[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/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:
> 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!


================
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; }
----------------
cebowleratibm wrote:
> formatting is not consistent with surrounding code.
This is from the `git-clang-format`.


================
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; }
----------------
cebowleratibm wrote:
> formatting is not consistent with surrounding code.
This is from `git-clang-format`.


================
Comment at: libunwind/src/UnwindCursor.hpp:1947
+
+enum frameType : unw_word_t { oldFrame = 0, clangFrame = 1 };
+
----------------
cebowleratibm wrote:
> 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.
Replaced `oldFrame` with `classicXLFrame` as suggested, thanks!

There should not be limitations of the interaction between the `classicXLFrame` and the `clangFrame` in libunwind. The `xlclang++` compiled EH using `oldFrame` does not call the legacy `libC` unwinder and is compatible with `clang` compiled EH when running against `libunwind`/`libc++abi`. I agree that the legacy `xlC` compiled EH uses the `libC` unwinder which is incompatible with libunwind but that is because of the difference in the design of unwinders not the stack frame.


================
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 *);
----------------
cebowleratibm wrote:
> Don't you want to use the __xlcxx_personality_v0_t typedef you just defined to declare __xlcxx_personality_v0?
Changed as suggested.  Thanks for pointing this out.


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


================
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);
----------------
cebowleratibm wrote:
> 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.
The semantic is the same as the legacy XL code: 1) if `uses_alloca` flag is set, use the content of the location after the name as the frame pointer register; 2) otherwise, use the default frame pointer register, i.e., SP.  Computing the location after the name is unnecessary in the case where `uses_alloca` is unset.  But Sean thought it is cleaner this way (see the comment below).


================
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.
----------------
cebowleratibm wrote:
> 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.
Added comments explaining why dynamically resolving.


================
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
----------------
cebowleratibm wrote:
> What is defining _CALL_ELF that it's necessary to mask this off with ~defined(_AIX) ?
`&& !defined(_AIX)` is added to mask off the code for `AIX`, no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100132