Hello Peter:
Sent version-3 of the patch addressing below review comments.
Thanks & Regards
Ajit
On 23/03/24 3:03 pm, Ajit Agarwal wrote:
> Hello Peter:
>
> On 23/03/24 10:07 am, Peter Bergner wrote:
>> On 3/22/24 5:15 AM, Ajit Agarwal wrote:
>>> 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.
>>
>> I think the git log entry commentary could be a little more descriptive
>> of what the problem is. How about something like the following?
>>
>> When using FlexiBLAS with OpenBLAS, we noticed corruption of the caller
>> stack frame when calling OpenBLAS functions. This was caused by the
>> FlexiBLAS C/C++ caller and OpenBLAS Fortran callee disagreeing on the
>> number of function parameters in the callee due to hidden Fortran
>> parameters. This can cause problems when the callee believes the caller
>> has allocated a parameter save area when the caller has not done so.
>> That means any writes by the callee into the non-existent parameter save
>> area will corrupt the caller stack frame.
>>
>> The workaround implemented here, is for the callee to determine whether
>> the caller has allocated a parameter save area or not, by ignoring any
>> unused hidden parameters when counting the number of parameters.
>>
>>
> I will address this change in the new version of the patch.
>>
>>> PR rtk-optimization/100799
>>
>> s/rtk/rtl/
>>
>>
>>
> I will address this in new version of the patch.
>>> * 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.
>>
>> The callee doesn't generate or allocate the parameter save area, the
>> caller does. The code here is for the callee trying to determine
>> whether the caller has done so. How about saying the following instead?
>>
>> Don't assume a parameter save area has been allocated if the number of
>> formal parameters, excluding unused hidden parameters, is less than or
>> equal to GP_ARG_NUM_REG (8).
>>
>>
>
> I will incorporate this change in new version of the patch.
>
>>
>>
>>> (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.
>>
>> Check for unused hidden Fortran parameters and set hidden_string_length
>> and actual_parm_length.
>>
>>
>
> I will address this change in new version of the patch.
>
>
>>> + /* 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. */
>>
>> The wrapper/caller is the one that allocates the parameter save area, so
>> saying "...doesn't expect the parameter save area on the caller side..."
>> doesn't make sense, since it knows whether it allocated it or not.
>> How about saying something like the following instead?
>>
>> Check whether this function contains any unused hidden parameters and
>> record how many there are for use in rs6000_function_arg() to determine
>> whether its callers have allocated a parameter save area or not.
>> See PR100799 for details.
>>
>>
>
> I will incorporate this change in new version of the patch.
>
>>
>>> + 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;
>>
>> This code looks fine, but do we really need two new fields in rs6000_args?
>> Can't we just get along with only cum->actual_parm_length by modifying
>> the rs6000_function_arg() change from:
>>
>>> + else if (align_words < GP_ARG_NUM_REG
>>> + || (cum->hidden_string_length
>>> + && cum->actual_parm_length <= GP_ARG_NUM_REG))
>>
>> to:
>>
>> + else if (align_words < GP_ARG_NUM_REG
>> + || cum->actual_parm_length <= GP_ARG_NUM_REG)
>>
>> ???
>>
>
> Yes we can do that. I will address this in new version of the patch.
>
>
>> That said, I have a further comment below on what happens here when
>> align_words >= GP_ARG_NUM_REG and cum->actual_parm_length <= GP_ARG_NUM_REG.
>>
>>
>
> If we exceed the align_words >= GP_ARG_NUM_REG then there could be hidden
> unused DECL paramter in align_words which is greater than 8 then it will
> return NULL_RTX. Hence in the above condition it should not return NULL_RTX.
> For the above condition it will not return NULL_RTX instead it will return a
> rtx reg.
>
>>
>>
>>> + /* 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. */
>>
>> Same comment as before, so same problem with the comment, but the following
>> change...
>>
>>> - else if (align_words < GP_ARG_NUM_REG)
>>> + 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);
>>
>> return gen_rtx_REG (mode, GP_ARG_MIN_REG + align_words);
>> }
>> else
>> return NULL_RTX;
>>
>> The old code for the unused hidden parameter (which was the 9th param) would
>> fall thru to the "return NULL_RTX;" which would make the callee assume there
>> was a parameter save area allocated. Now instead, we'll return a reg rtx,
>> probably of r11 (r3 thru r10 are our param regs) and I'm guessing we'll now
>> see a copy of r11 into a pseudo like we do for the other param regs.
>> Is that a problem? Given it's an unused parameter, it'll probably get deleted
>> as dead code, but could it cause any issues? What if we have more than one
>> unused hidden parameter and we return r12 and r13 which have specific uses
>> in our ABIs (eg, r13 is our TCB pointer), so it may not actually look dead.
>> Have you verified what the callee RTL looks like after expand for these
>> unused hidden parameters? Is there a rtx we can return that isn't a NULL_RTX
>> which triggers the assumption of a parameter save area, but isn't a reg rtx
>> which might lead to some rtl being generated? Would a (const_int 0) or
>> something else work?
>>
>>
> For the above use case it will return
>
> (reg:DI 5 %r5) and below check entry_parm =
> (reg:DI 5 %r5) and the following check will not return TRUE and hence
> parameter save area will not be allocated.
>
> It will not generate any rtx in the callee rtl code but it just used to check
> whether to allocate parameter save area or not when number of args > 8.
>
> /* If there is no incoming register, we need a stack. */
> entry_parm = rs6000_function_arg (args_so_far, arg);
> if (entry_parm == NULL)
> return true;
>
> /* Likewise if we need to pass both in registers and on the stack. */
> if (GET_CODE (entry_parm) == PARALLEL
> && XEXP (XVECEXP (entry_parm, 0, 0), 0) == NULL_RTX)
> return true;
>
> Thanks & Regards
> Ajit
>>
>>> + /* Actual parameter length ignoring hidden parameter.
>>> + This is done to C++ wrapper calling fortran procedures
>>> + which has hidden parameter that are not used. */
>>
>> I think a simpler comment will suffice:
>>
>> /* Actual parameter count ignoring unused hidden parameters. */
>>
>>
>> Peter
>>
>>
>>
