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

Message ID orfs04324k.fsf@lxoliva.fsfla.org
State Unresolved
Headers
Series [#1/2] strub: sparc: omit frame in strub_leave [PR112917] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Alexandre Oliva Dec. 14, 2023, 9:28 p.m. UTC
  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(-)
  

Comments

Richard Biener Dec. 15, 2023, 7:26 a.m. UTC | #1
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

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.  */