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?
>+
>+ gfc_conv_expr (&backse, backexpr);
>+ tree present = gfc_conv_expr_present (backexpr->symtree->n.sym, false);
>+ back = fold_build2_loc (input_location, TRUTH_ANDIF_EXPR,
>+ logical_type_node, present, backse.expr);
>+ }
> else
>- backss = nullptr;
>+ {
>+ gfc_conv_expr (&backse, backexpr);
>+ back = backse.expr;
>+ }
>+ gfc_add_block_to_block (&se->pre, &backse.pre);
>+ back = gfc_evaluate_now_loc (input_location, back, &se->pre);
>+ gfc_add_block_to_block (&se->pre, &backse.post);
>
> nonempty = NULL
[]
thanks
