[libcxx-commits] [PATCH] D114445: [libc++abi][AIX] Add 2 LIT tests for the AIX unwinder

[Xing Xue via Phabricator via libcxx-commits]

xingxue added a comment.

Hi @compnerd, Thanks very for your comments! I've made changes accordingly.  Please let me know if you have further comments!



================
Comment at: libcxxabi/test/vendor/ibm/cond_reg_restore.pass.cpp:22
+  // This asm code makes the prologue/epilogue to save/restore the condition
+  // register to/from the stack. The unwinder restores from the stack too.
+  asm volatile("nop" : ::"cr2");
----------------
compnerd wrote:
> The comment is a bit difficult to parse for me.  AIUI, it means to state that the inline assembly forces the prologue/epilogue to save/restore the condition register.  That makes sense since the cr2 register is marked as a clobber.
Sorry about that.  Reworded as suggested, thanks!


================
Comment at: libcxxabi/test/vendor/ibm/cond_reg_restore.pass.cpp:23
+  // register to/from the stack. The unwinder restores from the stack too.
+  asm volatile("nop" : ::"cr2");
+  if (i > 3) {
----------------
compnerd wrote:
> Nit: could you use `:::` or `: : :` for the inline assembly parameters please.
Changed to `: : :`, thanks!


================
Comment at: libcxxabi/test/vendor/ibm/vec_reg_restore.pass.cpp:40
+               :  "v30", "r29");
+  return test2(i);
+}
----------------
compnerd wrote:
> Why registers 29, 30 and v62, v63?  A comment about that would be nice.  All four of those are non-volatile registers, which should be restored during the unwinding.
Sorry, will add comments to better describe the code. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114445