Subject: Re: Failure to build librseq on ppc

From: Mathieu Desnoyers <[email-masked]>
Date: Wed, 8 Jul 2020 10:00:01 -0400 (EDT)

----- On Jul 8, 2020, at 8:33 AM, Mathieu Desnoyers [email-masked] wrote:

> ----- On Jul 7, 2020, at 8:59 PM, Segher Boessenkool [email-masked]
> wrote:
[...]
>> 
>> So perhaps you have code like
>> 
>>  int *p;
>>  int x;
>>  ...
>>  asm ("lwz %0,%1" : "=r"(x) : "m"(*p));
> 
> We indeed have explicit "lwz" and "stw" instructions in there.
> 
>> 
>> where that last line should actually read
>> 
>>  asm ("lwz%X1 %0,%1" : "=r"(x) : "m"(*p));
> 
> Indeed, turning those into "lwzx" and "stwx" seems to fix the issue.
> 
> There has been some level of extra CPP macro coating around those instructions
> to
> support both ppc32 and ppc64 with the same assembly. So adding %X[arg] is not
> trivial.
> Let me see what can be done here.

I did the following changes which appear to generate valid asm.
See attached corresponding .S output.

I grepped for uses of "m" asm operand in Linux powerpc code and noticed it's pretty much
always used with e.g. "lwz%U1%X1". I could find one blog post discussing that %U is about
update flag, and nothing about %X. Are those documented ?

Although it appears to generate valid asm, I have the feeling I'm relying on undocumented
features here. :-/

Here is the diff on https://git.kernel.org/pub/scm/libs/librseq/librseq.git/tree/include/rseq/rseq-ppc.h
It's only compile-tested on powerpc32 so far:

diff --git a/include/rseq/rseq-ppc.h b/include/rseq/rseq-ppc.h
index eb53953..f689fe9 100644
--- a/include/rseq/rseq-ppc.h
+++ b/include/rseq/rseq-ppc.h
@@ -47,9 +47,9 @@ do {                                                                  \

 #ifdef __PPC64__

-#define STORE_WORD     "std "
-#define LOAD_WORD      "ld "
-#define LOADX_WORD     "ldx "
+#define STORE_WORD(arg)        "std%U[" __rseq_str(arg) "]%X[" __rseq_str(arg) "] "    /* To memory ("m" constraint) */
+#define LOAD_WORD(arg) "lwd%U[" __rseq_str(arg) "]%X[" __rseq_str(arg) "] "    /* From memory ("m" constraint) */
+#define LOADX_WORD     "ldx "                                                  /* From base register ("b" constraint) */
 #define CMP_WORD       "cmpd "

 #define __RSEQ_ASM_DEFINE_TABLE(label, version, flags,                         \
@@ -89,9 +89,9 @@ do {                                                                  \

 #else /* #ifdef __PPC64__ */

-#define STORE_WORD     "stw "
-#define LOAD_WORD      "lwz "
-#define LOADX_WORD     "lwzx "
+#define STORE_WORD(arg)        "stw%U[" __rseq_str(arg) "]%X[" __rseq_str(arg) "] "    /* To memory ("m" constraint) */
+#define LOAD_WORD(arg) "lwz%U[" __rseq_str(arg) "]%X[" __rseq_str(arg) "] "    /* From memory ("m" constraint) */
+#define LOADX_WORD     "lwzx "                                                 /* From base register ("b" constraint) */
 #define CMP_WORD       "cmpw "

 #define __RSEQ_ASM_DEFINE_TABLE(label, version, flags,                         \
@@ -125,7 +125,7 @@ do {                                                                        \
                RSEQ_INJECT_ASM(1)                                              \
                "lis %%r17, (" __rseq_str(cs_label) ")@ha\n\t"                  \
                "addi %%r17, %%r17, (" __rseq_str(cs_label) ")@l\n\t"           \
-               "stw %%r17, %[" __rseq_str(rseq_cs) "]\n\t"                     \
+               "stw%U[" __rseq_str(rseq_cs) "]%X[" __rseq_str(rseq_cs) "] %%r17, %[" __rseq_str(rseq_cs) "]\n\t"                       \
                __rseq_str(label) ":\n\t"

 #endif /* #ifdef __PPC64__ */
@@ -136,7 +136,7 @@ do {                                                                        \

 #define RSEQ_ASM_CMP_CPU_ID(cpu_id, current_cpu_id, label)                     \
                RSEQ_INJECT_ASM(2)                                              \
-               "lwz %%r17, %[" __rseq_str(current_cpu_id) "]\n\t"              \
+               "lwz%U[" __rseq_str(current_cpu_id) "]%X[" __rseq_str(current_cpu_id) "] %%r17, %[" __rseq_str(current_cpu_id) "]\n\t" \
                "cmpw cr7, %[" __rseq_str(cpu_id) "], %%r17\n\t"                \
                "bne- cr7, " __rseq_str(label) "\n\t"
@@ -153,25 +153,25 @@ do {                                                                      \
  *     RSEQ_ASM_OP_* (else): doesn't have hard-code registers(unless cr7)
  */
 #define RSEQ_ASM_OP_CMPEQ(var, expect, label)                                  \
-               LOAD_WORD "%%r17, %[" __rseq_str(var) "]\n\t"                   \
+               LOAD_WORD(var) "%%r17, %[" __rseq_str(var) "]\n\t"              \
                CMP_WORD "cr7, %%r17, %[" __rseq_str(expect) "]\n\t"            \
                "bne- cr7, " __rseq_str(label) "\n\t"

 #define RSEQ_ASM_OP_CMPNE(var, expectnot, label)                               \
-               LOAD_WORD "%%r17, %[" __rseq_str(var) "]\n\t"                   \
+               LOAD_WORD(var) "%%r17, %[" __rseq_str(var) "]\n\t"              \
                CMP_WORD "cr7, %%r17, %[" __rseq_str(expectnot) "]\n\t"         \
                "beq- cr7, " __rseq_str(label) "\n\t"

 #define RSEQ_ASM_OP_STORE(value, var)                                          \
-               STORE_WORD "%[" __rseq_str(value) "], %[" __rseq_str(var) "]\n\t"
+               STORE_WORD(var) "%[" __rseq_str(value) "], %[" __rseq_str(var) "]\n\t"

 /* Load @var to r17 */
 #define RSEQ_ASM_OP_R_LOAD(var)                                                        \
-               LOAD_WORD "%%r17, %[" __rseq_str(var) "]\n\t"
+               LOAD_WORD(var) "%%r17, %[" __rseq_str(var) "]\n\t"

 /* Store r17 to @var */
 #define RSEQ_ASM_OP_R_STORE(var)                                               \
-               STORE_WORD "%%r17, %[" __rseq_str(var) "]\n\t"
+               STORE_WORD(var) "%%r17, %[" __rseq_str(var) "]\n\t"

 /* Add @count to r17 */
 #define RSEQ_ASM_OP_R_ADD(count)                                               \
@@ -196,11 +196,11 @@ do {                                                                      \
                "333:\n\t" \

 #define RSEQ_ASM_OP_R_FINAL_STORE(var, post_commit_label)                      \
-               STORE_WORD "%%r17, %[" __rseq_str(var) "]\n\t"                  \
+               STORE_WORD(var) "%%r17, %[" __rseq_str(var) "]\n\t"                     \
                __rseq_str(post_commit_label) ":\n\t"

 #define RSEQ_ASM_OP_FINAL_STORE(value, var, post_commit_label)                 \
-               STORE_WORD "%[" __rseq_str(value) "], %[" __rseq_str(var) "]\n\t" \
+               STORE_WORD(var) "%[" __rseq_str(value) "], %[" __rseq_str(var) "]\n\t" \
                __rseq_str(post_commit_label) ":\n\t"

 static inline __attribute__((always_inline))

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com