fortran: Fix a pasto in gfc_check_dependency
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Build passed
|
Commit Message
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.
In any case, bootstrapped/regtested on x86_64-linux and i686-linux
successfully, ok for trunk?
2024-08-01 Jakub Jelinek <jakub@redhat.com>
* dependency.cc (gfc_check_dependency): Fix a pasto, check
sym1->as->type for AS_ASSUMED_SHAPE rather than sym2->as->type.
Formatting fix.
Jakub
Comments
Hello,
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?
This looks like dead code to me.
> In any case, bootstrapped/regtested on x86_64-linux and i686-linux
> successfully, ok for trunk?
>
> 2024-08-01 Jakub Jelinek <jakub@redhat.com>
>
> * 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.jj 2024-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
>
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 <jakub@redhat.com>
> >
> > * 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.jj 2024-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
[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
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?
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
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?
On Fri, Aug 02, 2024 at 06:29:27PM +0200, Mikael Morin wrote:
> 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?
> They can alias, and they do alias. So in the end, writing either line is
> equivalent, what do I miss?
So, I had another look. Seems the reason why the testcase passes is that
gfc_could_be_alias (called from gfc_conv_resolve_dependencies) returns true,
so the assignment goes through a temporary array.
gfc_check_dependency is then only called for
if (lhs->rank > 0 && gfc_check_dependency (lhs, rhs, true) == 0)
optimize_binop_array_assignment (c, &rhs, false);
Guess the question is if one can construct a testcase where it would make a
difference.
Jakub
Le 05/08/2024 à 10:59, Jakub Jelinek a écrit :
> On Fri, Aug 02, 2024 at 06:29:27PM +0200, Mikael Morin wrote:
>> 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?
>
>> They can alias, and they do alias. So in the end, writing either line is
>> equivalent, what do I miss?
>
> So, I had another look. Seems the reason why the testcase passes is that
> gfc_could_be_alias (called from gfc_conv_resolve_dependencies) returns true,
> so the assignment goes through a temporary array.
> gfc_check_dependency is then only called for
> if (lhs->rank > 0 && gfc_check_dependency (lhs, rhs, true) == 0)
> optimize_binop_array_assignment (c, &rhs, false);
>
gfc_check_dependency is also used for WHERE and FORALL.
> Guess the question is if one can construct a testcase where it would make a
> difference.
>
The one I made up for PR116196 is a candidate, or can serve as base; I
think it's valid, but a bit twisted.
@@ -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
&& ((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))))