[PATCH] strub: Only unbias stack point for SPARC_STACK_BOUNDARY_HACK [PR113100]

[PATCH] strub: Only unbias stack point for SPARC_STACK_BOUNDARY_HACK [PR113100]

[Kewen.Lin]

Hi,

As PR113100 shows, the unbiasing introduced by r14-6737 can
cause the scrubbing to overrun and screw some critical data
on stack like saved toc base consequently cause segfault on
Power.

By checking PR112917, IMHO we should keep this unbiasing
guarded under SPARC_STACK_BOUNDARY_HACK (TARGET_ARCH64 &&
TARGET_STACK_BIAS), similar to some existing code special
treating SPARC stack bias.

Bootstrapped and regtested on x86_64-redhat-linux and
powerpc64{,le}-linux-gnu.  All reported failures in
PR113100 are gone.  I also expect the culprit commit can
affect those ports with nonzero STACK_POINTER_OFFSET.

Is it ok for trunk?

BR,
Kewen
-----
	PR middle-end/113100

gcc/ChangeLog:

	* builtins.cc (expand_builtin_stack_address): Guard stack point
	adjustment with SPARC_STACK_BOUNDARY_HACK.
---
 gcc/builtins.cc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gcc/builtins.cc b/gcc/builtins.cc
index 125ea158ebf..9bad1e962b4 100644
--- a/gcc/builtins.cc
+++ b/gcc/builtins.cc
@@ -5450,6 +5450,7 @@ expand_builtin_stack_address ()
   rtx ret = convert_to_mode (ptr_mode, copy_to_reg (stack_pointer_rtx),
 			     STACK_UNSIGNED);

+#ifdef SPARC_STACK_BOUNDARY_HACK
   /* Unbias the stack pointer, bringing it to the boundary between the
      stack area claimed by the active function calling this builtin,
      and stack ranges that could get clobbered if it called another
@@ -5476,7 +5477,9 @@ expand_builtin_stack_address ()
      (caller) function's active area as well, whereas those pushed or
      allocated temporarily for a call are regarded as part of the
      callee's stack range, rather than the caller's.  */
-  ret = plus_constant (ptr_mode, ret, STACK_POINTER_OFFSET);
+  if (SPARC_STACK_BOUNDARY_HACK)
+    ret = plus_constant (ptr_mode, ret, STACK_POINTER_OFFSET);
+#endif

   return force_reg (ptr_mode, ret);
 }
--
2.39.3

Re: [PATCH] strub: Only unbias stack point for SPARC_STACK_BOUNDARY_HACK [PR113100]

[Richard Biener]

On Mon, Jan 8, 2024 at 3:35 AM Kewen.Lin <linkw@linux.ibm.com> wrote:
>
> Hi,
>
> As PR113100 shows, the unbiasing introduced by r14-6737 can
> cause the scrubbing to overrun and screw some critical data
> on stack like saved toc base consequently cause segfault on
> Power.
>
> By checking PR112917, IMHO we should keep this unbiasing
> guarded under SPARC_STACK_BOUNDARY_HACK (TARGET_ARCH64 &&
> TARGET_STACK_BIAS), similar to some existing code special
> treating SPARC stack bias.
>
> Bootstrapped and regtested on x86_64-redhat-linux and
> powerpc64{,le}-linux-gnu.  All reported failures in
> PR113100 are gone.  I also expect the culprit commit can
> affect those ports with nonzero STACK_POINTER_OFFSET.
>
> Is it ok for trunk?

OK

> BR,
> Kewen
> -----
>         PR middle-end/113100
>
> gcc/ChangeLog:
>
>         * builtins.cc (expand_builtin_stack_address): Guard stack point
>         adjustment with SPARC_STACK_BOUNDARY_HACK.
> ---
>  gcc/builtins.cc | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/builtins.cc b/gcc/builtins.cc
> index 125ea158ebf..9bad1e962b4 100644
> --- a/gcc/builtins.cc
> +++ b/gcc/builtins.cc
> @@ -5450,6 +5450,7 @@ expand_builtin_stack_address ()
>    rtx ret = convert_to_mode (ptr_mode, copy_to_reg (stack_pointer_rtx),
>                              STACK_UNSIGNED);
>
> +#ifdef SPARC_STACK_BOUNDARY_HACK
>    /* Unbias the stack pointer, bringing it to the boundary between the
>       stack area claimed by the active function calling this builtin,
>       and stack ranges that could get clobbered if it called another
> @@ -5476,7 +5477,9 @@ expand_builtin_stack_address ()
>       (caller) function's active area as well, whereas those pushed or
>       allocated temporarily for a call are regarded as part of the
>       callee's stack range, rather than the caller's.  */
> -  ret = plus_constant (ptr_mode, ret, STACK_POINTER_OFFSET);
> +  if (SPARC_STACK_BOUNDARY_HACK)
> +    ret = plus_constant (ptr_mode, ret, STACK_POINTER_OFFSET);
> +#endif
>
>    return force_reg (ptr_mode, ret);
>  }
> --
> 2.39.3

Re: [PATCH] strub: Only unbias stack point for SPARC_STACK_BOUNDARY_HACK [PR113100]

[Kewen.Lin]

on 2024/1/8 19:44, Richard Biener wrote:
> On Mon, Jan 8, 2024 at 3:35 AM Kewen.Lin <linkw@linux.ibm.com> wrote:
>>
>> Hi,
>>
>> As PR113100 shows, the unbiasing introduced by r14-6737 can
>> cause the scrubbing to overrun and screw some critical data
>> on stack like saved toc base consequently cause segfault on
>> Power.
>>
>> By checking PR112917, IMHO we should keep this unbiasing
>> guarded under SPARC_STACK_BOUNDARY_HACK (TARGET_ARCH64 &&
>> TARGET_STACK_BIAS), similar to some existing code special
>> treating SPARC stack bias.
>>
>> Bootstrapped and regtested on x86_64-redhat-linux and
>> powerpc64{,le}-linux-gnu.  All reported failures in
>> PR113100 are gone.  I also expect the culprit commit can
>> affect those ports with nonzero STACK_POINTER_OFFSET.
>>
>> Is it ok for trunk?
> 
> OK

Thanks!  Pushed as r14-7089.

BR,
Kewen

> 
>> BR,
>> Kewen
>> -----
>>         PR middle-end/113100
>>
>> gcc/ChangeLog:
>>
>>         * builtins.cc (expand_builtin_stack_address): Guard stack point
>>         adjustment with SPARC_STACK_BOUNDARY_HACK.
>> ---
>>  gcc/builtins.cc | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/gcc/builtins.cc b/gcc/builtins.cc
>> index 125ea158ebf..9bad1e962b4 100644
>> --- a/gcc/builtins.cc
>> +++ b/gcc/builtins.cc
>> @@ -5450,6 +5450,7 @@ expand_builtin_stack_address ()
>>    rtx ret = convert_to_mode (ptr_mode, copy_to_reg (stack_pointer_rtx),
>>                              STACK_UNSIGNED);
>>
>> +#ifdef SPARC_STACK_BOUNDARY_HACK
>>    /* Unbias the stack pointer, bringing it to the boundary between the
>>       stack area claimed by the active function calling this builtin,
>>       and stack ranges that could get clobbered if it called another
>> @@ -5476,7 +5477,9 @@ expand_builtin_stack_address ()
>>       (caller) function's active area as well, whereas those pushed or
>>       allocated temporarily for a call are regarded as part of the
>>       callee's stack range, rather than the caller's.  */
>> -  ret = plus_constant (ptr_mode, ret, STACK_POINTER_OFFSET);
>> +  if (SPARC_STACK_BOUNDARY_HACK)
>> +    ret = plus_constant (ptr_mode, ret, STACK_POINTER_OFFSET);
>> +#endif
>>
>>    return force_reg (ptr_mode, ret);
>>  }
>> --
>> 2.39.3

Re: [PATCH] strub: Only unbias stack point for SPARC_STACK_BOUNDARY_HACK [PR113100]

[Alexandre Oliva]

On Jan  7, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:

> As PR113100 shows, the unbiasing introduced by r14-6737 can
> cause the scrubbing to overrun and screw some critical data
> on stack like saved toc base consequently cause segfault on
> Power.

Ugh.  Sorry about the breakage, and thanks for addressing it during my
absence.  Happy GNU Year! :-)

> By checking PR112917, IMHO we should keep this unbiasing
> guarded under SPARC_STACK_BOUNDARY_HACK (TARGET_ARCH64 &&
> TARGET_STACK_BIAS), similar to some existing code special
> treating SPARC stack bias.

I'm afraid this change will most certainly regress 32-bit sparc, because
of the large register save area.

I had been hesitant to introduce yet another target configuration knob,
but it looks like this is what we're going to have to do to accommodate
all targets.

> I also expect the culprit commit can
> affect those ports with nonzero STACK_POINTER_OFFSET.

IMHO it really shouldn't.  STACK_POINTER_OFFSET should be the "Offset
from the stack pointer register to the first location at which outgoing
arguments are placed", which suggests to me that no data that the callee
couldn't change should go in the area below (or above) %sp+S_P_O.

ISTM that PPC sets up a save area between the outgoing args and the
stack pointer; I don't think that's very common, but I suppose other
targets that do so would also define STACK_POINTER_OFFSET to nonzero so
as to reserve those bits.  But whether they should be cleared by stack
scrubbing, as on sparc, or preserved, as on ppc, depends on the ABI
conventions, so we probably can't help yet another knob :-/

I'll take care of that, and update the corresponding documentation.

Thanks,

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH] strub: Only unbias stack point for SPARC_STACK_BOUNDARY_HACK [PR113100]

[Kewen.Lin]

Hi Alexandre,

on 2024/1/11 17:05, Alexandre Oliva wrote:
> On Jan  7, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:
> 
>> As PR113100 shows, the unbiasing introduced by r14-6737 can
>> cause the scrubbing to overrun and screw some critical data
>> on stack like saved toc base consequently cause segfault on
>> Power.
> 
> Ugh.  Sorry about the breakage, and thanks for addressing it during my
> absence.  Happy GNU Year! :-)
> 

No problem!  Happy New Year! :)

>> By checking PR112917, IMHO we should keep this unbiasing
>> guarded under SPARC_STACK_BOUNDARY_HACK (TARGET_ARCH64 &&
>> TARGET_STACK_BIAS), similar to some existing code special
>> treating SPARC stack bias.
> 
> I'm afraid this change will most certainly regress 32-bit sparc, because
> of the large register save area.

Oh, I read the comments and commit logs in PR112917, mainly
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112917#{c4,c5,c6},
and the "sparc64" in subject of commit r14-6737 also implies
that this unbiasing is only required for sparc64, so I thought
it should be safe to guard with SPARC_STACK_BOUNDARY_HACK.

CC Rainer (reporter of PR112917), maybe he already noticed if it's
regressed or not.

> 
> I had been hesitant to introduce yet another target configuration knob,
> but it looks like this is what we're going to have to do to accommodate
> all targets.
> 
>> I also expect the culprit commit can
>> affect those ports with nonzero STACK_POINTER_OFFSET.
> 
> IMHO it really shouldn't.  STACK_POINTER_OFFSET should be the "Offset
> from the stack pointer register to the first location at which outgoing
> arguments are placed", which suggests to me that no data that the callee
> couldn't change should go in the area below (or above) %sp+S_P_O.
> 
> ISTM that PPC sets up a save area between the outgoing args and the

Yes, taking 64-bit PowerPC ELF abi 1.9 as example:

          |   Parameter save area    (SP + 48)
          |   TOC save area          (SP + 40)
          |   link editor doubleword (SP + 32)
          |   compiler doubleword    (SP + 24)
          |   LR save area           (SP + 16)
          |   CR save area           (SP + 8)
SP  --->  +-- Back chain             (SP + 0)

https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#STACK

64-bit PowerPC ELF abi v2 drops "link editor doubleword" and "compiler
doubleword".  PR113100 failures mainly suffer from the TOC saved value
changed when it's used for TOC BASE restoring.

> stack pointer; I don't think that's very common, but I suppose other
> targets that do so would also define STACK_POINTER_OFFSET to nonzero so
> as to reserve those bits.  But whether they should be cleared by stack
> scrubbing, as on sparc, or preserved, as on ppc, depends on the ABI
> conventions, so we probably can't help yet another knob :-/

I agree, it really depends on ABI conventions, taking 64-bit PowerPC
ELF abi as example, some of them is safe to be scrubbed like "LR save
area", some of them depends on if they are used like "TOC save area".
It reminds me that even if on somewhere there is no failures with
scrubbing them, it doesn't really mean it's always safe to scrub since
it's possible that there are no enough testing coverage.  For example,
back chain gets cleared but no issues would be exposed if there is no
test case happening to do some jobs with back chain.  From this
perspective, excepting for the special need of sparc unbiasing, without
examining the specific ABIs, IMHO it's more conservative (not risky) not
to scrub this area than scrubbing it?

> 
> I'll take care of that, and update the corresponding documentation.

Nice, thanks!  Welcome back. :-)

BR,
Kewen

Re: [PATCH] strub: Only unbias stack point for SPARC_STACK_BOUNDARY_HACK [PR113100]

[Alexandre Oliva]

On Jan 12, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:

>>> By checking PR112917, IMHO we should keep this unbiasing
>>> guarded under SPARC_STACK_BOUNDARY_HACK (TARGET_ARCH64 &&
>>> TARGET_STACK_BIAS), similar to some existing code special
>>> treating SPARC stack bias.
>> 
>> I'm afraid this change will most certainly regress 32-bit sparc, because
>> of the large register save area.

> Oh, I read the comments and commit logs in PR112917, mainly
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112917#{c4,c5,c6},
> and the "sparc64" in subject of commit r14-6737 also implies
> that this unbiasing is only required for sparc64, so I thought
> it should be safe to guard with SPARC_STACK_BOUNDARY_HACK.

It is safe, in a way, because that protects potentially active stack
areas, but it's unsafe in that it may leak data that stack scrubbing was
supposed to scrub.  There's no conservative solution here, alas; we have
to get it just right.

Specifically on sparc32, if __builtin_scrub_leave allocated its own
frame (it doesn't) with the large register-save area for its potential
(but inexistent) callees to use, it could overlap with a large chunk of
the very stack frame that it's supposed to clear.

Unfortunately, this is slowly drifting away from the notion of stack
address.  I mean, all of the following could conceivably be returned by
__builtin_stack_address:

- the (biased) stack pointer

- the address of the red zone

- the unbiased stack pointer

- the address of the save area reserved by callees for potential callees

- the boundary between caller- and callee-used stack space

The last one is what we need for stack scrubbing, so that's what I'm
planning to implement, but I'm pondering whether to change
__builtin_stack_address() to take an extra argument to select among the
different possibilities, or of other means to query these various
offsets.  It feels like overthinking, so I'm trying to push these
thoughts aside, but...  Does anyone think that would be a desirable
feature?  We can always add it later.


>> ISTM that PPC sets up a save area between the outgoing args and the

> Yes, taking 64-bit PowerPC ELF abi 1.9 as example:

*nod*, and that's a caller-used save area, as opposed to sparc's
callee-used save area.  Whereas the caller-used area needs to be
preserved across a call, the callee-used one could conceivably even be
used as scratch space by the caller.

> Nice, thanks!  Welcome back. :-)

Thank you!

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH] strub: Only unbias stack point for SPARC_STACK_BOUNDARY_HACK [PR113100]

[Kewen.Lin]

on 2024/1/12 19:03, Alexandre Oliva wrote:
> On Jan 12, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:
> 
>>>> By checking PR112917, IMHO we should keep this unbiasing
>>>> guarded under SPARC_STACK_BOUNDARY_HACK (TARGET_ARCH64 &&
>>>> TARGET_STACK_BIAS), similar to some existing code special
>>>> treating SPARC stack bias.
>>>
>>> I'm afraid this change will most certainly regress 32-bit sparc, because
>>> of the large register save area.
> 
>> Oh, I read the comments and commit logs in PR112917, mainly
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112917#{c4,c5,c6},
>> and the "sparc64" in subject of commit r14-6737 also implies
>> that this unbiasing is only required for sparc64, so I thought
>> it should be safe to guard with SPARC_STACK_BOUNDARY_HACK.
> 
> It is safe, in a way, because that protects potentially active stack
> areas, but it's unsafe in that it may leak data that stack scrubbing was
> supposed to scrub.  There's no conservative solution here, alas; we have
> to get it just right.
> 
> Specifically on sparc32, if __builtin_scrub_leave allocated its own
> frame (it doesn't) with the large register-save area for its potential
> (but inexistent) callees to use, it could overlap with a large chunk of
> the very stack frame that it's supposed to clear.

Thanks for the further explanation!

> 
> Unfortunately, this is slowly drifting away from the notion of stack
> address.  I mean, all of the following could conceivably be returned by
> __builtin_stack_address:
> 
> - the (biased) stack pointer
> 
> - the address of the red zone
> 
> - the unbiased stack pointer
> 
> - the address of the save area reserved by callees for potential callees
> 
> - the boundary between caller- and callee-used stack space
> 
> The last one is what we need for stack scrubbing, so that's what I'm
> planning to implement, but I'm pondering whether to change
> __builtin_stack_address() to take an extra argument to select among the
> different possibilities, or of other means to query these various
> offsets.  It feels like overthinking, so I'm trying to push these
> thoughts aside, but...  Does anyone think that would be a desirable
> feature?  We can always add it later.

One immature idea: maybe we can introduce a hook with clear meaning for
the last one and its default implementation still adopts the function
__builtin_stack_address directly, if this default implementation for
some port is imperfect, someone who is familiar with its own ABIs can
further enhance it with its own hook implementation.

BR,
Kewen

Re: [PATCH] strub: Only unbias stack point for SPARC_STACK_BOUNDARY_HACK [PR113100]

[Alexandre Oliva]

David,

On Jan  7, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:

> As PR113100 shows, the unbiasing introduced by r14-6737 can
> cause the scrubbing to overrun and screw some critical data
> on stack like saved toc base consequently cause segfault on
> Power.

I suppose this problem that Kewen fixed (thanks) was what caused you to
install commit r14-6838.  According to posted test results, strub worked
on AIX until Dec 20, when the fixes for sparc that broke strub on ppc
went in.

I can't seem to find the email in which you posted the patch, and I'd
have appreciated if you'd copied me.  I wouldn't have missed it for so
long if you had.  Since I couldn't find that patch, I'm responding in
this thread instead.

The r14-6838 patch is actually very very broken.  Disabling strub on a
target is not a matter of changing only the testsuite.  Your additions
to the tests even broke the strub-unsupported testcases, that tested
exactly the feature that enables ports to disable strub in a way that
informs users in case they attempt to use it.

I'd thus like to revert that patch.

Kewen's patch needs a little additional cleanup, that I'm preparing now,
to restore fully-functioning strub on sparc32.

Please let me know in case you observe any other problems related with
strub.  I'd be happy to fix them, but I can only do so once I'm aware of
them.

In case the reversal or the upcoming cleanup has any negative impact,
please make sure you let me know.

Thanks,

Happy GNU Year!

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH] strub: Only unbias stack point for SPARC_STACK_BOUNDARY_HACK [PR113100]

[David Edelsohn]

[-- Attachment #1: Type: text/plain, Size: 2100 bytes --]

If the fixes remove the failures on AIX, then the patch to disable the
tests also can be reverted.

Thanks, David


On Wed, Jan 17, 2024 at 8:06 PM Alexandre Oliva <oliva@adacore.com> wrote:

> David,
>
> On Jan  7, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:
>
> > As PR113100 shows, the unbiasing introduced by r14-6737 can
> > cause the scrubbing to overrun and screw some critical data
> > on stack like saved toc base consequently cause segfault on
> > Power.
>
> I suppose this problem that Kewen fixed (thanks) was what caused you to
> install commit r14-6838.  According to posted test results, strub worked
> on AIX until Dec 20, when the fixes for sparc that broke strub on ppc
> went in.
>
> I can't seem to find the email in which you posted the patch, and I'd
> have appreciated if you'd copied me.  I wouldn't have missed it for so
> long if you had.  Since I couldn't find that patch, I'm responding in
> this thread instead.
>
> The r14-6838 patch is actually very very broken.  Disabling strub on a
> target is not a matter of changing only the testsuite.  Your additions
> to the tests even broke the strub-unsupported testcases, that tested
> exactly the feature that enables ports to disable strub in a way that
> informs users in case they attempt to use it.
>
> I'd thus like to revert that patch.
>
> Kewen's patch needs a little additional cleanup, that I'm preparing now,
> to restore fully-functioning strub on sparc32.
>
> Please let me know in case you observe any other problems related with
> strub.  I'd be happy to fix them, but I can only do so once I'm aware of
> them.
>
> In case the reversal or the upcoming cleanup has any negative impact,
> please make sure you let me know.
>
> Thanks,
>
> Happy GNU Year!
>
> --
> Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
>    Free Software Activist                   GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive
>

Re: [PATCH] strub: Only unbias stack point for SPARC_STACK_BOUNDARY_HACK [PR113100]

[Kewen.Lin]

Hi David,

on 2024/1/18 09:27, David Edelsohn wrote:
> If the fixes remove the failures on AIX, then the patch to disable the tests also can be reverted.
> 

Since I didn't find strub-unsupported*.c failed on ppc64 linux, to ensure it's
related, I reverted your commit r14-6838 and my fix r14-7089 locally and supposed
to see those test cases failed on aix, but they passed.  Then I tried to reset
the repo to r14-6275 which added those test cases, and supposed to see they failed,
then they still passed.  Not sure if I missed something in the testing, could you
kindly double check if those test cases started to fail from r14-6275 on your
env? or some other specific commit?  Or maybe directly verify if they can pass
on latest trunk with r14-6838 reverted.  Just to ensure the reverting matches
our expectation.  Thanks in advance!

btw, the command I used to test on aix is:
make check-gcc RUNTESTFLAGS="--target_board=unix'{-m64,-m32}' dg.exp=strub-unsupported*.c"

BR,
Kewen
 
> Thanks, David
> 
> 
> On Wed, Jan 17, 2024 at 8:06 PM Alexandre Oliva <oliva@adacore.com <mailto:oliva@adacore.com>> wrote:
> 
>     David,
> 
>     On Jan  7, 2024, "Kewen.Lin" <linkw@linux.ibm.com <mailto:linkw@linux.ibm.com>> wrote:
> 
>     > As PR113100 shows, the unbiasing introduced by r14-6737 can
>     > cause the scrubbing to overrun and screw some critical data
>     > on stack like saved toc base consequently cause segfault on
>     > Power.
> 
>     I suppose this problem that Kewen fixed (thanks) was what caused you to
>     install commit r14-6838.  According to posted test results, strub worked
>     on AIX until Dec 20, when the fixes for sparc that broke strub on ppc
>     went in.
> 
>     I can't seem to find the email in which you posted the patch, and I'd
>     have appreciated if you'd copied me.  I wouldn't have missed it for so
>     long if you had.  Since I couldn't find that patch, I'm responding in
>     this thread instead.
> 
>     The r14-6838 patch is actually very very broken.  Disabling strub on a
>     target is not a matter of changing only the testsuite.  Your additions
>     to the tests even broke the strub-unsupported testcases, that tested
>     exactly the feature that enables ports to disable strub in a way that
>     informs users in case they attempt to use it.
> 
>     I'd thus like to revert that patch.
> 
>     Kewen's patch needs a little additional cleanup, that I'm preparing now,
>     to restore fully-functioning strub on sparc32.
> 
>     Please let me know in case you observe any other problems related with
>     strub.  I'd be happy to fix them, but I can only do so once I'm aware of
>     them.
> 
>     In case the reversal or the upcoming cleanup has any negative impact,
>     please make sure you let me know.
> 
>     Thanks,
> 
>     Happy GNU Year!
> 
>     -- 
>     Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/ <https://FSFLA.org/blogs/lxo/>
>        Free Software Activist                   GNU Toolchain Engineer
>     More tolerance and less prejudice are key for inclusion and diversity
>     Excluding neuro-others for not behaving ""normal"" is *not* inclusive
> 

Re: [PATCH] strub: Only unbias stack point for SPARC_STACK_BOUNDARY_HACK [PR113100]

[Alexandre Oliva]

On Jan 18, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:

> Not sure if I missed something in the testing, could you
> kindly double check if those test cases started to fail from r14-6275 on your
> env?

My guess is that they started to fail when David attempted to bypass the
strub tests by changing the dg proc that detects strub support.  The
tests then detected the mismatch between the result of the proc and the
expected errors when strub is disabled properly.


Here's the cleanup patch I promised.  Sorry it took so long, I was
hitting bootstrap errors even on a pristine tree yesterday.

Regstrapped on x86_64-linux-gnu.  This patch restores the status on
sparc-sun-solaris from before Kewen's patch, while leaving all other
ports unchanged.  Ok to install?


strub: introduce STACK_ADDRESS_OFFSET

Since STACK_POINTER_OFFSET is not necessarily at the boundary between
caller- and callee-owned stack, as desired by
__builtin_stack_address(), and using it as if it were or not causes
problems, introduce a new macro so that ports can define it suitably,
without modifying STACK_POINTER_OFFSET.


for  gcc/ChangeLog

	PR middle-end/112917
	PR middle-end/113100
	* builtins.cc (expand_builtin_stack_address): Use
	STACK_ADDRESS_OFFSET.
	* doc/extend.texi (__builtin_stack_address): Adjust.
	* config/sparc/sparc.h (STACK_ADDRESS_OFFSET): Define.
	* doc/tm.texi.in (STACK_ADDRESS_OFFSET): Document.
	* doc/tm.texi.in: Rebuilt.
---
 gcc/builtins.cc          |    5 ++---
 gcc/config/sparc/sparc.h |    7 +++++++
 gcc/doc/extend.texi      |    2 +-
 gcc/doc/tm.texi          |   29 +++++++++++++++++++++++++++++
 gcc/doc/tm.texi.in       |   29 +++++++++++++++++++++++++++++
 5 files changed, 68 insertions(+), 4 deletions(-)

diff --git a/gcc/builtins.cc b/gcc/builtins.cc
index 09f2354f1144b..37df7dcda0a0e 100644
--- a/gcc/builtins.cc
+++ b/gcc/builtins.cc
@@ -5450,7 +5450,7 @@ expand_builtin_stack_address ()
   rtx ret = convert_to_mode (ptr_mode, copy_to_reg (stack_pointer_rtx),
 			     STACK_UNSIGNED);
 
-#ifdef SPARC_STACK_BOUNDARY_HACK
+#ifdef STACK_ADDRESS_OFFSET
   /* Unbias the stack pointer, bringing it to the boundary between the
      stack area claimed by the active function calling this builtin,
      and stack ranges that could get clobbered if it called another
@@ -5477,8 +5477,7 @@ expand_builtin_stack_address ()
      (caller) function's active area as well, whereas those pushed or
      allocated temporarily for a call are regarded as part of the
      callee's stack range, rather than the caller's.  */
-  if (SPARC_STACK_BOUNDARY_HACK)
-    ret = plus_constant (ptr_mode, ret, STACK_POINTER_OFFSET);
+  ret = plus_constant (ptr_mode, ret, STACK_ADDRESS_OFFSET);
 #endif
 
   return force_reg (ptr_mode, ret);
diff --git a/gcc/config/sparc/sparc.h b/gcc/config/sparc/sparc.h
index fc064a92c22d9..fb074808d30d4 100644
--- a/gcc/config/sparc/sparc.h
+++ b/gcc/config/sparc/sparc.h
@@ -734,6 +734,13 @@ along with GCC; see the file COPYING3.  If not see
      parameter regs.  */
 #define STACK_POINTER_OFFSET (FIRST_PARM_OFFSET(0) + SPARC_STACK_BIAS)
 
+/* Unbias the stack pointer if needed, and move past the register save area,
+   that is never in use while a function is active, so that it is regarded as a
+   callee save area rather than as part of the function's own stack area.  This
+   enables __strub_leave() to do a better job of clearing the stack frame of a
+   previously-called sibling.  */
+#define STACK_ADDRESS_OFFSET STACK_POINTER_OFFSET
+
 /* Base register for access to local variables of the function.  */
 #define HARD_FRAME_POINTER_REGNUM 30
 
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 0bc586d120e76..00d8aa390cc5e 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -12791,7 +12791,7 @@ situations.
 
 @deftypefn {Built-in Function} {void *} __builtin_stack_address ()
 This function returns the stack pointer register, offset by
-@code{STACK_POINTER_OFFSET}.
+@code{STACK_ADDRESS_OFFSET} if that's defined.
 
 Conceptually, the returned address returned by this built-in function is
 the boundary between the stack area allocated for use by its caller, and
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 69ae63c77de6e..c8b8b126b2424 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -3456,6 +3456,35 @@ or type, otherwise return false.  The default implementation always returns
 true.
 @end deftypefn
 
+@defmac STACK_ADDRESS_OFFSET
+Offset from the stack pointer register to the boundary address between
+the stack area claimed by an active function, and stack ranges that
+could get clobbered if it called another function.  It should NOT
+encompass any stack red zone, that is used in leaf functions.
+
+This value is added to the stack pointer register to compute the address
+returned by @code{__builtin_stack_address}, and this is its only use.
+If this macro is not defined, no offset is added.  Defining it like
+@code{STACK_POINTER_OFFSET} may be appropriate for many machines, but
+not all.
+
+On SPARC, for example, the register save area is *not* considered active
+or used by the active function, but rather as akin to the area in which
+call-preserved registers are saved by callees, so the stack address is
+above that area, even though the (unbiased) stack pointer points below
+it.  This enables @code{__strub_leave} to clear what would otherwise
+overlap with its own register save area.
+
+On PowerPC, @code{STACK_POINTER_OFFSET} also reserves space for a save
+area, but that area is used by the caller rather than the callee, so the
+boundary address is below it.
+
+If the address is computed too high or too low, parts of a stack range
+that should be scrubbed may be left unscrubbed, scrubbing may corrupt
+active portions of the stack frame, and stack ranges may be
+doubly-scrubbed by caller and callee.
+@end defmac
+
 @defmac TARGET_STRUB_USE_DYNAMIC_ARRAY
 If defined to nonzero, @code{__strub_leave} will allocate a dynamic
 array covering the stack range that needs scrubbing before clearing it.
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 21343d4d1bf2f..658e1e63371e5 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -2688,6 +2688,35 @@ may reduce the size of debug information on some ports.
 
 @hook TARGET_HAVE_STRUB_SUPPORT_FOR
 
+@defmac STACK_ADDRESS_OFFSET
+Offset from the stack pointer register to the boundary address between
+the stack area claimed by an active function, and stack ranges that
+could get clobbered if it called another function.  It should NOT
+encompass any stack red zone, that is used in leaf functions.
+
+This value is added to the stack pointer register to compute the address
+returned by @code{__builtin_stack_address}, and this is its only use.
+If this macro is not defined, no offset is added.  Defining it like
+@code{STACK_POINTER_OFFSET} may be appropriate for many machines, but
+not all.
+
+On SPARC, for example, the register save area is *not* considered active
+or used by the active function, but rather as akin to the area in which
+call-preserved registers are saved by callees, so the stack address is
+above that area, even though the (unbiased) stack pointer points below
+it.  This enables @code{__strub_leave} to clear what would otherwise
+overlap with its own register save area.
+
+On PowerPC, @code{STACK_POINTER_OFFSET} also reserves space for a save
+area, but that area is used by the caller rather than the callee, so the
+boundary address is below it.
+
+If the address is computed too high or too low, parts of a stack range
+that should be scrubbed may be left unscrubbed, scrubbing may corrupt
+active portions of the stack frame, and stack ranges may be
+doubly-scrubbed by caller and callee.
+@end defmac
+
 @defmac TARGET_STRUB_USE_DYNAMIC_ARRAY
 If defined to nonzero, @code{__strub_leave} will allocate a dynamic
 array covering the stack range that needs scrubbing before clearing it.


-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH] strub: Only unbias stack point for SPARC_STACK_BOUNDARY_HACK [PR113100]

[Alexandre Oliva]

On Jan 19, 2024, Alexandre Oliva <oliva@adacore.com> wrote:

> On Jan 18, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:
>> Not sure if I missed something in the testing, could you
>> kindly double check if those test cases started to fail from r14-6275 on your
>> env?

> My guess is that they started to fail when David attempted to bypass the
> strub tests by changing the dg proc that detects strub support.  The
> tests then detected the mismatch between the result of the proc and the
> expected errors when strub is disabled properly.

The patch below, that realigns stack scrubbing on sparc32 and sparc64,
is still pending review (Ping?), but since your fix for ppc (that
worsened scrubbing on sparc32) has long been in, I'm pushing the
reversal of commit 9773ca519864e2e0706424b805c3314f1fbe7d10.


> strub: introduce STACK_ADDRESS_OFFSET

> Since STACK_POINTER_OFFSET is not necessarily at the boundary between
> caller- and callee-owned stack, as desired by
> __builtin_stack_address(), and using it as if it were or not causes
> problems, introduce a new macro so that ports can define it suitably,
> without modifying STACK_POINTER_OFFSET.


> for  gcc/ChangeLog

> 	PR middle-end/112917
> 	PR middle-end/113100
> 	* builtins.cc (expand_builtin_stack_address): Use
> 	STACK_ADDRESS_OFFSET.
> 	* doc/extend.texi (__builtin_stack_address): Adjust.
> 	* config/sparc/sparc.h (STACK_ADDRESS_OFFSET): Define.
> 	* doc/tm.texi.in (STACK_ADDRESS_OFFSET): Document.
> 	* doc/tm.texi.in: Rebuilt.

Ping?
https://gcc.gnu.org/pipermail/gcc-patches/2024-January/643408.html

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH] strub: Only unbias stack point for SPARC_STACK_BOUNDARY_HACK [PR113100]

[Richard Biener]

On Tue, Jan 30, 2024 at 4:35 AM Alexandre Oliva <oliva@adacore.com> wrote:
>
> On Jan 19, 2024, Alexandre Oliva <oliva@adacore.com> wrote:
>
> > On Jan 18, 2024, "Kewen.Lin" <linkw@linux.ibm.com> wrote:
> >> Not sure if I missed something in the testing, could you
> >> kindly double check if those test cases started to fail from r14-6275 on your
> >> env?
>
> > My guess is that they started to fail when David attempted to bypass the
> > strub tests by changing the dg proc that detects strub support.  The
> > tests then detected the mismatch between the result of the proc and the
> > expected errors when strub is disabled properly.
>
> The patch below, that realigns stack scrubbing on sparc32 and sparc64,
> is still pending review (Ping?), but since your fix for ppc (that
> worsened scrubbing on sparc32) has long been in, I'm pushing the
> reversal of commit 9773ca519864e2e0706424b805c3314f1fbe7d10.
>
>
> > strub: introduce STACK_ADDRESS_OFFSET
>
> > Since STACK_POINTER_OFFSET is not necessarily at the boundary between
> > caller- and callee-owned stack, as desired by
> > __builtin_stack_address(), and using it as if it were or not causes
> > problems, introduce a new macro so that ports can define it suitably,
> > without modifying STACK_POINTER_OFFSET.
>
>
> > for  gcc/ChangeLog
>
> >       PR middle-end/112917
> >       PR middle-end/113100
> >       * builtins.cc (expand_builtin_stack_address): Use
> >       STACK_ADDRESS_OFFSET.
> >       * doc/extend.texi (__builtin_stack_address): Adjust.
> >       * config/sparc/sparc.h (STACK_ADDRESS_OFFSET): Define.
> >       * doc/tm.texi.in (STACK_ADDRESS_OFFSET): Document.
> >       * doc/tm.texi.in: Rebuilt.
>
> Ping?
> https://gcc.gnu.org/pipermail/gcc-patches/2024-January/643408.html

OK

> --
> Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
>    Free Software Activist                   GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive