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 >