Re: [PATCH v7 2/5] x86/asm: add volatile, clobbers and zero-length check in inline memcmp

[Mauricio Faria de Oliveira <mfo@xxxxxxxxxx>]

On 2026-07-23 20:12, H. Peter Anvin wrote:
> On 2026-07-22 23:59, Jan Beulich wrote:
>>> 
>>> Also, this is silly. Instead of adding a whole separate test, just do "test 
>>> %3,%3" before the repe to set ZF and let the REPE skip.
>> 
>> Besides this, isn't the function effectively returning bool wrong anyway? 
>> This
>> way you can use it for equal / not-equal comparisons, but not for sorting and
>> alike.
>> 
> There is no use case in the early code for sorting, and it seems rather broken
> to burden the code with that.

If this is merged, other users not in early code might eventually
appear, which might introduce the use case for sorting. Even though that
is unlikely, as such users could probably use regular memcmp() instead,
consider that just in case, so I can ask for your input/advice on this:

> That being said it probably should return bool explicitly (it makes no sense
> for the prototype to be different than the internal variable.)
> 
> We could call it memneq() if someone really, really cares, I guess.
> 
> The early code is very size-sensitive, so I'm really not fond of the idea of
> burdening it further. Perhaps something like:

Do you think this implementation (returns -1/0/+1) is reasonable,
size-wise, for early code?

I considered submitting it eventually, once the return value difference
to regular memcmp() was called out [0], as an improvement, if there's
agreement this would be a good idea considering the scenario above.

static __always_inline int __inline_memcmp(const void *s1, const void
*s2, size_t len)
{
        int above, below;

        asm volatile("test %2, %2\n\t"
                     "repe cmpsb"
                     : "+S" (s1), "+D" (s2), "+c" (len),
                       "=@cca" (above), "=@ccb" (below)
                     : : "memory");

        return above - below;
}

@ arch/x86/boot/string.o
00000028 <memcmp>:
  28:   66 57                   push   %di
  2a:   66 56                   push   %si
  2c:   66 89 c6                mov    %ax,%si
  2f:   66 89 d7                mov    %dx,%di

  32:   66 85 c9                test   %cx,%cx
  35:   f3 a6                   repz cmpsb %es:(%edi),%ds:(%esi)
  37:   0f 97 c0                seta   %al
  3a:   66 0f b6 c0             movzbw %al,%ax
  3e:   0f 92 c2                setb   %dl
  41:   66 0f b6 d2             movzbw %dl,%dx
  45:   66 29 d0                sub    %dx,%ax

  48:   66 5e                   pop    %si
  4a:   66 5f                   pop    %di
  4c:   66 c3                   retw

> A memory clobber is ugly here since no memory is actually modified, although

I'm definitely not an expert, but IIUIC, this memory clobber is for
reads, not writes? Say, a caller/optimized code that writes to the
buffer(s) prior to __inline_memcmp() and data might still reside in
registers; even if theoretical/unlikely.

As in [1]:

  The "memory" clobber tells the compiler that the assembly code
performs memory reads or writes [...] (for example, accessing the memory
pointed to [...]). To ensure memory contains correct values, GCC may
need to flush specific register values to memory before executing the
asm.

> it probably doesn't affect code; "cc" is completely redundant with condition
> code output operand.

Thanks for explaining. I'll remove that in the next version.

[0]
https://lore.kernel.org/all/324ef97b16f52e0ccc72f6381d1b5dd2@xxxxxxxxxx/
[1]
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Clobbers-and-Scratch-Registers-1

cheers,

> static __always_inline bool
> __inline_memcmp(const void *s1, const void *s2, size_t len)
> {
>       bool diff;
> 
>       if (__builtin_constant_p(len == 0)) {
>               if (!len)
>                       return false;
>               asm volatile("repe cmpsb"
>                               : "=@ccnz" (diff),
>                                 "+D" (s1), "+S" (s2), "+c" (len)
>                               : : "memory");
>       } else {
>               /* Clear ZF beforehand in case len == 0 */
>               asm volatile("test %3,%3; repe cmpsb"
>                               : "=@ccnz" (diff),
>                                 "+D" (s1), "+S" (s2), "+c" (len)
>                               : : "memory");
>       }
>       return diff;
> }


-- 
Mauricio