On Fri, Nov 1, 2024 at 3:51 PM Jeff Law <jeffreya...@gmail.com> wrote:
>
> On 11/1/24 8:45 AM, Uros Bizjak wrote:
> > On Fri, Nov 1, 2024 at 2:18 PM Jeff Law <jeffreya...@gmail.com> wrote:
> >>
> >>
> >>
> >> On 11/1/24 5:34 AM, Uros Bizjak wrote:
> >>> Stack pointer modifications in asm are currently not flagged in
> >>> crtl->sp_is_unchanging due to RTX pointer comparison in
> >>> notice_stack_pointer_modification_1. Pointer comparison does not detect
> >>> that "(reg/v:DI 7 sp [ current_stack_pointer ])" and "(reg/f:DI 7 sp)"
> >>> RTXes actually all correspond to the same stack pointer register.
> >>>
> >>> Due to the above omission, the compiler does not detect that asm RTX
> >>> manipulates stack pointer in the following construct:
> >> But how did you get two distinct RTXs for the stack pointer? That's not
> >> supposed to happen IIRC.
> >
> > Please see the testcase in the patch:
> >
> > +register unsigned long current_stack_pointer asm ("%rsp");
> > +#define ASM_CALL_CONSTRAINT "+r" (current_stack_pointer)
> >
> > [...]
> >
> > + asm volatile ("pushfq; push %1; pop %0; popfq"
> > + : "=r" (y), ASM_CALL_CONSTRAINT
> > + : "e" (-1));
> >
> > When compiled for x86_64, the stack pointer RTX that is different from
> > the generic stack pointer RTX is created.
> If we're allowing that, then I suspect there's a good number of places
> that will ultimately need to be fixed. It's baked in pretty deep that
> there is only one stack pointer.
grepping for "== stack_pointer_rtx" in gcc/ and its subdirectories
returns 178 hits, which doesn't look that bad.
> So I think we need to conclude whether or not the testcase is valid or
> not first.
This is what x86 linux kernel uses (arch/x86/include/asm/asm.h) as a
scheduling barrier to prevent asm from being scheduled before stack
frame is constructed:
--cut here--
/*
* This output constraint should be used for any inline asm which has a "call"
* instruction. Otherwise the asm may be inserted before the frame pointer
* gets set up by the containing function. If you forget to do this, objtool
* may print a "call without frame pointer save/setup" warning.
*/
register unsigned long current_stack_pointer asm(_ASM_SP);
#define ASM_CALL_CONSTRAINT "+r" (current_stack_pointer)
--cut here--
since "%rbp" can't be used as a global register variable when
fno-omit-frame-pointer is in effect:
--cut here--
register unsigned long current_frame_pointer asm ("%rbp");
#define ASM_CALL_CONSTRAINT "+r" (current_frame_pointer)
--cut here--
In function ‘asm_clobbers_redzone’:
error: frame pointer required, but reserved
8 | asm_clobbers_redzone (void)
| ^~~~~~~~~~~~~~~~~~~~
note: for ‘current_frame_pointer’
1 | register unsigned long current_frame_pointer asm ("%rbp");
| ^~~~~~~~~~~~~~~~~~~~~
OTOH, declaring and using %rsp as a global register variable doesn't
error out, so one would expect that it would be used throughout the
compiler as an alias to the internal stack pointer. The presented
testcase exposes one place where this assumption doesn't hold.
Uros.
