[libunwind][PowerPC] Fix saving/restoring VSX registers on LE systems

[nemanjai]

Summary

Currently, libunwind just uses stxvd2x/lxvd2x to save/restore VSX registers respectively. This puts the registers in doubleword-reversed order into memory on little endian systems. If both the save and restore are done the same way, this isn't a problem. However if the unwinder is just restoring a callee-saved register, it will restore it in the wrong order (since function prologues save them in the correct order).

This patch fixes the order by:

• Adding a check for ISA 3.0 if the GLIBC used at build time supports such a check
• On machines that support ISA 3.0, it uses non-swapping DQ-Form instructions (stxv/lxv)
• On machines that don't support ISA 3.0 (or on builds with GLIBC that is too old) swaps are added after the loads and before the stores

[xingxue]

The compiler generated prologue/epilogue uses instructions stvx/lvx to save/restore vector registers onto/from the stack. On the other hand, libunwind currently uses stxvd2x/lxvd2x to save/restore VSX vector to/from unwinder's context. Since stxvd2x/lxvd2x stores/loads VSX vector as 2 double words into/from memory, these instructions essentially change the byte orders of the contents if the machine is in little-endian mode. As a result, VSX vectors saved by prologues are not correctly represented in the unwinder context. PowerPC ISA 3.0 (Power9) has added instructions stxv/lxv that stores/loads VSX vectors and matches the behavior of stvx/lvx. Unfortunately, since supporting of older PowerPCs such as Power8 and earlier are still needed, we cannot replace stxvd2x/lxvd2x with stxv/lxv yet. Checking if the build system has the GLIBC facility for detecting the CPU architecture, and detecting at run time and going a different path add a lot of complicity to the code base. IMHO, we can simply add instruction xxswapd to before stxvd2x and after lxvd2x for little-endian like in your patch as a workaround for now. xxswapd is not an expensive instruction so it is unlikely to affect the performance in a significant way, noting that saving and restoring context are only executed when raising an exception and jumping to the landing pad. It will be good though to have a comment stating that we should replace them with stxv/lxv once only Power9 and higher are supported for little-endian PowerPC (RHEL 9.0 already supports minimum Power9). Thanks very much for working on this issue!

We probably can adapt test case libcxxabi/test/vendor/ibm/vec_reg_restore.pass.cpp to cover little-endian Linux on Power as well.

[MaskRay]

I created D139368 to simplify the repetitions with .irp.

libunwind/src/assembly.h

194 ↗

(On Diff #473822)

This doesn't work as intended.

.quad __parse_hwcap_and_convert_at_platform without a definition creates an undefined symbol.
There it is un-referenced there won't be a undefined symbol/reference error.

[nemanjai]

libunwind/src/assembly.h

194 ↗

(On Diff #473822)

I don't follow what you mean here. Under which conditions will this fail to produce an undefined symbol/reference error?

Perhaps it is a moot point since I will be removing this code as per Xing's comment, but I am just curious (especially since GCC also does this for emitting code for __builtin_cpu_is/__builtin_cpu_supports and Clang will follow GCC's implementation).

[nemanjai]

Reduce complexity of the code at the expense of performance on Power9 and newer subtargets. The updated code uses {st|l}xvd2x for saving/restoring with the necessary swaps regardless of the subtarget.
Added a test case to libcxxabi.

[xingxue]

LGTM; thanks!

[nemanjai]

Could I get someone from libunwind and libc++abi groups to review this?

[nemanjai]

Ping.

[nemanjai]

As of tomorrow, this patch has been sitting in approved state for 1 month. I plan to commit it at this point without approval from anyone in the libc++-abi and libunwind groups.
This bug is blocking a real-world user of the code and I need to commit it and back-port it to the LLVM 16 branch.

[mstorsjo]

The change seems reasonable to me - thus LGTM.

The patch description talks about checks for ISA 3.0, but I don't see any references to that in the patch itself. If this isn't relevant, remember to update the message before pushing.

[MaskRay]

libcxxabi/test/vendor/ibm/vec_reg_restore-le.pass.cpp

42

add blank line

62

blank line after a macro definition

libcxxabi/test/vendor/ibm/vec_reg_restore-le.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// Check that the PowerPC vector registers are restored properly during
+// unwinding.
+
+// REQUIRES: target=powerpc{{(64)?}}le-unknown-linux-gnu
+// UNSUPPORTED: no-exceptions
+
+// Callee-saved VSR's 62 and 63 (vr30, vr31 respectively) are set to 16 bytes
+// with values 1, 2 respectively in main. In order to ensure the two doublewords
+// in each register are different, they are merged. Then they are reset to 16
+// bytes with values 9 and 12 respectively in a callee and an exception is
+// thrown. When catching an exception in main, the values in the two registers
+// need to be the original ones (including the correct doubleword order).
+
+#include <cassert>
+#include <cstdlib>
+
+int __attribute__((noinline)) test2(int i) {
+  if (i > 3)
+    throw i;
+  srand(i);
+  return rand();
+}
+
+int __attribute__((noinline)) test(int i) {
+  // Clobber VS63 and VS62 in the function body.
+  // Set VS63 to 16 bytes each with value 9
+  asm volatile("vspltisb 31, 9" : : : "v31");
+
+  // Set VS62 to 16 bytes each with value 12
+  asm volatile("vspltisb 30, 12" : : : "v30");
+  return test2(i);
+}
+
+#define cmpVS63(vec, result)                                                   \
+  {                                                                            \
+    vector unsigned char gbg;                                                  \
+    asm volatile("vcmpequb. %[gbg], 31, %[veca];"                              \
+                 "mfocrf %[res], 2;"                                           \
+                 "rlwinm %[res], %[res], 25, 31, 31"                           \
+                 : [res] "=r"(result), [gbg] "=v"(gbg)                         \
+                 : [veca] "v"(vec)                                             \
+                 : "cr6");                                                     \
+  }
+
+#define cmpVS62(vec, result)                                                   \
+  {                                                                            \
+    vector unsigned char gbg;                                                  \
+    asm volatile("vcmpequb. %[gbg], 30, %[veca];"                              \
+                 "mfocrf %[res], 2;"                                           \
+                 "rlwinm %[res], %[res], 25, 31, 31"                           \
+                 : [res] "=r"(result), [gbg] "=v"(gbg)                         \
+                 : [veca] "v"(vec)                                             \
+                 : "cr6");                                                     \
+  }
+
+int main(int, char **) {
+  // Set VS63 to 16 bytes each with value 1.
+  asm volatile("vspltisb 31, 1" : : : "v31");
+
+  // Set VS62 to 16 bytes each with value 2.
+  asm volatile("vspltisb 30, 2" : : : "v30");
+
+  // Mix doublewords for both VS62 and VS63.
+  asm volatile("xxmrghd 63, 63, 62");
+  asm volatile("xxmrghd 62, 63, 62");
+
+  vector unsigned long long expectedVS63Value = {0x202020202020202,
+                                                 0x101010101010101};
+  vector unsigned long long expectedVS62Value = {0x202020202020202,
+                                                 0x101010101010101};
+  try {
+    test(4);
+  } catch (int num) {
+    // If the unwinder restores VS63 and VS62 correctly, they should contain
+    // 0x01's and 0x02's respectively instead of 0x09's and 0x12's.
+    bool isEqualVS63, isEqualVS62;
+    cmpVS63(expectedVS63Value, isEqualVS63);
+    cmpVS62(expectedVS62Value, isEqualVS62);
+    assert(isEqualVS63 && isEqualVS62);
+  }
+  return 0;
+}

libunwind/src/UnwindRegistersRestore.S

#if defined(__VSX__)
 
   // restore VS registers
   // (note that this also restores floating point registers and V registers,
   // because part of VS is mapped to these registers)
 
   addi  4, 3, PPC64_OFFS_FP
 
 // load VS register
+#ifdef __LITTLE_ENDIAN__
+// For little-endian targets, we need a swap since lxvd2x will load the register
+// in the incorrect doubleword order.
+// FIXME: when supporting targets older than Power9 on LE is no longer required,
+//        this can be changed to simply `lxv n, (16 * n)(4)`.
 #define PPC64_LVS(n)         \
   lxvd2x  n, 0, 4           ;\
+  xxswapd n, n              ;\
   addi    4, 4, 16
+#else
+#define PPC64_LVS(n)         \
+  lxvd2x  n, 0, 4           ;\
+  addi    4, 4, 16
+#endif
 
   // restore the first 32 VS regs (and also all floating point regs)
   PPC64_LVS(0)
   PPC64_LVS(1)
   PPC64_LVS(2)
   PPC64_LVS(3)
   PPC64_LVS(4)
   PPC64_LVS(5)
#endif
   PPC64_LVS(25)
   PPC64_LVS(26)
   PPC64_LVS(27)
   PPC64_LVS(28)
   PPC64_LVS(29)
   PPC64_LVS(30)
   PPC64_LVS(31)
 
+#ifdef __LITTLE_ENDIAN__
+#define PPC64_CLVS_RESTORE(n)                    \
+  addi   4, 3, PPC64_OFFS_FP + n * 16           ;\
+  lxvd2x n, 0, 4                                ;\
+  xxswapd n, n
+#else
 #define PPC64_CLVS_RESTORE(n)                    \
   addi   4, 3, PPC64_OFFS_FP + n * 16           ;\
   lxvd2x n, 0, 4
+#endif
 
 #if !defined(_AIX)
   // use VRSAVE to conditionally restore the remaining VS regs, that are
   // where the V regs are mapped. In the AIX ABI, VRSAVE is not used.
   ld    5, PPC64_OFFS_VRSAVE(3)   // test VRsave
   cmpwi 5, 0
   beq   Lnovec
 

libunwind/src/UnwindRegistersSave.S

 #if defined(__VSX__)
   // save VS registers
   // (note that this also saves floating point registers and V registers,
   // because part of VS is mapped to these registers)
 
   addi  4, 3, PPC64_OFFS_FP
 
 // store VS register
+#ifdef __LITTLE_ENDIAN__
+// For little-endian targets, we need a swap since stxvd2x will store the
+// register in the incorrect doubleword order.
+// FIXME: when supporting targets older than Power9 on LE is no longer required
+//        this can be changed to simply `stxv n, 16 * n(4)`.
 #define PPC64_STVS(n)      \
+  xxswapd n, n            ;\
   stxvd2x n, 0, 4         ;\
   addi    4, 4, 16
+#else
+#define PPC64_STVS(n)      \
+  stxvd2x n, 0, 4         ;\
+  addi    4, 4, 16
+#endif
 
   PPC64_STVS(0)
   PPC64_STVS(1)
   PPC64_STVS(2)
   PPC64_STVS(3)
   PPC64_STVS(4)
   PPC64_STVS(5)
   PPC64_STVS(6)