On Tue, Nov 5, 2024 at 8:54 AM Uros Bizjak <ubiz...@gmail.com> wrote:
>
> On Tue, Nov 5, 2024 at 1:08 AM Jeff Law <jeffreya...@gmail.com> wrote:
> >
> >
> >
> > On 11/1/24 10:44 AM, Uros Bizjak wrote:
> > > 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.
> > But if we allow this case, then we probably need to fix the frame
> > pointer, hard frame pointer and arg pointer cases too.
>
> I don't think it is possible to declare a global register that would
> conflict with "%frame" or "%argp" and "%rbp" (with
> -fno-omit-frame-pointer). Declaring these as global register in the
> following source:
>
> --cut here--
> register unsigned long hardreg asm ("%frame");
> #define ASM_CONSTRAINT "+r" (hardreg)
>
> void foo (void)
> {
> asm volatile ("#" : ASM_CONSTRAINT);
> }
> --cut here--
>
> errors out with:
>
> error: register specified for ‘hardreg’ is an internal GCC implementation
> detail
>
> while declaring global register with "%rbp" errors out with:
>
> error: frame pointer required, but reserved
>
> when -fno-omit-frame-pointer is in effect.
>
> > >
> > >> 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:
> > Just because the kernel does it, doesn't make it right :-)
>
> True, it was not my intention to declare the usage in the kernel the
> correct one ;) . There is in fact PR [1] with a request for what is
> the correct construct to use to prevent scheduling in front of the
> frame pointer initialization (and other issues with changing SP) in
> case when assembly changes (not clobbers!) the stack pointer.
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117311
>
> > ~~~~~~~~~~~~~~
> > >
> > > 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.
> > But maybe it should error out or should be transformed internally to
> > reference the singleton rtx object.
>
> Adding a SP clobber to asm:
>
> void bar (void)
> {
> asm volatile ("#" : : : "rsp");
> }
>
> results in:
>
> warning: listing the stack pointer register ‘rsp’ in a clobber list is
> deprecated [-Wdeprecated]
> note: the value of the stack pointer after an ‘asm’ statement must be
> the same as it was before the statement
>
> But what is the non-deprecated way to communicate the fact that SP
> changes, and possibly clobbers stack in the asm to the compiler?
I don't think there's an existing way to tell the compiler that "arbitrary stack
locations" are clobbered (SP doesn't really change). Only stack portions
allocated to variables are considered clobbered with a "memory" clobber.
I suggest to write an assembly routine if people want to scribble to random
%sp based addresses.
I'll note that those "random" (not variable backed) stack locations might be
used by the compiler for example for spill slots - there's also no way to
prevent or communicate the allocation of stack for such purposes, so the
asm() would have to preserve all stack content and not only the stack
pointer.
Richard.
>
> Uros.
