| Message ID | trinity-9e4a13d5-7919-4569-8bff-7bae9a658e23-1629461868330@3c-app-gmx-bap33 |
|---|---|
| State | New |
| Headers | show |
| Series | Aw: Re: Re: Re: [PATCH] PR fortran/100950 - ICE in output_constructor_regular_field, at varasm.c:5514 | expand |
Hi Harald, On 20.08.21 14:17, Harald Anlauf wrote: >> I can confirm this. – I think in order to reduce the clutter, the >> diagnostic probably should be removed. > I am unable to prove that we will never that check. So how about: > diff --git a/gcc/fortran/simplify.c b/gcc/fortran/simplify.c > index eaabbffca4d..04722a8640c 100644 > --- a/gcc/fortran/simplify.c > +++ b/gcc/fortran/simplify.c > @@ -4552,27 +4552,13 @@ substring_has_constant_len (gfc_expr *e) > > if (istart <= iend) > { > - char buffer[21]; > - if (istart < 1) > - { > - sprintf (buffer, HOST_WIDE_INT_PRINT_DEC, istart); > - gfc_error ("Substring start index (%s) at %L below 1", > - buffer, &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) > - { > - sprintf (buffer, HOST_WIDE_INT_PRINT_DEC, iend); > - gfc_error ("Substring end index (%s) at %L exceeds string length", > - buffer, &ref->u.ss.end->where); > - return false; > - } > + > + gcc_assert (istart >= 1 && iend <= length); > length = iend - istart + 1; > } > else > > Or skip the gcc_assert and cross fingers... (we then would not even need to > verify ref->u.ss.length in that depth). 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. 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. 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. ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
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 eaabbffca4d..04722a8640c 100644 --- a/gcc/fortran/simplify.c +++ b/gcc/fortran/simplify.c @@ -4552,27 +4552,13 @@ substring_has_constant_len (gfc_expr *e) if (istart <= iend) { - char buffer[21]; - if (istart < 1) - { - sprintf (buffer, HOST_WIDE_INT_PRINT_DEC, istart); - gfc_error ("Substring start index (%s) at %L below 1", - buffer, &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) - { - sprintf (buffer, HOST_WIDE_INT_PRINT_DEC, iend); - gfc_error ("Substring end index (%s) at %L exceeds string length", - buffer, &ref->u.ss.end->where); - return false; - } + + gcc_assert (istart >= 1 && iend <= length); length = iend - istart + 1; } else