| Message ID | 98db0650-04a5-e398-d6bd-60d3e78bce2e@netcologne.de |
|---|---|
| State | New |
| Headers | show |
| Series | [fortran] Fix PR PR93500 | expand |
On Thu, Apr 16, 2020 at 7:53 AM Thomas Koenig via Fortran <fortran@gcc.gnu.org> wrote: > > Hello world, > > this patch fixes PR PR93500. One part of it is due to > what Steve wrote in the patch (returning from resolutions when both > operands are NULL), but that still left a nonsensical error. > Returning &gfc_bad_expr when simplifying bounds resulted in the > division by zero error actually reaching the user. > > As to why there is an extra error when this is done in the main > program, as compared to a subroutine, I don't know, but I do not > particularly care. What is important is that the first error message > is clear and reaches the user. > > Regression-tested. OK for trunk? How odd. It seems something in the procedure matching routine fails to free the symbol node for "a", while this _is_ done for the program case. A bug for another day... IMO a more clear test case uses "implicit none", which reveals the more sensible "Symbol .a. at ... has no IMPLICIT type" (at least for the program case, where a second error is displayed). One can see another variation of this with a declaration like "integer, dimension(lbound(a)) :: c" which gives a similar error "Symbol .a. is used before it is typed at ...". Regarding the new code in simplify.c: bounds[d] = simplify_bound_dim([...]) if (bounds[d] == NULL || bounds[d] == &gfc_bad_expr) { [...] if (gfc_seen_div0) { gfc_free_expr (bounds[d]); return &gfc_bad_expr; } [...] First, it appears if simplify_bound_dim returns &gfc_bad_expr (and a div/0 occurs) then this code will free &gfc_bad_expr. I'm not sure whether or not that can actually occur, but it is certainly incorrect, since &gfc_bad_expr points to static storage. The only other possible case is bounds[d] == NULL, in which case the free is a no-op. I suggest removing the free call. That being said, it looks like the same error condition can occur with the lcobound intrinsic. I see code inside simplify_cobound nearly identical to that in simplify_bound which is not guarded by the new gfc_seen_div0 check. Someone more familiar with coarrays may be able to generate a testcase which exhibits the same regression using lcobound, but I am confident it can occur. This suggests to me that the check is better placed in simplify_bound_dim, which both simplify_bound and simplify_cobound call. If simplify_bound_dim returns &gfc_bad_expr, it appears both routines should continue to do the right thing already (which would not include freeing gfc_bad_expr). It is the call to gfc_resolve_array_spec within simplify_bound_dim which signals the div0, so I believe the div0 check should be inserted right here (around line 4075). How about the following patch to simplify.c instead (which appears to have the fortunate side-effect of fixing a formerly leaked result expression): diff --git a/gcc/fortran/simplify.c b/gcc/fortran/simplify.c index d5703e38251..5395694dc67 100644 --- a/gcc/fortran/simplify.c +++ b/gcc/fortran/simplify.c @@ -4073,7 +4073,13 @@ simplify_bound_dim (gfc_expr *array, gfc_expr *kind, int d, int upper, gcc_assert (as); if (!gfc_resolve_array_spec (as, 0)) - return NULL; + { + gfc_free_expr (result); + if (gfc_seen_div0) + return &gfc_bad_expr; + else + return NULL; + } /* The last dimension of an assumed-size array is special. */ if ((!coarray && d == as->rank && as->type == AS_ASSUMED_SIZE && !upper) --- Fritz Reese diff --git a/gcc/fortran/simplify.c b/gcc/fortran/simplify.c index d5703e38251..5395694dc67 100644 --- a/gcc/fortran/simplify.c +++ b/gcc/fortran/simplify.c @@ -4073,7 +4073,13 @@ simplify_bound_dim (gfc_expr *array, gfc_expr *kind, int d, int upper, gcc_assert (as); if (!gfc_resolve_array_spec (as, 0)) - return NULL; + { + gfc_free_expr (result); + if (gfc_seen_div0) + return &gfc_bad_expr; + else + return NULL; + } /* The last dimension of an assumed-size array is special. */ if ((!coarray && d == as->rank && as->type == AS_ASSUMED_SIZE && !upper)
Hi Fritz, > First, it appears if simplify_bound_dim returns &gfc_bad_expr (and a > div/0 occurs) then this code will free &gfc_bad_expr. I'm not sure > whether or not that can actually occur, but it is certainly incorrect, > since &gfc_bad_expr points to static storage. The only other possible > case is bounds[d] == NULL, in which case the free is a no-op. I > suggest removing the free call. OK, removed. > That being said, it looks like the same error condition can occur with > the lcobound intrinsic. I have adjusted the test case so it also checks for the coarray case; this is handled correctly with a "Divion by zero" error without changing the patch in that respect. So, anything else? If not, I will commit this within a few days. Regards Thomas diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c index 9b95200c241..650837e18c3 100644 --- a/gcc/fortran/resolve.c +++ b/gcc/fortran/resolve.c @@ -3986,6 +3986,9 @@ resolve_operator (gfc_expr *e) op1 = e->value.op.op1; op2 = e->value.op.op2; + if (op1 == NULL && op2 == NULL) + return false; + dual_locus_error = false; /* op1 and op2 cannot both be BOZ. */ diff --git a/gcc/fortran/simplify.c b/gcc/fortran/simplify.c index 807565b4e80..855fbe27d9f 100644 --- a/gcc/fortran/simplify.c +++ b/gcc/fortran/simplify.c @@ -4251,7 +4251,11 @@ simplify_bound (gfc_expr *array, gfc_expr *dim, gfc_expr *kind, int upper) for (j = 0; j < d; j++) gfc_free_expr (bounds[j]); - return bounds[d]; + + if (gfc_seen_div0) + return &gfc_bad_expr; + else + return bounds[d]; } } diff --git a/gcc/testsuite/gfortran.dg/arith_divide_3.f90 b/gcc/testsuite/gfortran.dg/arith_divide_3.f90 new file mode 100644 index 00000000000..95682dfdda7 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/arith_divide_3.f90 @@ -0,0 +1,17 @@ +! { dg-do compile } +! { dg-options "-fcoarray=single" } +! PR 93500 - this used to cause an ICE + +program p + integer :: a(min(2,0)/0) ! { dg-error "Division by zero" } + integer, save :: c[min(2,0)/0,*] ! { dg-error "Division by zero|must have constant shape" } + integer :: b = lbound(a) ! { dg-error "must be an array" } + print *,lcobound(c) +end program p + +subroutine s + integer :: a(min(2,0)/0) ! { dg-error "Division by zero" } + integer, save :: c[min(2,0)/0,*] ! { dg-error "Division by zero" } + integer :: b = lbound(a) + print *,lcobound(c) +end subroutine s
On Fri, Apr 17, 2020 at 10:33 AM Thomas Koenig <tkoenig@netcologne.de> wrote: > > Hi Fritz, > > > First, it appears if simplify_bound_dim returns &gfc_bad_expr (and a > > div/0 occurs) then this code will free &gfc_bad_expr. I'm not sure > > whether or not that can actually occur, but it is certainly incorrect, > > since &gfc_bad_expr points to static storage. The only other possible > > case is bounds[d] == NULL, in which case the free is a no-op. I > > suggest removing the free call. > > OK, removed. > > > That being said, it looks like the same error condition can occur with > > the lcobound intrinsic. > > I have adjusted the test case so it also checks for the coarray case; > this is handled correctly with a "Divion by zero" error without > changing the patch in that respect. > > So, anything else? If not, I will commit this within a few days. > Let the record show I am unsettled that the error path is different between simplify_bound and simplify_cobound, and that different errors occur for the program and subroutine case. There is probably a root cause somewhere else that should be fixed one day. However! That is not a problem for this PR, nor for stage 4, and is certainly no fault of this patch. Therefore the patch looks OK to me for now. Thank you for your work! --- Fritz Reese
diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c index 9b95200c241..650837e18c3 100644 --- a/gcc/fortran/resolve.c +++ b/gcc/fortran/resolve.c @@ -3986,6 +3986,9 @@ resolve_operator (gfc_expr *e) op1 = e->value.op.op1; op2 = e->value.op.op2; + if (op1 == NULL && op2 == NULL) + return false; + dual_locus_error = false; /* op1 and op2 cannot both be BOZ. */ diff --git a/gcc/fortran/simplify.c b/gcc/fortran/simplify.c index 807565b4e80..fba7f7020be 100644 --- a/gcc/fortran/simplify.c +++ b/gcc/fortran/simplify.c @@ -4251,7 +4251,13 @@ simplify_bound (gfc_expr *array, gfc_expr *dim, gfc_expr *kind, int upper) for (j = 0; j < d; j++) gfc_free_expr (bounds[j]); - return bounds[d]; + if (gfc_seen_div0) + { + gfc_free_expr (bounds[d]); + return &gfc_bad_expr; + } + else + return bounds[d]; } } diff --git a/gcc/testsuite/gfortran.dg/arith_divide_3.f90 b/gcc/testsuite/gfortran.dg/arith_divide_3.f90 new file mode 100644 index 00000000000..d9eb4a0d590 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/arith_divide_3.f90 @@ -0,0 +1,13 @@ +! { dg-do compile } +! PR 93500 - this used to cause an ICE + +program p + integer :: a(min(2,0)/0) ! { dg-error "Division by zero" } + integer :: b = lbound(a) ! { dg-error "must be an array" } +end + +subroutine s + integer :: a(min(2,0)/0) ! { dg-error "Division by zero" } + integer :: b = lbound(a) +end +