On 27 July 2024 21:11:19 CEST, Mikael Morin <morin-mik...@orange.fr> wrote:
>Le 27/07/2024 à 19:23, rep.dot....@gmail.com a écrit :
>> On 22 July 2024 20:53:18 CEST, Mikael Morin <morin-mik...@orange.fr> wrote:
>>> From: Mikael Morin <mik...@gcc.gnu.org>
>>>
>>> Hello,
>>>
>>> this fixes a null pointer dereference with absent optional dummy passed
>>> as BACK argument of MINLOC/MAXLOC.
>>>
>>> Tested for regression on x86_64-linux.
>>> OK for master?
>>>
>>> -- >8 --
>>>
>>> Protect the evaluation of BACK with a check that the reference is non-null
>>> in case the expression is an optional dummy, in the inline code generated
>>> for MINLOC and MAXLOC.
>>>
>>> This change contains a revert of the non-testsuite part of commit
>>> r15-1994-ga55d24b3cf7f4d07492bb8e6fcee557175b47ea3, which factored the
>>> evaluation of BACK out of the loop using the scalarizer. It was a bad idea,
>>> because delegating the argument evaluation to the scalarizer makes it
>>> cumbersome to add a null pointer check next to the evaluation.
>>>
>>> Instead, evaluate BACK at the beginning, before scalarization, add a check
>>> that the argument is present if necessary, and evaluate the resulting
>>> expression to a variable, before using the variable in the inline code.
>>>
>>> gcc/fortran/ChangeLog:
>>>
>>> * trans-intrinsic.cc (maybe_absent_optional_variable): New function.
>>> (gfc_conv_intrinsic_minmaxloc): Remove BACK from scalarization and
>>> evaluate it before. Add a check that BACK is not null if the
>>> expression is an optional dummy. Save the resulting expression to a
>>> variable. Use the variable in the generated inline code.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> * gfortran.dg/maxloc_6.f90: New test.
>>> * gfortran.dg/minloc_7.f90: New test.
>>> ---
>>> gcc/fortran/trans-intrinsic.cc | 81 +++++-
>>> gcc/testsuite/gfortran.dg/maxloc_6.f90 | 366 +++++++++++++++++++++++++
>>> gcc/testsuite/gfortran.dg/minloc_7.f90 | 366 +++++++++++++++++++++++++
>>> 3 files changed, 799 insertions(+), 14 deletions(-)
>>> create mode 100644 gcc/testsuite/gfortran.dg/maxloc_6.f90
>>> create mode 100644 gcc/testsuite/gfortran.dg/minloc_7.f90
>>>
>>> diff --git a/gcc/fortran/trans-intrinsic.cc b/gcc/fortran/trans-intrinsic.cc
>>> index 180d0d7a88c..9f3c3ce47bc 100644
>>> --- a/gcc/fortran/trans-intrinsic.cc
>>> +++ b/gcc/fortran/trans-intrinsic.cc
>>> @@ -5209,6 +5209,50 @@ gfc_conv_intrinsic_dot_product (gfc_se * se,
>>> gfc_expr * expr)
>>> }
>>>
>>>
>>> +/* Tells whether the expression E is a reference to an optional variable
>>> whose
>>> + presence is not known at compile time. Those are variable references
>>> without
>>> + subreference; if there is a subreference, we can assume the variable is
>>> + present. We have to special case full arrays, which we represent with
>>> a fake
>>> + "full" reference, and class descriptors for which a reference to data
>>> is not
>>> + really a subreference. */
>>> +
>>> +bool
>>> +maybe_absent_optional_variable (gfc_expr *e)
>>> +{
>>> + if (!(e && e->expr_type == EXPR_VARIABLE))
>>> + return false;
>>
>> []
>>
>>> +}
>>> +
>>> +
>>> /* Remove unneeded kind= argument from actual argument list when the
>>> result conversion is dealt with in a different place. */
>>>
>>> @@ -5321,11 +5365,11 @@ gfc_conv_intrinsic_minmaxloc (gfc_se * se, gfc_expr
>>> * expr, enum tree_code op)
>>> tree nonempty;
>>> tree lab1, lab2;
>>> tree b_if, b_else;
>>> + tree back;
>>> gfc_loopinfo loop;
>>> gfc_actual_arglist *actual;
>>> gfc_ss *arrayss;
>>> gfc_ss *maskss;
>>> - gfc_ss *backss;
>>> gfc_se arrayse;
>>> gfc_se maskse;
>>> gfc_expr *arrayexpr;
>>> @@ -5391,10 +5435,27 @@ gfc_conv_intrinsic_minmaxloc (gfc_se * se, gfc_expr
>>> * expr, enum tree_code op)
>>> && maskexpr->symtree->n.sym->attr.dummy
>>> && maskexpr->symtree->n.sym->attr.optional;
>>> backexpr = actual->next->next->expr;
>>> - if (backexpr)
>>> - backss = gfc_get_scalar_ss (gfc_ss_terminator, backexpr);
>>> +
>>> + gfc_init_se (&backse, NULL);
>>> + if (backexpr == nullptr)
>>> + back = logical_false_node;
>>> + else if (maybe_absent_optional_variable (backexpr))
>>> + {
>>
>> /* if this fires, some wonky race is going on.. */
>>
>>> + gcc_assert (backexpr->expr_type == EXPR_VARIABLE);
>>
>> drop it, downgrade to checking, or is it worth?
>>
>Whether it is worth it, I don't know; it's protecting the access to
>backexpr->symtree a few lines down, idependently of the implementation of
>maybe_absent_optional_variable.
nod
>
>I expect the compiler to optimize it away,
ye[l]*p
> so I can surely make it a checking-only assert.
let's keep the assert unless someone opts for any downgrade within 72h ?
maybe add the comment if an assert persists, though. LGTM otherwise AFAIC
