diff mbox

[bootstrap-O3,fortran] silence warning in simplify_transformation_to_array

Message ID orpok43e5a.fsf@lxoliva.fsfla.org
State New
Headers show

Commit Message

Alexandre Oliva Jan. 3, 2017, 5:29 a.m. UTC 
simplify_transformation_to_array had the nested loop unrolled 7 times,
which is reasonable given that it iterates over arrays of size
GFC_MAX_DIMENSIONS == 7.

The problem is that the last iteration increments the index, tests
that it's less than result->rank, and then accesses the arrays with
the incremented index.

We did not optimize out that part in the 7th iteration, so VRP flagged
the unreachable code as accessing arrays past the end.

It couldn't possibly know that we'd never reach that part, since the
test was on result->rank, and it's not obvious (for the compiler) that
result->rank <= GFC_MAX_DIMENSIONS.

Even an assert to that effect before the enclosing loop didn't avoid
the warning turned to error, though; I suppose there might be some
aliasing at play, because moving the assert into the loop does, but
then, it's not as efficient as testing the index itself against the
limit.

Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  OK to install?

for  gcc/fortran/ChangeLog

	* simplify.c (simplify_transformation_to_array): Assert the
	array access is in range.  Fix whitespace.
---
 gcc/fortran/simplify.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jeff Law Jan. 3, 2017, 6:19 p.m. UTC | #1 
On 01/02/2017 10:29 PM, Alexandre Oliva wrote:
> simplify_transformation_to_array had the nested loop unrolled 7 times,
> which is reasonable given that it iterates over arrays of size
> GFC_MAX_DIMENSIONS == 7.
>
> The problem is that the last iteration increments the index, tests
> that it's less than result->rank, and then accesses the arrays with
> the incremented index.
>
> We did not optimize out that part in the 7th iteration, so VRP flagged
> the unreachable code as accessing arrays past the end.
>
> It couldn't possibly know that we'd never reach that part, since the
> test was on result->rank, and it's not obvious (for the compiler) that
> result->rank <= GFC_MAX_DIMENSIONS.
>
> Even an assert to that effect before the enclosing loop didn't avoid
> the warning turned to error, though; I suppose there might be some
> aliasing at play, because moving the assert into the loop does, but
> then, it's not as efficient as testing the index itself against the
> limit.
>
> Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  OK to install?
>
> for  gcc/fortran/ChangeLog
>
> 	* simplify.c (simplify_transformation_to_array): Assert the
> 	array access is in range.  Fix whitespace.
OK.
jeff
Richard Biener Jan. 4, 2017, 12:13 p.m. UTC | #2 
On Tue, Jan 3, 2017 at 7:19 PM, Jeff Law <law@redhat.com> wrote:
> On 01/02/2017 10:29 PM, Alexandre Oliva wrote:
>>
>> simplify_transformation_to_array had the nested loop unrolled 7 times,
>> which is reasonable given that it iterates over arrays of size
>> GFC_MAX_DIMENSIONS == 7.
>>
>> The problem is that the last iteration increments the index, tests
>> that it's less than result->rank, and then accesses the arrays with
>> the incremented index.
>>
>> We did not optimize out that part in the 7th iteration, so VRP flagged
>> the unreachable code as accessing arrays past the end.
>>
>> It couldn't possibly know that we'd never reach that part, since the
>> test was on result->rank, and it's not obvious (for the compiler) that
>> result->rank <= GFC_MAX_DIMENSIONS.
>>
>> Even an assert to that effect before the enclosing loop didn't avoid
>> the warning turned to error, though; I suppose there might be some
>> aliasing at play, because moving the assert into the loop does, but
>> then, it's not as efficient as testing the index itself against the
>> limit.
>>
>> Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  OK to install?
>>
>> for  gcc/fortran/ChangeLog
>>
>>         * simplify.c (simplify_transformation_to_array): Assert the
>>         array access is in range.  Fix whitespace.

But once we default to release checking it will warn again?  That said, I think
this kind of workaround is bad :/

Richard.

> OK.
> jeff
>
Alexandre Oliva Jan. 4, 2017, 1:23 p.m. UTC | #3 
On Jan  4, 2017, Richard Biener <richard.guenther@gmail.com> wrote:

> On Tue, Jan 3, 2017 at 7:19 PM, Jeff Law <law@redhat.com> wrote:
>> On 01/02/2017 10:29 PM, Alexandre Oliva wrote:
>>> * simplify.c (simplify_transformation_to_array): Assert the
>>> array access is in range.  Fix whitespace.

> But once we default to release checking it will warn again?

I guess it will, if one builds with -O3 in that setting.

> That said, I think this kind of workaround is bad :/

I can't say I'm proud of it ;-)

The assert is probably stands on its own: it's reasonable, for
documentation purposes, to state we're not accessing the arrays past
their end, because other parts of the code ensure this is the case.

Now, it's a bit fortunate and unfortunate that adding the assert *also*
silences the warning, because then we add to the patch analysis not just
whether the assert is desirable on its own, but whether the consequence
of silencing the warning in this particular way is desirable.

I figured the assert was still desirable, but if that's not the
consensus, I won't mind dropping the proposed change.  Unfortunately I
don't have another that silences the warning without (or even with)
impact on codegen.
Richard Biener Jan. 4, 2017, 2:11 p.m. UTC | #4 
On Wed, Jan 4, 2017 at 2:23 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Jan  4, 2017, Richard Biener <richard.guenther@gmail.com> wrote:
>
>> On Tue, Jan 3, 2017 at 7:19 PM, Jeff Law <law@redhat.com> wrote:
>>> On 01/02/2017 10:29 PM, Alexandre Oliva wrote:
>>>> * simplify.c (simplify_transformation_to_array): Assert the
>>>> array access is in range.  Fix whitespace.
>
>> But once we default to release checking it will warn again?
>
> I guess it will, if one builds with -O3 in that setting.
>
>> That said, I think this kind of workaround is bad :/
>
> I can't say I'm proud of it ;-)
>
> The assert is probably stands on its own: it's reasonable, for
> documentation purposes, to state we're not accessing the arrays past
> their end, because other parts of the code ensure this is the case.
>
> Now, it's a bit fortunate and unfortunate that adding the assert *also*
> silences the warning, because then we add to the patch analysis not just
> whether the assert is desirable on its own, but whether the consequence
> of silencing the warning in this particular way is desirable.
>
> I figured the assert was still desirable, but if that's not the
> consensus, I won't mind dropping the proposed change.  Unfortunately I
> don't have another that silences the warning without (or even with)
> impact on codegen.

#pragma GCC diagnostic push ("Wno-array-bounds")

#pragma GCC diagnostic pop

? (well, fix the syntax for me please ;))

Richard.

>
> --
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Alexandre Oliva Jan. 4, 2017, 9:20 p.m. UTC | #5 
On Jan  4, 2017, Richard Biener <richard.guenther@gmail.com> wrote:

>> Unfortunately I don't have another that silences the warning without
>> (or even with) impact on codegen.

> #pragma GCC diagnostic push ("Wno-array-bounds")

> #pragma GCC diagnostic pop

Wwwhaaat?  #pragma GCC diagnostic to silence warnings?  Who'd have
thunk? :-D

*blush*


> ? (well, fix the syntax for me please ;))

Sure can do, but then...  Unlike the assert, that might legitimately
have been put there, this will pollute the source code for the sake of a
non-default build configuration that doesn't usually deserve.  I'll
prepare and post a patch anyway, but do we want to make it standard
practice?
diff mbox

Patch

diff --git a/gcc/fortran/simplify.c b/gcc/fortran/simplify.c
index a46fbc5..1036681 100644
--- a/gcc/fortran/simplify.c
+++ b/gcc/fortran/simplify.c
@@ -610,7 +610,8 @@  simplify_transformation_to_array (gfc_expr *result, gfc_expr *array, gfc_expr *d
 	  n++;
 	  if (n < result->rank)
 	    {
-	      count [n]++;
+	      gcc_checking_assert (n < GFC_MAX_DIMENSIONS);
+	      count[n]++;
 	      base += sstride[n];
 	      dest += dstride[n];
 	    }