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
>
