diff mbox series

[v1] rs6000: Stackoverflow in optimized code on PPC [PR100799]

Message ID 3ff6a834-2d45-4f8d-8746-99fad5a73e60@linux.ibm.com
State New
Headers show
Series [v1] rs6000: Stackoverflow in optimized code on PPC [PR100799] | expand

Commit Message

Ajit Agarwal March 22, 2024, 9:25 a.m. UTC 
Hello Jakub:

When using FlexiBLAS with OpenBLAS we noticed corruption of
the parameters passed to OpenBLAS functions. FlexiBLAS
basically provides a BLAS interface where each function
is a stub that forwards the arguments to a real BLAS lib,
like OpenBLAS.

Fixes the corruption of caller frame checking number of
arguments is less than equal to GP_ARG_NUM_REG (8)
excluding hidden unused DECLS.

Bootstrapped and regtested on powerpc64-linux-gnu.

Thanks & Regards
Ajit


rs6000: Stackoverflow in optimized code on PPC [PR100799]

When using FlexiBLAS with OpenBLAS we noticed corruption of
the parameters passed to OpenBLAS functions. FlexiBLAS
basically provides a BLAS interface where each function
is a stub that forwards the arguments to a real BLAS lib,
like OpenBLAS.

Fixes the corruption of caller frame checking number of
arguments is less than equal to GP_ARG_NUM_REG (8)
excluding hidden unused DECLS.

2024-03-22  Ajit Kumar Agarwal  <aagarwa1@linux.ibm.com>

gcc/ChangeLog:

	PR rtk-optimization/100799
	* config/rs6000/rs6000-calls.cc (rs6000_function_arg): Don't
	generate parameter save area if number of arguments passed
	less than equal to GP_ARG_NUM_REG (8) excluding hidden
	parameter.
	(init_cumulative_args): Check for hidden parameter in fortran
	routine and set the flag hidden_string_length and actual
	parameter passed excluding hidden unused DECLS.
	* config/rs6000/rs6000.h (rs6000_args): Add new field
	hidden_string_length and actual_parm_length.
---
 gcc/config/rs6000/rs6000-call.cc | 40 ++++++++++++++++++++++++++++++--
 gcc/config/rs6000/rs6000.h       |  8 +++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

Comments

Jakub Jelinek March 22, 2024, 9:36 a.m. UTC | #1 
On Fri, Mar 22, 2024 at 02:55:43PM +0530, Ajit Agarwal wrote:
> rs6000: Stackoverflow in optimized code on PPC [PR100799]
> 
> When using FlexiBLAS with OpenBLAS we noticed corruption of
> the parameters passed to OpenBLAS functions. FlexiBLAS
> basically provides a BLAS interface where each function
> is a stub that forwards the arguments to a real BLAS lib,
> like OpenBLAS.
> 
> Fixes the corruption of caller frame checking number of
> arguments is less than equal to GP_ARG_NUM_REG (8)
> excluding hidden unused DECLS.

Looks mostly good to me except some comment nits, but I'll defer
the actual ack to the rs6000 maintainers.

> +  /* Workaround buggy C/C++ wrappers around Fortran routines with
> +     character(len=constant) arguments if the hidden string length arguments
> +     are passed on the stack; if the callers forget to pass those arguments,
> +     attempting to tail call in such routines leads to stack corruption.

I thought it isn't just tail calls, even normal calls.
When the buggy C/C++ wrappers call the function with fewer arguments
than it actually has and doesn't expect the parameter save area on the
caller side because of that while the callee expects it and the callee
actually stores something in the parameter save area, it corrupts whatever
is in the caller stack frame at that location.

> +     Avoid return stack space for parameters <= 8 excluding hidden string
> +     length argument is passed (partially or fully) on the stack in the
> +     caller and the callee needs to pass any arguments on the stack.  */
> +  unsigned int num_args = 0;
> +  unsigned int hidden_length = 0;
> +
> +  for (tree arg = DECL_ARGUMENTS (current_function_decl);
> +       arg; arg = DECL_CHAIN (arg))
> +    {
> +      num_args++;
> +      if (DECL_HIDDEN_STRING_LENGTH (arg))
> +	{
> +	  tree parmdef = ssa_default_def (cfun, arg);
> +	  if (parmdef == NULL || has_zero_uses (parmdef))
> +	    {
> +	      cum->hidden_string_length = 1;
> +	      hidden_length++;
> +	    }
> +	}
> +   }
> +
> +  cum->actual_parm_length = num_args - hidden_length;
> +
>    /* Check for a longcall attribute.  */
>    if ((!fntype && rs6000_default_long_calls)
>        || (fntype
> @@ -1857,7 +1884,16 @@ rs6000_function_arg (cumulative_args_t cum_v, const function_arg_info &arg)
>  
>  	  return rs6000_finish_function_arg (mode, rvec, k);
>  	}
> -      else if (align_words < GP_ARG_NUM_REG)
> +     /* Workaround buggy C/C++ wrappers around Fortran routines with
> +	character(len=constant) arguments if the hidden string length arguments
> +	are passed on the stack; if the callers forget to pass those arguments,
> +	attempting to tail call in such routines leads to stack corruption.
> +	Avoid return stack space for parameters <= 8 excluding hidden string
> +	length argument is passed (partially or fully) on the stack in the
> +	caller and the callee needs to pass any arguments on the stack.  */
> +      else if (align_words < GP_ARG_NUM_REG
> +	       || (cum->hidden_string_length
> +	       && cum->actual_parm_length <= GP_ARG_NUM_REG))
>  	{
>  	  if (TARGET_32BIT && TARGET_POWERPC64)
>  	    return rs6000_mixed_function_arg (mode, type, align_words);
> diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
> index 68bc45d65ba..a1d3ed00b14 100644
> --- a/gcc/config/rs6000/rs6000.h
> +++ b/gcc/config/rs6000/rs6000.h
> @@ -1490,6 +1490,14 @@ typedef struct rs6000_args
>    int named;			/* false for varargs params */
>    int escapes;			/* if function visible outside tu */
>    int libcall;			/* If this is a compiler generated call.  */
> +  /* Actual parameter length ignoring hidden paramter.

s/paramter/parameter/

> +     This is done to C++ wrapper calling fortran module
> +     which has hidden parameter that are not used.  */
> +  unsigned int actual_parm_length;
> +  /* Hidden parameters while calling C++ wrapper to fortran
> +     module. Set if there is hidden parameter in fortran
> +     module while called C++ wrapper.  */

modules in Fortran are something completely different.
You should IMHO talk about procedures instead of modules
in both of the above comments (multiple times even).

> +  unsigned int hidden_string_length : 1;
>  } CUMULATIVE_ARGS;
>  
>  /* Initialize a variable CUM of type CUMULATIVE_ARGS
> -- 
> 2.39.3

	Jakub
Ajit Agarwal March 22, 2024, 10:16 a.m. UTC | #2 
Hello Jakub:

Thanks for review. Addressed below review comments and sent
version 2 of the patch for review.

Thanks & Regards
Ajit

On 22/03/24 3:06 pm, Jakub Jelinek wrote:
> On Fri, Mar 22, 2024 at 02:55:43PM +0530, Ajit Agarwal wrote:
>> rs6000: Stackoverflow in optimized code on PPC [PR100799]
>>
>> When using FlexiBLAS with OpenBLAS we noticed corruption of
>> the parameters passed to OpenBLAS functions. FlexiBLAS
>> basically provides a BLAS interface where each function
>> is a stub that forwards the arguments to a real BLAS lib,
>> like OpenBLAS.
>>
>> Fixes the corruption of caller frame checking number of
>> arguments is less than equal to GP_ARG_NUM_REG (8)
>> excluding hidden unused DECLS.
> 
> Looks mostly good to me except some comment nits, but I'll defer
> the actual ack to the rs6000 maintainers.
> 
>> +  /* Workaround buggy C/C++ wrappers around Fortran routines with
>> +     character(len=constant) arguments if the hidden string length arguments
>> +     are passed on the stack; if the callers forget to pass those arguments,
>> +     attempting to tail call in such routines leads to stack corruption.
> 
> I thought it isn't just tail calls, even normal calls.
> When the buggy C/C++ wrappers call the function with fewer arguments
> than it actually has and doesn't expect the parameter save area on the
> caller side because of that while the callee expects it and the callee
> actually stores something in the parameter save area, it corrupts whatever
> is in the caller stack frame at that location.
> 
>> +     Avoid return stack space for parameters <= 8 excluding hidden string
>> +     length argument is passed (partially or fully) on the stack in the
>> +     caller and the callee needs to pass any arguments on the stack.  */
>> +  unsigned int num_args = 0;
>> +  unsigned int hidden_length = 0;
>> +
>> +  for (tree arg = DECL_ARGUMENTS (current_function_decl);
>> +       arg; arg = DECL_CHAIN (arg))
>> +    {
>> +      num_args++;
>> +      if (DECL_HIDDEN_STRING_LENGTH (arg))
>> +	{
>> +	  tree parmdef = ssa_default_def (cfun, arg);
>> +	  if (parmdef == NULL || has_zero_uses (parmdef))
>> +	    {
>> +	      cum->hidden_string_length = 1;
>> +	      hidden_length++;
>> +	    }
>> +	}
>> +   }
>> +
>> +  cum->actual_parm_length = num_args - hidden_length;
>> +
>>    /* Check for a longcall attribute.  */
>>    if ((!fntype && rs6000_default_long_calls)
>>        || (fntype
>> @@ -1857,7 +1884,16 @@ rs6000_function_arg (cumulative_args_t cum_v, const function_arg_info &arg)
>>  
>>  	  return rs6000_finish_function_arg (mode, rvec, k);
>>  	}
>> -      else if (align_words < GP_ARG_NUM_REG)
>> +     /* Workaround buggy C/C++ wrappers around Fortran routines with
>> +	character(len=constant) arguments if the hidden string length arguments
>> +	are passed on the stack; if the callers forget to pass those arguments,
>> +	attempting to tail call in such routines leads to stack corruption.
>> +	Avoid return stack space for parameters <= 8 excluding hidden string
>> +	length argument is passed (partially or fully) on the stack in the
>> +	caller and the callee needs to pass any arguments on the stack.  */
>> +      else if (align_words < GP_ARG_NUM_REG
>> +	       || (cum->hidden_string_length
>> +	       && cum->actual_parm_length <= GP_ARG_NUM_REG))
>>  	{
>>  	  if (TARGET_32BIT && TARGET_POWERPC64)
>>  	    return rs6000_mixed_function_arg (mode, type, align_words);
>> diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
>> index 68bc45d65ba..a1d3ed00b14 100644
>> --- a/gcc/config/rs6000/rs6000.h
>> +++ b/gcc/config/rs6000/rs6000.h
>> @@ -1490,6 +1490,14 @@ typedef struct rs6000_args
>>    int named;			/* false for varargs params */
>>    int escapes;			/* if function visible outside tu */
>>    int libcall;			/* If this is a compiler generated call.  */
>> +  /* Actual parameter length ignoring hidden paramter.
> 
> s/paramter/parameter/
> 
>> +     This is done to C++ wrapper calling fortran module
>> +     which has hidden parameter that are not used.  */
>> +  unsigned int actual_parm_length;
>> +  /* Hidden parameters while calling C++ wrapper to fortran
>> +     module. Set if there is hidden parameter in fortran
>> +     module while called C++ wrapper.  */
> 
> modules in Fortran are something completely different.
> You should IMHO talk about procedures instead of modules
> in both of the above comments (multiple times even).
> 
>> +  unsigned int hidden_string_length : 1;
>>  } CUMULATIVE_ARGS;
>>  
>>  /* Initialize a variable CUM of type CUMULATIVE_ARGS
>> -- 
>> 2.39.3
> 
> 	Jakub
>
Kewen.Lin April 2, 2024, 5:45 a.m. UTC | #3 
Hi!

on 2024/3/22 17:36, Jakub Jelinek wrote:
> On Fri, Mar 22, 2024 at 02:55:43PM +0530, Ajit Agarwal wrote:
>> rs6000: Stackoverflow in optimized code on PPC [PR100799]
>>
>> When using FlexiBLAS with OpenBLAS we noticed corruption of
>> the parameters passed to OpenBLAS functions. FlexiBLAS
>> basically provides a BLAS interface where each function
>> is a stub that forwards the arguments to a real BLAS lib,
>> like OpenBLAS.
>>
>> Fixes the corruption of caller frame checking number of
>> arguments is less than equal to GP_ARG_NUM_REG (8)
>> excluding hidden unused DECLS.
> 
> Looks mostly good to me except some comment nits, but I'll defer
> the actual ack to the rs6000 maintainers.
> 
>> +  /* Workaround buggy C/C++ wrappers around Fortran routines with
>> +     character(len=constant) arguments if the hidden string length arguments
>> +     are passed on the stack; if the callers forget to pass those arguments,
>> +     attempting to tail call in such routines leads to stack corruption.
> 
> I thought it isn't just tail calls, even normal calls.
> When the buggy C/C++ wrappers call the function with fewer arguments
> than it actually has and doesn't expect the parameter save area on the
> caller side because of that while the callee expects it and the callee
> actually stores something in the parameter save area, it corrupts whatever
> is in the caller stack frame at that location.

I agree it's not just tail calls, but currently DECL_HIDDEN_STRING_LENGTH
setting is guarded with flag_tail_call_workaround, which was intended to
be only for tail calls.  So I wonder if we should update this option name,
or introduce another option which is more for C/Fortran interoperability
workaround, set DECL_HIDDEN_STRING_LENGTH with this guard and also enable
this existing flag_tail_call_workaround.

> 
>> +     Avoid return stack space for parameters <= 8 excluding hidden string
>> +     length argument is passed (partially or fully) on the stack in the
>> +     caller and the callee needs to pass any arguments on the stack.  */
>> +  unsigned int num_args = 0;
>> +  unsigned int hidden_length = 0;
>> +
>> +  for (tree arg = DECL_ARGUMENTS (current_function_decl);
>> +       arg; arg = DECL_CHAIN (arg))
>> +    {
>> +      num_args++;
>> +      if (DECL_HIDDEN_STRING_LENGTH (arg))
>> +	{
>> +	  tree parmdef = ssa_default_def (cfun, arg);
>> +	  if (parmdef == NULL || has_zero_uses (parmdef))
>> +	    {
>> +	      cum->hidden_string_length = 1;
>> +	      hidden_length++;
>> +	    }
>> +	}

As Fortran allows to have some string with unknown length, it's possible to
have some test cases which have mixed used and unused hidden lengths, since
the used ones matter, users may already modify their C code to prepare the
required used hidden length.  But with this change, the modified code could
not work any more.  For example, 7th and 8th are unused but 9th argument is
used, 9th is passed by caller on stack but callee expects it comes from r9
instead (7th arg).  So IMHO we should be more conservative and only make
this workaround by taking care of the continuous unused hidden length at the
end of arg list.  Someone may argue if users already know how to modify their
C code to interoperate with Fortran, we should already modify all their C code
and won't adopt this workaround, but if this restriction still works for all
the motivated test cases, IMHO keeping more conservative is good, as users
could only update some "broken" cases not "all".

BR,
Kewen


>> +   }
>> +
>> +  cum->actual_parm_length = num_args - hidden_length;
>> +
>>    /* Check for a longcall attribute.  */
>>    if ((!fntype && rs6000_default_long_calls)
>>        || (fntype
>> @@ -1857,7 +1884,16 @@ rs6000_function_arg (cumulative_args_t cum_v, const function_arg_info &arg)
>>  
>>  	  return rs6000_finish_function_arg (mode, rvec, k);
>>  	}
>> -      else if (align_words < GP_ARG_NUM_REG)
>> +     /* Workaround buggy C/C++ wrappers around Fortran routines with
>> +	character(len=constant) arguments if the hidden string length arguments
>> +	are passed on the stack; if the callers forget to pass those arguments,
>> +	attempting to tail call in such routines leads to stack corruption.
>> +	Avoid return stack space for parameters <= 8 excluding hidden string
>> +	length argument is passed (partially or fully) on the stack in the
>> +	caller and the callee needs to pass any arguments on the stack.  */
>> +      else if (align_words < GP_ARG_NUM_REG
>> +	       || (cum->hidden_string_length
>> +	       && cum->actual_parm_length <= GP_ARG_NUM_REG))
>>  	{
>>  	  if (TARGET_32BIT && TARGET_POWERPC64)
>>  	    return rs6000_mixed_function_arg (mode, type, align_words);
>> diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
>> index 68bc45d65ba..a1d3ed00b14 100644
>> --- a/gcc/config/rs6000/rs6000.h
>> +++ b/gcc/config/rs6000/rs6000.h
>> @@ -1490,6 +1490,14 @@ typedef struct rs6000_args
>>    int named;			/* false for varargs params */
>>    int escapes;			/* if function visible outside tu */
>>    int libcall;			/* If this is a compiler generated call.  */
>> +  /* Actual parameter length ignoring hidden paramter.
> 
> s/paramter/parameter/
> 
>> +     This is done to C++ wrapper calling fortran module
>> +     which has hidden parameter that are not used.  */
>> +  unsigned int actual_parm_length;
>> +  /* Hidden parameters while calling C++ wrapper to fortran
>> +     module. Set if there is hidden parameter in fortran
>> +     module while called C++ wrapper.  */
> 
> modules in Fortran are something completely different.
> You should IMHO talk about procedures instead of modules
> in both of the above comments (multiple times even).
> 
>> +  unsigned int hidden_string_length : 1;
>>  } CUMULATIVE_ARGS;
>>  
>>  /* Initialize a variable CUM of type CUMULATIVE_ARGS
>> -- 
>> 2.39.3
> 
> 	Jakub
>
diff mbox series

Patch

diff --git a/gcc/config/rs6000/rs6000-call.cc b/gcc/config/rs6000/rs6000-call.cc
index 1f8f93a2ee7..2620ce16943 100644
--- a/gcc/config/rs6000/rs6000-call.cc
+++ b/gcc/config/rs6000/rs6000-call.cc
@@ -64,7 +64,7 @@ 
 #include "ppc-auxv.h"
 #include "targhooks.h"
 #include "opts.h"
-
+#include "tree-dfa.h"
 #include "rs6000-internal.h"
 
 #ifndef TARGET_PROFILE_KERNEL
@@ -584,6 +584,33 @@  init_cumulative_args (CUMULATIVE_ARGS *cum, tree fntype,
   if (incoming || cum->prototype)
     cum->nargs_prototype = n_named_args;
 
+  /* Workaround buggy C/C++ wrappers around Fortran routines with
+     character(len=constant) arguments if the hidden string length arguments
+     are passed on the stack; if the callers forget to pass those arguments,
+     attempting to tail call in such routines leads to stack corruption.
+     Avoid return stack space for parameters <= 8 excluding hidden string
+     length argument is passed (partially or fully) on the stack in the
+     caller and the callee needs to pass any arguments on the stack.  */
+  unsigned int num_args = 0;
+  unsigned int hidden_length = 0;
+
+  for (tree arg = DECL_ARGUMENTS (current_function_decl);
+       arg; arg = DECL_CHAIN (arg))
+    {
+      num_args++;
+      if (DECL_HIDDEN_STRING_LENGTH (arg))
+	{
+	  tree parmdef = ssa_default_def (cfun, arg);
+	  if (parmdef == NULL || has_zero_uses (parmdef))
+	    {
+	      cum->hidden_string_length = 1;
+	      hidden_length++;
+	    }
+	}
+   }
+
+  cum->actual_parm_length = num_args - hidden_length;
+
   /* Check for a longcall attribute.  */
   if ((!fntype && rs6000_default_long_calls)
       || (fntype
@@ -1857,7 +1884,16 @@  rs6000_function_arg (cumulative_args_t cum_v, const function_arg_info &arg)
 
 	  return rs6000_finish_function_arg (mode, rvec, k);
 	}
-      else if (align_words < GP_ARG_NUM_REG)
+     /* Workaround buggy C/C++ wrappers around Fortran routines with
+	character(len=constant) arguments if the hidden string length arguments
+	are passed on the stack; if the callers forget to pass those arguments,
+	attempting to tail call in such routines leads to stack corruption.
+	Avoid return stack space for parameters <= 8 excluding hidden string
+	length argument is passed (partially or fully) on the stack in the
+	caller and the callee needs to pass any arguments on the stack.  */
+      else if (align_words < GP_ARG_NUM_REG
+	       || (cum->hidden_string_length
+	       && cum->actual_parm_length <= GP_ARG_NUM_REG))
 	{
 	  if (TARGET_32BIT && TARGET_POWERPC64)
 	    return rs6000_mixed_function_arg (mode, type, align_words);
diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
index 68bc45d65ba..a1d3ed00b14 100644
--- a/gcc/config/rs6000/rs6000.h
+++ b/gcc/config/rs6000/rs6000.h
@@ -1490,6 +1490,14 @@  typedef struct rs6000_args
   int named;			/* false for varargs params */
   int escapes;			/* if function visible outside tu */
   int libcall;			/* If this is a compiler generated call.  */
+  /* Actual parameter length ignoring hidden paramter.
+     This is done to C++ wrapper calling fortran module
+     which has hidden parameter that are not used.  */
+  unsigned int actual_parm_length;
+  /* Hidden parameters while calling C++ wrapper to fortran
+     module. Set if there is hidden parameter in fortran
+     module while called C++ wrapper.  */
+  unsigned int hidden_string_length : 1;
 } CUMULATIVE_ARGS;
 
 /* Initialize a variable CUM of type CUMULATIVE_ARGS