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?
Uros.
