----- On Jul 8, 2020, at 8:10 PM, Segher Boessenkool seg...@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
