Re: Failure to build librseq on ppc

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- On Jul 8, 2020, at 8:10 PM, Segher Boessenkool segher@kernel.crashing.org wrote:

> Hi!
> 
> On Wed, Jul 08, 2020 at 10:00:01AM -0400, Mathieu Desnoyers wrote:
[...]
> 
>> -#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) */
> 
> That cannot work (you typoed "ld" here).

Indeed, I noticed it before pushing to master (lwd -> ld).

> 
> Some more advice about this code, pretty generic stuff:

Let's take an example to support the discussion here. I'm taking it from
master branch (after a cleanup changing e.g. LOAD_WORD into RSEQ_LOAD_LONG).
So for powerpc32 we have (code edited to remove testing instrumentation):

#define __rseq_str_1(x) #x
#define __rseq_str(x)           __rseq_str_1(x)

#define RSEQ_STORE_LONG(arg)    "stw%U[" __rseq_str(arg) "]%X[" __rseq_str(arg) "] "    /* To memory ("m" constraint) */
#define RSEQ_STORE_INT(arg)     RSEQ_STORE_LONG(arg)                                    /* To memory ("m" constraint) */
#define RSEQ_LOAD_LONG(arg)     "lwz%U[" __rseq_str(arg) "]%X[" __rseq_str(arg) "] "    /* From memory ("m" constraint) */
#define RSEQ_LOAD_INT(arg)      RSEQ_LOAD_LONG(arg)                                     /* From memory ("m" constraint) */
#define RSEQ_LOADX_LONG         "lwzx "                                                 /* From base register ("b" constraint) */
#define RSEQ_CMP_LONG           "cmpw "

#define __RSEQ_ASM_DEFINE_TABLE(label, version, flags,                          \
                        start_ip, post_commit_offset, abort_ip)                 \
                ".pushsection __rseq_cs, \"aw\"\n\t"                            \
                ".balign 32\n\t"                                                \
                __rseq_str(label) ":\n\t"                                       \
                ".long " __rseq_str(version) ", " __rseq_str(flags) "\n\t"      \
                /* 32-bit only supported on BE */                               \
                ".long 0x0, " __rseq_str(start_ip) ", 0x0, " __rseq_str(post_commit_offset) ", 0x0, " __rseq_str(abort_ip) "\n\t" \
                ".popsection\n\t"                                       \
                ".pushsection __rseq_cs_ptr_array, \"aw\"\n\t"          \
                ".long 0x0, " __rseq_str(label) "b\n\t"                 \
                ".popsection\n\t"

/*
 * Exit points of a rseq critical section consist of all instructions outside
 * of the critical section where a critical section can either branch to or
 * reach through the normal course of its execution. The abort IP and the
 * post-commit IP are already part of the __rseq_cs section and should not be
 * explicitly defined as additional exit points. Knowing all exit points is
 * useful to assist debuggers stepping over the critical section.
 */
#define RSEQ_ASM_DEFINE_EXIT_POINT(start_ip, exit_ip)                           \
                ".pushsection __rseq_exit_point_array, \"aw\"\n\t"              \
                /* 32-bit only supported on BE */                               \
                ".long 0x0, " __rseq_str(start_ip) ", 0x0, " __rseq_str(exit_ip) "\n\t" \
                ".popsection\n\t"

#define RSEQ_ASM_STORE_RSEQ_CS(label, cs_label, rseq_cs)                        \
                "lis %%r17, (" __rseq_str(cs_label) ")@ha\n\t"                  \
                "addi %%r17, %%r17, (" __rseq_str(cs_label) ")@l\n\t"           \
                RSEQ_STORE_INT(rseq_cs) "%%r17, %[" __rseq_str(rseq_cs) "]\n\t" \
                __rseq_str(label) ":\n\t"

#define RSEQ_ASM_DEFINE_TABLE(label, start_ip, post_commit_ip, abort_ip)        \
                __RSEQ_ASM_DEFINE_TABLE(label, 0x0, 0x0, start_ip,              \
                                        (post_commit_ip - start_ip), abort_ip)

#define RSEQ_ASM_CMP_CPU_ID(cpu_id, current_cpu_id, label)                      \
                RSEQ_LOAD_INT(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"

#define RSEQ_ASM_DEFINE_ABORT(label, abort_label)                               \
                ".pushsection __rseq_failure, \"ax\"\n\t"                       \
                ".long " __rseq_str(RSEQ_SIG) "\n\t"                            \
                __rseq_str(label) ":\n\t"                                       \
                "b %l[" __rseq_str(abort_label) "]\n\t"                         \
                ".popsection\n\t"

#define RSEQ_ASM_OP_CMPEQ(var, expect, label)                                   \
                RSEQ_LOAD_LONG(var) "%%r17, %[" __rseq_str(var) "]\n\t"         \
                RSEQ_CMP_LONG "cr7, %%r17, %[" __rseq_str(expect) "]\n\t"               \
                "bne- cr7, " __rseq_str(label) "\n\t"

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

static inline __attribute__((always_inline))
int rseq_cmpeqv_storev(intptr_t *v, intptr_t expect, intptr_t newv, int cpu)
{
        __asm__ __volatile__ goto (
                RSEQ_ASM_DEFINE_TABLE(3, 1f, 2f, 4f) /* start, commit, abort */
                RSEQ_ASM_DEFINE_EXIT_POINT(1f, %l[cmpfail])

                /* Start rseq by storing table entry pointer into rseq_cs. */
                RSEQ_ASM_STORE_RSEQ_CS(1, 3b, rseq_cs)
                /* cmp cpuid */
                RSEQ_ASM_CMP_CPU_ID(cpu_id, current_cpu_id, 4f)
                /* cmp @v equal to @expect */
                RSEQ_ASM_OP_CMPEQ(v, expect, %l[cmpfail])

                /* final store */
                RSEQ_ASM_OP_FINAL_STORE(newv, v, 2)
                RSEQ_ASM_DEFINE_ABORT(4, abort)

                : /* gcc asm goto does not allow outputs */
                : [cpu_id]              "r" (cpu),
                  [current_cpu_id]      "m" (__rseq_abi.cpu_id),
                  [rseq_cs]             "m" (__rseq_abi.rseq_cs),
                  [v]                   "m" (*v),
                  [expect]              "r" (expect),
                  [newv]                "r" (newv)
                : "memory", "cc", "r17"
                : abort, cmpfail
        );
        return 0;
abort:
        RSEQ_INJECT_FAILED
        return -1;
cmpfail:
        return 1;
}

> 
> The way this all uses r17 will likely not work reliably.

r17 is only used as a temporary register within the inline assembler, and it is
in the clobber list. In which scenario would it not work reliably ?

> The way multiple asm statements are used seems to have missing
> dependencies between the statements.

I'm not sure I follow here. Note that we are injecting the CPP macros into
a single inline asm statement as strings.

> 
> Don't try to work *against* the compiler.  You will not win.
> 
> Alternatively, write assembler code, if that is what you actually want
> to do?  Not C code.
>
> And done macro-mess this, you want to be able to debug it, and you need
> other people to be able to read it!

I understand that looking at macros can be cumbersome from the perspective
of a reviewer only interested in a single architecture,

However, from my perspective, as a maintainer who must maintain similar code
for x86 32/64, powerpc 32/64, arm, aarch64, s390, s390x, mips 32/64, and likely
other architectures in the future, the macros abstracting 32-bit and 64-bit
allow to eliminate code duplication for each architecture with 32-bit and 64-bit
variants, which is better for maintainability.

Thanks,

Mathieu

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