[PATCH #1/2] strub: sparc: omit frame in strub_leave [PR112917]

[PATCH #1/2] strub: sparc: omit frame in strub_leave [PR112917]

[Alexandre Oliva]

If we allow __strub_leave to allocate a frame on sparc, it will
overlap with a lot of the stack range we're supposed to scrub, because
of the large fixed-size outgoing args and register save area.
Unfortunately, setting up the PIC register seems to prevent the frame
pointer from being omitted.

Since the strub runtime doesn't issue calls or use global variables,
at least on sparc, disabling PIC to compile strub.c seems to do the
right thing.

Regstrapped on x86_64-linux-gnu, also testing on sparc-solaris2.11.3.
(but it will likely take forever on the cfarm machine; if someone with
faster sparc machines could give this a spin and confirm, that would be
appreciated.) Ok to install?

IIRC this patch gets 32-bit sparc to pass all strub tests, but sparc64
still fails many of them; there's another one for sparc64 that fixes
them, and that will improve sparc -m32 as well.


for  libgcc/ChangeLog

	PR middle-end/112917
	* config.host (sparc, sparc64): Enable...
	* config/sparc/t-sparc: ... this new fragment.
---
 libgcc/config.host          |    2 ++
 libgcc/config/sparc/t-sparc |    4 ++++
 2 files changed, 6 insertions(+)
 create mode 100644 libgcc/config/sparc/t-sparc

diff --git a/libgcc/config.host b/libgcc/config.host
index 694e3e9f54cad..54d06978a5d2c 100644
--- a/libgcc/config.host
+++ b/libgcc/config.host
@@ -199,9 +199,11 @@ riscv*-*-*)
 	;;
 sparc64*-*-*)
 	cpu_type=sparc
+	tmake_file="${tmake_file} sparc/t-sparc"
 	;;
 sparc*-*-*)
 	cpu_type=sparc
+	tmake_file="${tmake_file} sparc/t-sparc"
 	;;
 s390*-*-*)
 	cpu_type=s390
diff --git a/libgcc/config/sparc/t-sparc b/libgcc/config/sparc/t-sparc
new file mode 100644
index 0000000000000..fb1bf1fc29cc4
--- /dev/null
+++ b/libgcc/config/sparc/t-sparc
@@ -0,0 +1,4 @@
+# This is needed for __strub_leave to omit the frame pointer, without
+# which it will allocate a register save area on the stack and leave
+# it unscrubbed and most likely unused, because it's a leaf function.
+CFLAGS-strub.c += -fno-PIC -fomit-frame-pointer

-- 
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

[PATCH #2/2] strub: sparc64: unbias the stack address [PR112917]

[Alexandre Oliva]

The stack pointer is biased by 2047 bytes on sparc64, so the range it
delimits is way off.  Unbias the addresses returned by
__builtin_stack_address (), so that the strub builtins, inlined or
not, can function correctly.  I've considered introducing a new target
macro, but using STACK_POINTER_OFFSET seems safe, and it enables the
register save areas to be scrubbed as well.

Because of the large fixed-size outgoing args area next to the
register save area on sparc, we still need __strub_leave to not
allocate its own frame, otherwise it won't be able to clear part of
the frame it should.

Regstrapped on x86_64-linux-gnu, also testing on sparc-solaris2.11.3.
Ok to install?



for  gcc/ChangeLog

	PR middle-end/112917
	* builtins.cc (expand_bultin_stack_address): Add
	STACK_POINTER_OFFSET.
---
 gcc/builtins.cc |   34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/gcc/builtins.cc b/gcc/builtins.cc
index 7c2732ab79e6f..4c8c514fe8618 100644
--- a/gcc/builtins.cc
+++ b/gcc/builtins.cc
@@ -5443,8 +5443,38 @@ expand_builtin_frame_address (tree fndecl, tree exp)
 static rtx
 expand_builtin_stack_address ()
 {
-  return convert_to_mode (ptr_mode, copy_to_reg (stack_pointer_rtx),
-			  STACK_UNSIGNED);
+  rtx ret = convert_to_mode (ptr_mode, copy_to_reg (stack_pointer_rtx),
+			     STACK_UNSIGNED);
+
+  /* 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
+     function.  It should NOT encompass any stack red zone, that is
+     used in leaf functions.
+
+     On SPARC, 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.  This
+     enables __strub_leave to clear what would otherwise overlap with
+     its own register save area.
+
+     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.
+
+     In order for it to be just right, the area delimited by
+     @code{__builtin_stack_address} and @code{__builtin_frame_address
+     (0)} should encompass caller's registers saved by the function,
+     local on-stack variables and @code{alloca} stack areas.
+     Accumulated outgoing on-stack arguments, preallocated as part of
+     a function's own prologue, are to be regarded as part of the
+     (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);
+
+  return force_reg (ptr_mode, ret);
 }
 
 /* Expand a call to builtin function __builtin_strub_enter.  */

-- 
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 #2/2] strub: sparc64: unbias the stack address [PR112917]

[Richard Biener]

On Thu, Dec 14, 2023 at 10:29 PM Alexandre Oliva <oliva@adacore.com> wrote:
>
>
> The stack pointer is biased by 2047 bytes on sparc64, so the range it
> delimits is way off.  Unbias the addresses returned by
> __builtin_stack_address (), so that the strub builtins, inlined or
> not, can function correctly.  I've considered introducing a new target
> macro, but using STACK_POINTER_OFFSET seems safe, and it enables the
> register save areas to be scrubbed as well.
>
> Because of the large fixed-size outgoing args area next to the
> register save area on sparc, we still need __strub_leave to not
> allocate its own frame, otherwise it won't be able to clear part of
> the frame it should.
>
> Regstrapped on x86_64-linux-gnu, also testing on sparc-solaris2.11.3.
> Ok to install?

It might be worth amending the documentation in case this
is unexpected to users?

>
>
> for  gcc/ChangeLog
>
>         PR middle-end/112917
>         * builtins.cc (expand_bultin_stack_address): Add
>         STACK_POINTER_OFFSET.
> ---
>  gcc/builtins.cc |   34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/builtins.cc b/gcc/builtins.cc
> index 7c2732ab79e6f..4c8c514fe8618 100644
> --- a/gcc/builtins.cc
> +++ b/gcc/builtins.cc
> @@ -5443,8 +5443,38 @@ expand_builtin_frame_address (tree fndecl, tree exp)
>  static rtx
>  expand_builtin_stack_address ()
>  {
> -  return convert_to_mode (ptr_mode, copy_to_reg (stack_pointer_rtx),
> -                         STACK_UNSIGNED);
> +  rtx ret = convert_to_mode (ptr_mode, copy_to_reg (stack_pointer_rtx),
> +                            STACK_UNSIGNED);
> +
> +  /* 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
> +     function.  It should NOT encompass any stack red zone, that is
> +     used in leaf functions.
> +
> +     On SPARC, 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.  This
> +     enables __strub_leave to clear what would otherwise overlap with
> +     its own register save area.
> +
> +     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.
> +
> +     In order for it to be just right, the area delimited by
> +     @code{__builtin_stack_address} and @code{__builtin_frame_address
> +     (0)} should encompass caller's registers saved by the function,
> +     local on-stack variables and @code{alloca} stack areas.
> +     Accumulated outgoing on-stack arguments, preallocated as part of
> +     a function's own prologue, are to be regarded as part of the
> +     (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);
> +
> +  return force_reg (ptr_mode, ret);
>  }
>
>  /* Expand a call to builtin function __builtin_strub_enter.  */
>
> --
> 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

[PATCH #2v2/2] strub: sparc64: unbias the stack address [PR112917]

[Alexandre Oliva]

On Dec 15, 2023, Richard Biener <richard.guenther@gmail.com> wrote:

> It might be worth amending the documentation in case this
> is unexpected to users?

Oh, yes indeed, thanks!

Here's a patch that brings relevant parts of the implementation comment
to the user-facing documentation, so that it reflects the change in
implementation.

Regstrapped on x86_64-linux-gnu.  Ok to install?


strub: sparc64: unbias the stack address [PR112917]

The stack pointer is biased by 2047 bytes on sparc64, so the range it
delimits is way off.  Unbias the addresses returned by
__builtin_stack_address (), so that the strub builtins, inlined or
not, can function correctly.  I've considered introducing a new target
macro, but using STACK_POINTER_OFFSET seems safe, and it enables the
register save areas to be scrubbed as well.

Because of the large fixed-size outgoing args area next to the
register save area on sparc, we still need __strub_leave to not
allocate its own frame, otherwise it won't be able to clear part of
the frame it should.


for  gcc/ChangeLog

	PR middle-end/112917
	* builtins.cc (expand_bultin_stack_address): Add
	STACK_POINTER_OFFSET.
	* doc/extend.texi (__builtin_stack_address): Adjust.
---
 gcc/builtins.cc     |   34 ++++++++++++++++++++++++++++++++--
 gcc/doc/extend.texi |   23 ++++++++++++++++++++++-
 2 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/gcc/builtins.cc b/gcc/builtins.cc
index 7c2732ab79e6f..4c8c514fe8618 100644
--- a/gcc/builtins.cc
+++ b/gcc/builtins.cc
@@ -5443,8 +5443,38 @@ expand_builtin_frame_address (tree fndecl, tree exp)
 static rtx
 expand_builtin_stack_address ()
 {
-  return convert_to_mode (ptr_mode, copy_to_reg (stack_pointer_rtx),
-			  STACK_UNSIGNED);
+  rtx ret = convert_to_mode (ptr_mode, copy_to_reg (stack_pointer_rtx),
+			     STACK_UNSIGNED);
+
+  /* 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
+     function.  It should NOT encompass any stack red zone, that is
+     used in leaf functions.
+
+     On SPARC, 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.  This
+     enables __strub_leave to clear what would otherwise overlap with
+     its own register save area.
+
+     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.
+
+     In order for it to be just right, the area delimited by
+     @code{__builtin_stack_address} and @code{__builtin_frame_address
+     (0)} should encompass caller's registers saved by the function,
+     local on-stack variables and @code{alloca} stack areas.
+     Accumulated outgoing on-stack arguments, preallocated as part of
+     a function's own prologue, are to be regarded as part of the
+     (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);
+
+  return force_reg (ptr_mode, ret);
 }
 
 /* Expand a call to builtin function __builtin_strub_enter.  */
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index b585e2d810230..5ac6a820e2a03 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -12706,7 +12706,28 @@ situations.
 @enddefbuiltin
 
 @deftypefn {Built-in Function} {void *} __builtin_stack_address ()
-This function returns the value of the stack pointer register.
+This function returns the stack pointer register, offset by
+@code{STACK_POINTER_OFFSET}.
+
+Conceptually, the returned address returned by this built-in function is
+the boundary between the stack area allocated for use by its caller, and
+the area that could be modified by a function call, that the caller
+could safely zero-out before or after (but not during) the call
+sequence.
+
+Arguments for a callee may be preallocated as part of the caller's stack
+frame, or allocated on a per-call basis, depending on the target, so
+they may be on either side of this boundary.
+
+Even if the stack pointer is biased, the result is not.  The register
+save area on SPARC is regarded as modifiable by calls, rather than as
+allocated for use by the caller function, since it is never in use while
+the caller function itself is running.
+
+Red zones that only leaf functions could use are also regarded as
+modifiable by calls, rather than as allocated for use by the caller.
+This is only theoretical, since leaf functions do not issue calls, but a
+constant offset makes this built-in function more predictable.
 @end deftypefn
 
 @node Stack Scrubbing


-- 
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 #2v2/2] strub: sparc64: unbias the stack address [PR112917]

[Richard Biener]

On Wed, Dec 20, 2023 at 12:51 AM Alexandre Oliva <oliva@adacore.com> wrote:
>
> On Dec 15, 2023, Richard Biener <richard.guenther@gmail.com> wrote:
>
> > It might be worth amending the documentation in case this
> > is unexpected to users?
>
> Oh, yes indeed, thanks!
>
> Here's a patch that brings relevant parts of the implementation comment
> to the user-facing documentation, so that it reflects the change in
> implementation.
>
> Regstrapped on x86_64-linux-gnu.  Ok to install?

OK.

>
> strub: sparc64: unbias the stack address [PR112917]
>
> The stack pointer is biased by 2047 bytes on sparc64, so the range it
> delimits is way off.  Unbias the addresses returned by
> __builtin_stack_address (), so that the strub builtins, inlined or
> not, can function correctly.  I've considered introducing a new target
> macro, but using STACK_POINTER_OFFSET seems safe, and it enables the
> register save areas to be scrubbed as well.
>
> Because of the large fixed-size outgoing args area next to the
> register save area on sparc, we still need __strub_leave to not
> allocate its own frame, otherwise it won't be able to clear part of
> the frame it should.
>
>
> for  gcc/ChangeLog
>
>         PR middle-end/112917
>         * builtins.cc (expand_bultin_stack_address): Add
>         STACK_POINTER_OFFSET.
>         * doc/extend.texi (__builtin_stack_address): Adjust.
> ---
>  gcc/builtins.cc     |   34 ++++++++++++++++++++++++++++++++--
>  gcc/doc/extend.texi |   23 ++++++++++++++++++++++-
>  2 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/builtins.cc b/gcc/builtins.cc
> index 7c2732ab79e6f..4c8c514fe8618 100644
> --- a/gcc/builtins.cc
> +++ b/gcc/builtins.cc
> @@ -5443,8 +5443,38 @@ expand_builtin_frame_address (tree fndecl, tree exp)
>  static rtx
>  expand_builtin_stack_address ()
>  {
> -  return convert_to_mode (ptr_mode, copy_to_reg (stack_pointer_rtx),
> -                         STACK_UNSIGNED);
> +  rtx ret = convert_to_mode (ptr_mode, copy_to_reg (stack_pointer_rtx),
> +                            STACK_UNSIGNED);
> +
> +  /* 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
> +     function.  It should NOT encompass any stack red zone, that is
> +     used in leaf functions.
> +
> +     On SPARC, 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.  This
> +     enables __strub_leave to clear what would otherwise overlap with
> +     its own register save area.
> +
> +     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.
> +
> +     In order for it to be just right, the area delimited by
> +     @code{__builtin_stack_address} and @code{__builtin_frame_address
> +     (0)} should encompass caller's registers saved by the function,
> +     local on-stack variables and @code{alloca} stack areas.
> +     Accumulated outgoing on-stack arguments, preallocated as part of
> +     a function's own prologue, are to be regarded as part of the
> +     (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);
> +
> +  return force_reg (ptr_mode, ret);
>  }
>
>  /* Expand a call to builtin function __builtin_strub_enter.  */
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index b585e2d810230..5ac6a820e2a03 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -12706,7 +12706,28 @@ situations.
>  @enddefbuiltin
>
>  @deftypefn {Built-in Function} {void *} __builtin_stack_address ()
> -This function returns the value of the stack pointer register.
> +This function returns the stack pointer register, offset by
> +@code{STACK_POINTER_OFFSET}.
> +
> +Conceptually, the returned address returned by this built-in function is
> +the boundary between the stack area allocated for use by its caller, and
> +the area that could be modified by a function call, that the caller
> +could safely zero-out before or after (but not during) the call
> +sequence.
> +
> +Arguments for a callee may be preallocated as part of the caller's stack
> +frame, or allocated on a per-call basis, depending on the target, so
> +they may be on either side of this boundary.
> +
> +Even if the stack pointer is biased, the result is not.  The register
> +save area on SPARC is regarded as modifiable by calls, rather than as
> +allocated for use by the caller function, since it is never in use while
> +the caller function itself is running.
> +
> +Red zones that only leaf functions could use are also regarded as
> +modifiable by calls, rather than as allocated for use by the caller.
> +This is only theoretical, since leaf functions do not issue calls, but a
> +constant offset makes this built-in function more predictable.
>  @end deftypefn
>
>  @node Stack Scrubbing
>
>
> --
> 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