Re: [PATCH] fortran: Fix a pasto in gfc_check_dependency
On Thu, Aug 01, 2024 at 09:03:39PM +0200, Mikael Morin wrote:
> Le 01/08/2024 à 12:00, Jakub Jelinek a écrit :
> > Hi!
> >
> > A static analyzer found what seems like a pasto in the PR45019 changes,
> > the second branch of || only accesses sym2 while the first one sym1 except
> > for this one spot.
> >
> > Not sure I'm able to construct a testcase for this though.
> >
> What is reported exactly by the static analyzer?
I'm not sure I'm allowed to cite it, it is some proprietary static analyzer
and I got that indirectly. But if I paraphrase it, the diagnostics was
a possible pasto. The static analyzer likely doesn't see that
sym1->attr.target implies attr1.target and sym2->attr.target implies
attr2.target.
> This looks like dead code to me.
Seems it wasn't originally dead when it was added in PR45019, but then the
PR62278 change made it dead.
But the function actually returns 0 rather than 1 that PR45019 meant.
So I bet in addition to fixing the pasto we should move that conditional
from where it is right now to the return 0; location after
check_data_pointer_types calls.
What I wonder is why the testcase doesn't actually fail.
And the pasto fix would guess fix
aliasing_dummy_5.f90 with
arg(2:3) = arr(1:2)
instead of
arr(2:3) = arg(1:2)
if the original testcase would actually fail.
> > In any case, bootstrapped/regtested on x86_64-linux and i686-linux
> > successfully, ok for trunk?
> >
> > 2024-08-01 Jakub Jelinek
> >
> > * dependency.cc (gfc_check_dependency): Fix a pasto, check
> > sym1->as->type for AS_ASSUMED_SHAPE rather than sym2->as->type.
> > Formatting fix.
> >
> > --- gcc/fortran/dependency.cc.jj2024-07-01 11:28:22.705237968 +0200
> > +++ gcc/fortran/dependency.cc 2024-07-31 20:53:34.471588828 +0200
> > @@ -1368,7 +1368,7 @@ gfc_check_dependency (gfc_expr *expr1, g
> > if ((attr1.pointer || attr1.target) && (attr2.pointer ||
> > attr2.target))
> > {
> > if (check_data_pointer_types (expr1, expr2)
> > - && check_data_pointer_types (expr2, expr1))
> > + && check_data_pointer_types (expr2, expr1))
> > return 0;
> > return 1;
> > @@ -1380,7 +1380,7 @@ gfc_check_dependency (gfc_expr *expr1, g
> > if (sym1->attr.target && sym2->attr.target
>
> if both target attributes are true here, both attr1.target and attr2.target
> are true as well, so the conditional before is true and this branch is
> skipped.
>
> > && ((sym1->attr.dummy && !sym1->attr.contiguous
> >&& (!sym1->attr.dimension
> > - || sym2->as->type == AS_ASSUMED_SHAPE))
> > + || sym1->as->type == AS_ASSUMED_SHAPE))
> > || (sym2->attr.dummy && !sym2->attr.contiguous
> > && (!sym2->attr.dimension
> > || sym2->as->type == AS_ASSUMED_SHAPE
Jakub
Re: [PATCH] fortran: Fix a pasto in gfc_check_dependency
[static analyzer] Jakub Jelinek wrote: […] it is some proprietary static analyzer I want to point out that a under utilized static analyzer keeps scanning GCC: Coverity Scan. If someone has the time, I think it would be worthwhile to have a look at the reports. There are a bunch of persons having access to it – and more can be added (I think I can grant access). Thus, is someone of the GCC developers has interest + time … Tobias
Re: [PATCH] fortran: Support optional dummy as BACK argument of MINLOC/MAXLOC.
Le 01/08/2024 à 21:02, Thomas Koenig a écrit :
Hi Mikael,
+ 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.
I expect the compiler to optimize it away, so I can surely make it a
checking-only assert.
I would also lean towards checking only.
OK with that change (or, if you really prefer, as submitted is also
fine).
Thanks for the patch! It's good to see so much progress...
Best regards
Thomas
Thanks to you and Bernhard.
This is what I'm going to push.From 40122a405386a8b67c11bbaad523ffce5c1c7855 Mon Sep 17 00:00:00 2001
From: Mikael Morin
Date: Fri, 2 Aug 2024 14:24:34 +0200
Subject: [PATCH] fortran: Support optional dummy as BACK argument of
MINLOC/MAXLOC.
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 | 83 +-
gcc/testsuite/gfortran.dg/maxloc_6.f90 | 366 +
gcc/testsuite/gfortran.dg/minloc_7.f90 | 366 +
3 files changed, 801 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..150cb9ff963 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;
+
+ gfc_symbol *sym = e->symtree->n.sym;
+ if (!sym->attr.optional)
+return false;
+
+ gfc_ref *ref = e->ref;
+ if (ref == nullptr)
+return true;
+
+ if (ref->type == REF_ARRAY
+ && ref->u.ar.type == AR_FULL
+ && ref->next == nullptr)
+return true;
+
+ if (!(sym->ts.type == BT_CLASS
+ && ref->type == REF_COMPONENT
+ && ref->u.c.component == CLASS_DATA (sym)))
+return false;
+
+ gfc_ref *next_ref = ref->next;
+ if (next_ref == nullptr)
+return true;
+
+ if (next_ref->type == REF_ARRAY
+ && next_ref->u.ar.type == AR_FULL
+ && next_ref->next == nullptr)
+return true;
+
+ 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,29 @@ 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))
+{
+ /* This should have been checked already by
+ maybe_abse
Re: [PATCH] fortran: Fix a pasto in gfc_check_dependency
Le 02/08/2024 à 10:12, Jakub Jelinek a écrit : On Thu, Aug 01, 2024 at 09:03:39PM +0200, Mikael Morin wrote: Le 01/08/2024 à 12:00, Jakub Jelinek a écrit : Hi! A static analyzer found what seems like a pasto in the PR45019 changes, the second branch of || only accesses sym2 while the first one sym1 except for this one spot. Not sure I'm able to construct a testcase for this though. What is reported exactly by the static analyzer? I'm not sure I'm allowed to cite it, it is some proprietary static analyzer and I got that indirectly. But if I paraphrase it, the diagnostics was a possible pasto. The static analyzer likely doesn't see that sym1->attr.target implies attr1.target and sym2->attr.target implies attr2.target. This looks like dead code to me. Seems it wasn't originally dead when it was added in PR45019, but then the PR62278 change made it dead. nods But the function actually returns 0 rather than 1 that PR45019 meant. So I bet in addition to fixing the pasto we should move that conditional from where it is right now to the return 0; location after check_data_pointer_types calls. No, the function check_data_pointer_types returns true if there is no aliasing, according to its function comment, so after both calls everything is safe and returning anything but 0 is overly pessimistic. I'm inclined to just remove the dead code. By the way, looking for a testcase exercising gfc_check_dependency, I found an example showing check_data_pointer_types is not up to its promise. This is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116196 What I wonder is why the testcase doesn't actually fail. From https://gcc.gnu.org/bugzilla/show_bug.cgi?id=45019#c7 : I think the patch below looks fine, however, if I set a break point, the function "gfc_check_dependency" is never called for test program. So the dependency.c part of the patch is not exercised by the testcase. And the pasto fix would guess fix aliasing_dummy_5.f90 with arg(2:3) = arr(1:2) instead of arr(2:3) = arg(1:2) if the original testcase would actually fail. Mmh, aren't they both actually the same?
Re: [PATCH] fortran: Fix a pasto in gfc_check_dependency
On Fri, Aug 02, 2024 at 04:58:09PM +0200, Mikael Morin wrote: > > But the function actually returns 0 rather than 1 that PR45019 meant. > > So I bet in addition to fixing the pasto we should move that conditional > > from where it is right now to the return 0; location after > > check_data_pointer_types calls. > > > No, the function check_data_pointer_types returns true if there is no > aliasing, according to its function comment, so after both calls everything > is safe and returning anything but 0 is overly pessimistic. My (limited) understanding is that the original PR is about some corner case in the standard wording where in order to allow aliasing of TARGET vars with POINTER vars it also makes valid the weirdo stuff in the testcase. And so should return 1 which means that they possibly alias. > > And the pasto fix would guess fix > > aliasing_dummy_5.f90 with > > arg(2:3) = arr(1:2) > > instead of > > arr(2:3) = arg(1:2) > > if the original testcase would actually fail. > > > Mmh, aren't they both actually the same? It is just bad choice of variable/argument names, I also originally thought they are the same, but they aren't. arr is a variable in the parent routine which is passed to the arg dummy, both have TARGET attribute and one of them is assumed shape and so that "the dummy argument has the TARGET attribute, the dummy argument does not have IN- TENT (IN), the dummy argument is a scalar object or an assumed-shape array without the CONTIGUOUS attribute, and the actual argument is a target other than an array section with a vector subscript." means that "Action that affects the value of the entity or any subobject of it shall be taken only through the dummy argument" is not true and so one can doesn't need to access the object just through arg, but can access it as arr as well and those two can alias. Jakub
Re: [PATCH] fortran: Fix a pasto in gfc_check_dependency
Le 02/08/2024 à 17:05, Jakub Jelinek a écrit : On Fri, Aug 02, 2024 at 04:58:09PM +0200, Mikael Morin wrote: But the function actually returns 0 rather than 1 that PR45019 meant. So I bet in addition to fixing the pasto we should move that conditional from where it is right now to the return 0; location after check_data_pointer_types calls. No, the function check_data_pointer_types returns true if there is no aliasing, according to its function comment, so after both calls everything is safe and returning anything but 0 is overly pessimistic. My (limited) understanding is that the original PR is about some corner case in the standard wording where in order to allow aliasing of TARGET vars with POINTER vars it also makes valid the weirdo stuff in the testcase. And so should return 1 which means that they possibly alias. I agree with all of that. Sure keeping the condition around would be the safest. I'm just afraid of keeping code that would remain dead. And the pasto fix would guess fix aliasing_dummy_5.f90 with arg(2:3) = arr(1:2) instead of arr(2:3) = arg(1:2) if the original testcase would actually fail. Mmh, aren't they both actually the same? It is just bad choice of variable/argument names, I also originally thought they are the same, but they aren't. arr is a variable in the parent routine which is passed to the arg dummy, both have TARGET attribute and one of them is assumed shape and so that "the dummy argument has the TARGET attribute, the dummy argument does not have IN- TENT (IN), the dummy argument is a scalar object or an assumed-shape array without the CONTIGUOUS attribute, and the actual argument is a target other than an array section with a vector subscript." means that "Action that affects the value of the entity or any subobject of it shall be taken only through the dummy argument" is not true and so one can doesn't need to access the object just through arg, but can access it as arr as well and those two can alias. They can alias, and they do alias. So in the end, writing either line is equivalent, what do I miss?
[Patch, Fortran] PR104626 ICE in gfc_format_decoder, at fortran/error.cc:1071
Hi all,
Doing some catchup here. I plan to commit the following shortly. This is
one of Steve's patches posted on bugzilla.
I have created a new test case.
Regression tested on linux x86-64.
git show:
commit 4d4549937b789afe4037c2f8f80dfc2285504a1e (HEAD -> master)
Author: Steve Kargl
Date: Thu Aug 1 21:50:49 2024 -0700
Fortran: Fix ICE on invalid in gfc_format_decoder.
PR fortran/104626
gcc/fortran/ChangeLog:
* symbol.cc (gfc_add_save): Add checks for SAVE attribute
conflicts and duplicate SAVE attribute.
gcc/testsuite/ChangeLog:
* gfortran.dg/pr104626.f90: New test.
diff --git a/gcc/fortran/symbol.cc b/gcc/fortran/symbol.cc
index a8479b862e3..b5143d9f790 100644
--- a/gcc/fortran/symbol.cc
+++ b/gcc/fortran/symbol.cc
@@ -1307,9 +1307,8 @@ gfc_add_save (symbol_attribute *attr, save_state
s, const char *name,
if (s == SAVE_EXPLICIT && gfc_pure (NULL))
{
- gfc_error
- ("SAVE attribute at %L cannot be specified in a PURE procedure",
-where);
+ gfc_error ("SAVE attribute at %L cannot be specified in a PURE "
+"procedure", where);
return false;
}
@@ -1319,10 +1318,15 @@ gfc_add_save (symbol_attribute *attr, save_state
s, const char *name,
if (s == SAVE_EXPLICIT && attr->save == SAVE_EXPLICIT
&& (flag_automatic || pedantic))
{
- if (!gfc_notify_std (GFC_STD_LEGACY,
-"Duplicate SAVE attribute specified at %L",
-where))
+ if (!where)
+ {
+ gfc_error ("Duplicate SAVE attribute specified near %C");
return false;
+ }
+
+ if (!gfc_notify_std (GFC_STD_LEGACY, "Duplicate SAVE attribute "
+ "specified at %L", where))
+ return false;
}
attr->save = s;
diff --git a/gcc/testsuite/gfortran.dg/pr104626.f90
b/gcc/testsuite/gfortran.dg/pr104626.f90
new file mode 100644
index 000..faff65a8c92
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr104626.f90
@@ -0,0 +1,8 @@
+! { dg-do compile }
+program p
+ procedure(g), save :: f ! { dg-error "PROCEDURE attribute conflicts" }
+ procedure(g), save :: f ! { dg-error "Duplicate SAVE attribute" }
+contains
+ subroutine g
+ end
+end
