Re: [PATCH v6 1/2] x86/delay: Fix the wrong asm constraint in `delay_loop()`

From: Ammar Faizi
Date: Sun Apr 03 2022 - 13:11:51 EST


On 4/3/22 11:57 PM, Thomas Gleixner wrote:
On Tue, Mar 29 2022 at 17:47, Ammar Faizi wrote:
The asm constraint does not reflect that the asm statement can modify
the value of @loops. But the asm statement in delay_loop() does modify
the @loops.

Specifiying the wrong constraint may lead to undefined behavior, it may
clobber random stuff (e.g. local variable, important temporary value in
regs, etc.). This is especially dangerous when the compiler decides to
inline the function and since it doesn't know that the value gets
modified, it might decide to use it from a register directly without
reloading it.

Fix this by changing the constraint from "a" (as an input) to "+a" (as
an input and output).

This analysis is plain wrong. The assembly code operates on a register
and not on memory:



asm volatile(
" test %0,%0 \n"
" jz 3f \n"
" jmp 1f \n"

".align 16 \n"
"1: jmp 2f \n"

".align 16 \n"
"2: dec %0 \n"
" jnz 2b \n"
"3: dec %0 \n"

: /* we don't need output */
----> :"a" (loops)

This tells the compiler to use [RE]AX and initialize it from the
variable 'loops'. It's never written back because all '%0' in the above
assembly are substituted with [RE]AX. This also tells the compiler that
the inline assembly clobbers [RE]AX and that's all it needs to know.

Hi Thomas,

Thanks for taking a look. I doubt about your sentence "This also tells
the compiler that the inline assembly clobbers [RE]AX".

How come it tells the compiler that the inline ASM clobbers [RE]AX?

That's an input constraint. Doesn't that mean it is read-only for the
ASM statement? That means the compiler is allowed to assume [RE]AX doesn't
change inside the ASM statement.

Those `dec`s do really change the [RE]AX. Please review this again.

Thanks!

--
Ammar Faizi