Messages in this thread |  | | Date | Mon, 4 Apr 2022 00:11:31 +0700 | From | Ammar Faizi <> | Subject | Re: [PATCH v6 1/2] x86/delay: Fix the wrong asm constraint in `delay_loop()` |
| |
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
|  |