On Wed, Nov 06, 2024 at 10:44:54AM +0100, Uros Bizjak wrote:
> After some more thinking and considering all recent discussion
> (thanks!), I am convinced that a slightly simplified original patch
> (attached), now one-liner, is the way to go.
>
> Let's look at the following test:
>
> --cut here--
> unsigned long foo (void)
> {
> return __builtin_ia32_readeflags_u64 ();
> }
> --cut here--
In theory we could handle this one not by disabling red zone altogether,
but just forcing the backend to reserve the uppermost 64 bits of the red zone
if it sees such an instruction. But probably not worth it. And
could work fine if push is enforced to be adjacent to the pop (but
having them in the same insn will not result in correct .eh_frame),
though if it can be separated from it and some memory loads/stores can
appear in between it could be nasty.
> Now consider the following test:
>
> --cut here--
> register unsigned long current_stack_pointer asm ("%rsp");
> #define ASM_CALL_CONSTRAINT "+r" (current_stack_pointer)
>
> unsigned long bar (void)
> {
> unsigned long res;
>
> asm ("pushfq" : ASM_CALL_CONSTRAINT);
> asm ("popq %0" : "=r" (res), ASM_CALL_CONSTRAINT);
> return res;
> }
> --cut here--
This test is certainly invalid.
Modifying stack pointer by the inline asm results in something the
compiler can't deal with, e.g. if it needs to reload something across
the asm and not using frame pointers, it just can't recover, it doesn't
know where what it saved earlier will be, because stack pointer changed.
Similarly it would be invalid to do current_stack_pointer = buf;
or similar.
> --cut here--
> register unsigned long current_stack_pointer asm ("%rsp");
> #define ASM_CALL_CONSTRAINT "+r" (current_stack_pointer)
>
> unsigned long baz (void)
> {
> unsigned long res;
>
> asm ("pushfq; popq %0" : "=r" (res), ASM_CALL_CONSTRAINT);
> return res;
> }
> --cut here--
>
> here we inform the compiler that we read and write the rsp, not
> necessarily change it, maybe something like "add %rsp, $0". As above,
> notice_stack_pointer_modification_1 does not trigger here.
While this happened to work, we've discussed several times this isn't
something we want to recommend to people.
Generally, if inline asm temporarily changes some register and restores it
back before returning to C code, there is no need to mention that register
in clobbers or in "+r" operand. NAnd the "+r" (current_stack_pointer)
says that the stack pointer was or could have been changed, which is wrong,
valid inline asm can never change the stack pointer. It can change it
temporarily and then restore if it knows what it is doing.
> --cut here--
> register unsigned long current_stack_pointer asm ("%rsp");
> #define ASM_CALL_CONSTRAINT "+r" (current_stack_pointer)
>
> void test (void)
> {
> current_stack_pointer += 1;
> }
> --cut here
As I said above, this is invalid testcase too.
Jakub
