* [patch, fortran] Fix PR 79956, part two
@ 2017-03-12 21:27 Thomas Koenig
2017-03-12 22:23 ` Jerry DeLisle
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Koenig @ 2017-03-12 21:27 UTC (permalink / raw)
To: fortran, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]
Hello world,
the attached patch fixes another occurence of PR 79956.
In this case, we did
sdim = GFC_DESCRIPTOR_RANK (source);
..
for (n = 0; n < sdim; n++)
{
sstride[n] = GFC_DESCRIPTOR_STRIDE(source,n)
}
Now, we know that sdim can never be zero or lower, but
the compiler doesn't, so it warned. (Why that didn't
happen on x86_64 is an unsolved mystery so far).
So, the attached patch inserts a call to internal_error
when sdim is lower than one.
Regression-tested. OK for trunk?
Thomas
2017-03-12 Thomas Koenig <tkoenig@gcc.gnu.org>
PR libfortran/79956
* m4/reshape.m4 (reshape_'rtype_ccode`): Add check for sdim < 1 so
that later assignment from sstride[0] does not cause problems.
* intrinsic/reshape_generic.c (reshape_internal): Likweise.
* generated/reshape_c10.c: Regenerated.
* generated/reshape_c16.c: Regenerated.
* generated/reshape_c4.c: Regenerated.
* generated/reshape_c8.c: Regenerated.
* generated/reshape_i16.c: Regenerated.
* generated/reshape_i4.c: Regenerated.
* generated/reshape_i8.c: Regenerated.
* generated/reshape_r10.c: Regenerated.
* generated/reshape_r16.c: Regenerated.
* generated/reshape_r4.c: Regenerated.
* generated/reshape_r8.c: Regenerated.
[-- Attachment #2: p4.diff --]
[-- Type: text/x-patch, Size: 7311 bytes --]
Index: generated/reshape_c10.c
===================================================================
--- generated/reshape_c10.c (Revision 245760)
+++ generated/reshape_c10.c (Arbeitskopie)
@@ -232,6 +232,12 @@ reshape_c10 (gfc_array_c10 * const restrict ret,
}
sdim = GFC_DESCRIPTOR_RANK (source);
+
+ /* This is needed so that gcc knows that n is at least one,
+ and that sstride[0] is always initialized. */
+ if (sdim < 1)
+ internal_error (NULL, "Source array cannot be scalar");
+
ssize = 1;
sempty = 0;
for (n = 0; n < sdim; n++)
Index: generated/reshape_c16.c
===================================================================
--- generated/reshape_c16.c (Revision 245760)
+++ generated/reshape_c16.c (Arbeitskopie)
@@ -232,6 +232,12 @@ reshape_c16 (gfc_array_c16 * const restrict ret,
}
sdim = GFC_DESCRIPTOR_RANK (source);
+
+ /* This is needed so that gcc knows that n is at least one,
+ and that sstride[0] is always initialized. */
+ if (sdim < 1)
+ internal_error (NULL, "Source array cannot be scalar");
+
ssize = 1;
sempty = 0;
for (n = 0; n < sdim; n++)
Index: generated/reshape_c4.c
===================================================================
--- generated/reshape_c4.c (Revision 245760)
+++ generated/reshape_c4.c (Arbeitskopie)
@@ -232,6 +232,12 @@ reshape_c4 (gfc_array_c4 * const restrict ret,
}
sdim = GFC_DESCRIPTOR_RANK (source);
+
+ /* This is needed so that gcc knows that n is at least one,
+ and that sstride[0] is always initialized. */
+ if (sdim < 1)
+ internal_error (NULL, "Source array cannot be scalar");
+
ssize = 1;
sempty = 0;
for (n = 0; n < sdim; n++)
Index: generated/reshape_c8.c
===================================================================
--- generated/reshape_c8.c (Revision 245760)
+++ generated/reshape_c8.c (Arbeitskopie)
@@ -232,6 +232,12 @@ reshape_c8 (gfc_array_c8 * const restrict ret,
}
sdim = GFC_DESCRIPTOR_RANK (source);
+
+ /* This is needed so that gcc knows that n is at least one,
+ and that sstride[0] is always initialized. */
+ if (sdim < 1)
+ internal_error (NULL, "Source array cannot be scalar");
+
ssize = 1;
sempty = 0;
for (n = 0; n < sdim; n++)
Index: generated/reshape_i16.c
===================================================================
--- generated/reshape_i16.c (Revision 245760)
+++ generated/reshape_i16.c (Arbeitskopie)
@@ -232,6 +232,12 @@ reshape_16 (gfc_array_i16 * const restrict ret,
}
sdim = GFC_DESCRIPTOR_RANK (source);
+
+ /* This is needed so that gcc knows that n is at least one,
+ and that sstride[0] is always initialized. */
+ if (sdim < 1)
+ internal_error (NULL, "Source array cannot be scalar");
+
ssize = 1;
sempty = 0;
for (n = 0; n < sdim; n++)
Index: generated/reshape_i4.c
===================================================================
--- generated/reshape_i4.c (Revision 245760)
+++ generated/reshape_i4.c (Arbeitskopie)
@@ -232,6 +232,12 @@ reshape_4 (gfc_array_i4 * const restrict ret,
}
sdim = GFC_DESCRIPTOR_RANK (source);
+
+ /* This is needed so that gcc knows that n is at least one,
+ and that sstride[0] is always initialized. */
+ if (sdim < 1)
+ internal_error (NULL, "Source array cannot be scalar");
+
ssize = 1;
sempty = 0;
for (n = 0; n < sdim; n++)
Index: generated/reshape_i8.c
===================================================================
--- generated/reshape_i8.c (Revision 245760)
+++ generated/reshape_i8.c (Arbeitskopie)
@@ -232,6 +232,12 @@ reshape_8 (gfc_array_i8 * const restrict ret,
}
sdim = GFC_DESCRIPTOR_RANK (source);
+
+ /* This is needed so that gcc knows that n is at least one,
+ and that sstride[0] is always initialized. */
+ if (sdim < 1)
+ internal_error (NULL, "Source array cannot be scalar");
+
ssize = 1;
sempty = 0;
for (n = 0; n < sdim; n++)
Index: generated/reshape_r10.c
===================================================================
--- generated/reshape_r10.c (Revision 245760)
+++ generated/reshape_r10.c (Arbeitskopie)
@@ -232,6 +232,12 @@ reshape_r10 (gfc_array_r10 * const restrict ret,
}
sdim = GFC_DESCRIPTOR_RANK (source);
+
+ /* This is needed so that gcc knows that n is at least one,
+ and that sstride[0] is always initialized. */
+ if (sdim < 1)
+ internal_error (NULL, "Source array cannot be scalar");
+
ssize = 1;
sempty = 0;
for (n = 0; n < sdim; n++)
Index: generated/reshape_r16.c
===================================================================
--- generated/reshape_r16.c (Revision 245760)
+++ generated/reshape_r16.c (Arbeitskopie)
@@ -232,6 +232,12 @@ reshape_r16 (gfc_array_r16 * const restrict ret,
}
sdim = GFC_DESCRIPTOR_RANK (source);
+
+ /* This is needed so that gcc knows that n is at least one,
+ and that sstride[0] is always initialized. */
+ if (sdim < 1)
+ internal_error (NULL, "Source array cannot be scalar");
+
ssize = 1;
sempty = 0;
for (n = 0; n < sdim; n++)
Index: generated/reshape_r4.c
===================================================================
--- generated/reshape_r4.c (Revision 245760)
+++ generated/reshape_r4.c (Arbeitskopie)
@@ -232,6 +232,12 @@ reshape_r4 (gfc_array_r4 * const restrict ret,
}
sdim = GFC_DESCRIPTOR_RANK (source);
+
+ /* This is needed so that gcc knows that n is at least one,
+ and that sstride[0] is always initialized. */
+ if (sdim < 1)
+ internal_error (NULL, "Source array cannot be scalar");
+
ssize = 1;
sempty = 0;
for (n = 0; n < sdim; n++)
Index: generated/reshape_r8.c
===================================================================
--- generated/reshape_r8.c (Revision 245760)
+++ generated/reshape_r8.c (Arbeitskopie)
@@ -232,6 +232,12 @@ reshape_r8 (gfc_array_r8 * const restrict ret,
}
sdim = GFC_DESCRIPTOR_RANK (source);
+
+ /* This is needed so that gcc knows that n is at least one,
+ and that sstride[0] is always initialized. */
+ if (sdim < 1)
+ internal_error (NULL, "Source array cannot be scalar");
+
ssize = 1;
sempty = 0;
for (n = 0; n < sdim; n++)
Index: intrinsics/reshape_generic.c
===================================================================
--- intrinsics/reshape_generic.c (Revision 245760)
+++ intrinsics/reshape_generic.c (Arbeitskopie)
@@ -158,6 +158,12 @@ reshape_internal (parray *ret, parray *source, sha
source_extent = 1;
sdim = GFC_DESCRIPTOR_RANK (source);
+
+ /* This is needed so that gcc knows that n is at least one,
+ and that sstride[0] is always initialized. */
+ if (sdim < 1)
+ internal_error (NULL, "Source array cannot be scalar");
+
for (n = 0; n < sdim; n++)
{
index_type se;
Index: m4/reshape.m4
===================================================================
--- m4/reshape.m4 (Revision 245760)
+++ m4/reshape.m4 (Arbeitskopie)
@@ -236,6 +236,12 @@ reshape_'rtype_ccode` ('rtype` * const restrict re
}
sdim = GFC_DESCRIPTOR_RANK (source);
+
+ /* This is needed so that gcc knows that n is at least one,
+ and that sstride[0] is always initialized. */
+ if (sdim < 1)
+ internal_error (NULL, "Source array cannot be scalar");
+
ssize = 1;
sempty = 0;
for (n = 0; n < sdim; n++)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch, fortran] Fix PR 79956, part two
2017-03-12 21:27 [patch, fortran] Fix PR 79956, part two Thomas Koenig
@ 2017-03-12 22:23 ` Jerry DeLisle
2017-03-12 23:10 ` Thomas Koenig
0 siblings, 1 reply; 5+ messages in thread
From: Jerry DeLisle @ 2017-03-12 22:23 UTC (permalink / raw)
To: fortran
On 03/12/2017 02:27 PM, Thomas Koenig wrote:
> Hello world,
>
> the attached patch fixes another occurence of PR 79956.
> In this case, we did
>
> sdim = GFC_DESCRIPTOR_RANK (source);
> ..
> for (n = 0; n < sdim; n++)
> {
> sstride[n] = GFC_DESCRIPTOR_STRIDE(source,n)
> }
>
> Now, we know that sdim can never be zero or lower, but
> the compiler doesn't, so it warned. (Why that didn't
> happen on x86_64 is an unsolved mystery so far).
>
> So, the attached patch inserts a call to internal_error
> when sdim is lower than one.
>
Can this condition:
+ if (sdim < 1)
+ internal_error (NULL, "Source array cannot be scalar");
Ever exist in reality?
Does adding this statement add a small runtime performance penalty or is it
optimized out?
Jerry
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch, fortran] Fix PR 79956, part two
2017-03-12 22:23 ` Jerry DeLisle
@ 2017-03-12 23:10 ` Thomas Koenig
2017-03-13 1:57 ` Steve Kargl
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Koenig @ 2017-03-12 23:10 UTC (permalink / raw)
To: Jerry DeLisle, fortran
Hi Jerry,
> + if (sdim < 1)
> + internal_error (NULL, "Source array cannot be scalar");
>
> Ever exist in reality?
I think it can only occur when memory corruption has happened.
> Does adding this statement add a small runtime performance penalty or is
> it optimized out?
I think it is still tested at runtime. Sometimes it is hard to read the
generated assembly...
Regards
Thomas
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch, fortran] Fix PR 79956, part two
2017-03-12 23:10 ` Thomas Koenig
@ 2017-03-13 1:57 ` Steve Kargl
2017-03-13 8:21 ` Richard Biener
0 siblings, 1 reply; 5+ messages in thread
From: Steve Kargl @ 2017-03-13 1:57 UTC (permalink / raw)
To: Thomas Koenig; +Cc: Jerry DeLisle, fortran
On Mon, Mar 13, 2017 at 12:10:05AM +0100, Thomas Koenig wrote:
> Hi Jerry,
>
> > + if (sdim < 1)
> > + internal_error (NULL, "Source array cannot be scalar");
> >
> > Ever exist in reality?
>
> I think it can only occur when memory corruption has happened.
>
> > Does adding this statement add a small runtime performance penalty or is
> > it optimized out?
>
> I think it is still tested at runtime. Sometimes it is hard to read the
> generated assembly...
>
Please, don't add a performance penalty to gfortran
to silence a silly gcc warning.
--
Steve
20161221 https://www.youtube.com/watch?v=IbCHE-hONow
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch, fortran] Fix PR 79956, part two
2017-03-13 1:57 ` Steve Kargl
@ 2017-03-13 8:21 ` Richard Biener
0 siblings, 0 replies; 5+ messages in thread
From: Richard Biener @ 2017-03-13 8:21 UTC (permalink / raw)
To: Steve Kargl; +Cc: Thomas Koenig, Jerry DeLisle, fortran
On Mon, Mar 13, 2017 at 2:57 AM, Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
> On Mon, Mar 13, 2017 at 12:10:05AM +0100, Thomas Koenig wrote:
>> Hi Jerry,
>>
>> > + if (sdim < 1)
>> > + internal_error (NULL, "Source array cannot be scalar");
>> >
>> > Ever exist in reality?
>>
>> I think it can only occur when memory corruption has happened.
>>
>> > Does adding this statement add a small runtime performance penalty or is
>> > it optimized out?
>>
>> I think it is still tested at runtime. Sometimes it is hard to read the
>> generated assembly...
>>
>
> Please, don't add a performance penalty to gfortran
> to silence a silly gcc warning.
Note it helps GCC in optimizing the following loops - GCC then can assume
the loops are always executed, saving a loop header copy and avoding the
threading code to separate the case of sdim == 0 (that just generates extra
dead code as well).
The "silly" warning tells you GCC has gone amok thinking those cases
can get special-casing for speed.
Bounding input values is always a good idea. If you think they cannot happen
at runtime a possibility is to use sth like gcc_assert in system.h:
#define gcc_assert(EXPR) \
((void)(__builtin_expect (!(EXPR), 0) ? __builtin_unreachable (), 0 : 0))
this does not result in any extra code generation but still allows GCC
to constrain
the values you assert (as a bonus you can build the library with a special flag
turning __builtin_unreachable () into a runtime error).
Richard.
> --
> Steve
> 20161221 https://www.youtube.com/watch?v=IbCHE-hONow
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-03-13 8:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-12 21:27 [patch, fortran] Fix PR 79956, part two Thomas Koenig
2017-03-12 22:23 ` Jerry DeLisle
2017-03-12 23:10 ` Thomas Koenig
2017-03-13 1:57 ` Steve Kargl
2017-03-13 8:21 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).