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

[Xing Xue via Phabricator via libcxx-commits]

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


================
Comment at: libunwind/src/assembly.h:63
 
 #if defined(__powerpc64__) && (!defined(_CALL_ELF) || _CALL_ELF == 1)
 #define PPC64_OPD1 .section .opd,"aw", at progbits SEPARATOR
----------------
sfertile wrote:
> Should we update the guards so we don't define these macros on AIX?
Changed as suggested, thanks!


================
Comment at: libunwind/src/assembly.h:197
+// clang-format off
+#define DEFINE_LIBUNWIND_FUNCTION_AND_WEAKALIAS(name, aliasname)               \
+  .toc SEPARATOR                                                               \
----------------
sfertile wrote:
> Minor nit: `_WEAK_ALIAS` to match the other weak alias macros.
Renamed the macro as suggested.


================
Comment at: libunwind/src/assembly.h:198
+#define DEFINE_LIBUNWIND_FUNCTION_AND_WEAKALIAS(name, aliasname)               \
+  .toc SEPARATOR                                                               \
+  .globl name SEPARATOR                                                        \
----------------
sfertile wrote:
> Same comment as below about this .toc pseudo op.
Good idea, thanks!


================
Comment at: libunwind/src/assembly.h:224
+#define DEFINE_LIBUNWIND_FUNCTION(name)                                        \
+  .toc SEPARATOR                                                               \
+  .globl name SEPARATOR                                                        \
----------------
sfertile wrote:
> Sorry, I seem to have deleted this comment before submitting my last set of comments. Its extra emabahrasing because one of the comments I left referred to this one :(.
> 
> The .toc pseudo is only needed once per file that includes this header, not really once per function being defined (although the extras are harmless). We have a change at the top of the 2 files that include assembly.h to not emit the `.text` directive, we can instead change that to emit the .toc directive on AIX and delete the .toc directive from this macro and `DEFINE_LIBUNWIND_FUNCTION_AND_WEAKALIAS`
No worries.  Thanks for the good suggestion!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100132