[PATCH v8 7/8] aarch64: Add rseq_load32_load32_relaxed

[DJ Delorie]

Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:

> On 2024-02-16 22:53, DJ Delorie wrote:
> [...]
>> 
>>> +#define RSEQ_ASM_OP_CMPEQ(var, expect, label)					\
>>> +	"	ldr	" RSEQ_ASM_TMP_REG ", %[" __rseq_str(var) "]\n"		\
>>> +	"	sub	" RSEQ_ASM_TMP_REG ", " RSEQ_ASM_TMP_REG		\
>>> +			", %[" __rseq_str(expect) "]\n"				\
>>> +	"	cbnz	" RSEQ_ASM_TMP_REG ", " __rseq_str(label) "\n"
>> 
>> This is why we need documentation; I would have guessed this was a CMPNE
>> operation, but it depends on how you define "label"
>
> This comes from the x86 implementation I originally did which has
> "RSEQ_ASM_CMP_CPU_ID()" and static inline functions such as
> "rseq_cmpeqv_storev()". The meaning of the "cmp" here is that the
> critical section does _not_ abort (does not branch) if the comparison
> matches.

Given that I'm looking at it as "someone not familiar with the RSEQ
API[*]", perhaps a one line comment that says "compare VAR and EXPECT
and ensure they're equal, else abort to LABEL" would have made me think
"Oh, that makes sense".  Otherwise I have to read the inline asm, and my
familiarity with x86 asm would make me confused.

> However, the ASM macros would benefit from a logic flip. Even though
> the API is not stable, I would like to introduce this in a way that
> will allow users of the API to catch the change at compile-time.

I'm not pushing for an ABI change at this point.  *Any* undocumented
inline assembler macro is going to be confusing to someone not familiar
with it, and needs a human-understandable comment or documentation for
it.

>>> +#define RSEQ_ASM_OP_R_BAD_MEMCPY(dst, src, len)					\
>>> +	"	cbz	%[" __rseq_str(len) "], 333f\n"				\
>>> +	"	mov	" RSEQ_ASM_TMP_REG_2 ", %[" __rseq_str(len) "]\n"	\
>>> +	"222:	sub	" RSEQ_ASM_TMP_REG_2 ", " RSEQ_ASM_TMP_REG_2 ", #1\n"	\
>>> +	"	ldrb	" RSEQ_ASM_TMP_REG32 ", [%[" __rseq_str(src) "]"	\
>>> +			", " RSEQ_ASM_TMP_REG_2 "]\n"				\
>>> +	"	strb	" RSEQ_ASM_TMP_REG32 ", [%[" __rseq_str(dst) "]"	\
>>> +			", " RSEQ_ASM_TMP_REG_2 "]\n"				\
>>> +	"	cbnz	" RSEQ_ASM_TMP_REG_2 ", 222b\n"				\
>>> +	"333:\n"
>> 
>> Ok, but WHY?
>
> This is a memcpy from src to dst which can be aborted at any point
> during the copy. Do you recommend we add documentation about what it does,

Yes.  A short comment that says "This is a slow dumb memcpy, but it can
be used in an rseq abortable code sequence." would be sufficient.
That's what I meant by my WHY? comment - there should be something that
explains why it exists and/or why you'd use it.

And for a bit of humorous snark, I'll point out that this mail thread is
already longer than the new documentation would need to be ;-)


[*] And someone old enough to realize that comments aren't just for your
    peers, but also for your future self, to remind you what the heck
    you were thinking when you wrote that ;-)