Hi Tobias,
> LGTM – I am fine with either variant, but I am slightly inclined to
> removing the gcc_assert*
> – as I believe that the existing checks come early enough and do seem to
> work well.
I played some more and found additional cases that we hadn't discussed
before. (At least I hadn't thought of them because of the focus on
deferred length strings.)
- automatic string variables / arrays
- assumed length strings
- PDTs with character components.
The last one actually turned out sort of "hopeless" for now, so I opened
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102003
to track this.
I added the other cases to testcase pr100950.f90 and reduced the checks
and code within "substring_has_constant_len" to the bare minimum.
See the attached follow-up patch.
> Can you check ('grep') whether we already have sufficient coverage of
> substring out of bounds?
> We presumably have, but if you spot something, I think it makes sense to
> add those to the testsuite.
We do have some checks on substring indices (e.g. substr_10.f90),
but not really extensive coverage.
> Tobias
> *I think GCC won't complain but if ENABLE_ASSERT_CHECKING is not defined,
> there is then a pointless 'length =' assignment, overridden before it is
> ever used.
Yes. This is fixed below.
I guess I have to apologize for things getting a bit out of control for
this PR, but the results are on the other hand way beyond my initial
expectations...
Re-regtested on x86_64-pc-linux-gnu. Should be safe elsewhere...
OK?
Thanks,
Harald
Fortran - extend set of substring expressions handled in length simplification
gcc/fortran/ChangeLog:
PR fortran/100950
* simplify.c (substring_has_constant_len): Minimize checks for
substring expressions being allowed.
gcc/testsuite/ChangeLog:
PR fortran/100950
* gfortran.dg/pr100950.f90: Extend coverage.
diff --git a/gcc/fortran/simplify.c b/gcc/fortran/simplify.c
index 4cb73e836c7..b46cbfa90ab 100644
--- a/gcc/fortran/simplify.c
+++ b/gcc/fortran/simplify.c
@@ -4533,14 +4533,7 @@ substring_has_constant_len (gfc_expr *e)
|| !ref->u.ss.start
|| ref->u.ss.start->expr_type != EXPR_CONSTANT
|| !ref->u.ss.end
- || ref->u.ss.end->expr_type != EXPR_CONSTANT
- || !ref->u.ss.length)
- return false;
-
- /* For non-deferred strings the given length shall be constant. */
- if (!e->ts.deferred
- && (!ref->u.ss.length->length
- || ref->u.ss.length->length->expr_type != EXPR_CONSTANT))
+ || ref->u.ss.end->expr_type != EXPR_CONSTANT)
return false;
/* Basic checks on substring starting and ending indices. */
@@ -4551,27 +4544,7 @@ substring_has_constant_len (gfc_expr *e)
iend = gfc_mpz_get_hwi (ref->u.ss.end->value.integer);
if (istart <= iend)
- {
- if (istart < 1)
- {
- gfc_error ("Substring start index (%wd) at %L below 1",
- istart, &ref->u.ss.start->where);
- return false;
- }
-
- /* For deferred strings use end index as proxy for length. */
- if (e->ts.deferred)
- length = iend;
- else
- length = gfc_mpz_get_hwi (ref->u.ss.length->length->value.integer);
- if (iend > length)
- {
- gfc_error ("Substring end index (%wd) at %L exceeds string length",
- iend, &ref->u.ss.end->where);
- return false;
- }
- length = iend - istart + 1;
- }
+ length = iend - istart + 1;
else
length = 0;
diff --git a/gcc/testsuite/gfortran.dg/pr100950.f90 b/gcc/testsuite/gfortran.dg/pr100950.f90
index cb9d126bc18..a19409c2507 100644
--- a/gcc/testsuite/gfortran.dg/pr100950.f90
+++ b/gcc/testsuite/gfortran.dg/pr100950.f90
@@ -46,6 +46,18 @@ program p
integer, parameter :: l9 = len (r(1)%u(:)(3:4))
if (l9 /= 2) stop 13
end block
+
+ call sub (42, "abcde")
+contains
+ subroutine sub (m, c)
+ integer, intent(in) :: m
+ character(len=*), intent(in) :: c
+ character(len=m) :: p, o(3)
+ integer, parameter :: l10 = len (p(6:7))
+ integer, parameter :: l11 = len (o(:)(6:7))
+ integer, parameter :: l12 = len (c(2:3))
+ if (l10 /= 2 .or. l11 /= 2 .or. l12 /= 2) stop 14
+ end subroutine sub
end
! { dg-final { scan-tree-dump-times "_gfortran_stop_numeric" 2 "original" } }
