diff mbox series

Aw: Re: [PATCH] PR fortran/100950 - ICE in output_constructor_regular_field, at varasm.c:5514

Message ID trinity-24a1c978-4091-4505-9f52-25e293ab054a-1629452733397@3c-app-gmx-bap31
State New
Headers show
Series Aw: Re: [PATCH] PR fortran/100950 - ICE in output_constructor_regular_field, at varasm.c:5514 | expand

Commit Message

Harald Anlauf Aug. 20, 2021, 9:45 a.m. UTC 
Hi Jakob,

thanks for the detailed explanation!

> The other much easier but uglier option is to use a temporary buffer:
>   char buffer[21];
>   sprintf (buffer, HOST_WIDE_INT_PRINT_DEC, hwint_val);
>   gfc_error ("... %s ...", ... buffer ...);
> This works, as it uses the host sprintf i.e. *printf family for which
> HOST_WIDE_INT_PRINT_DEC macro is designed.

The attached followup patch implements this.

Can anybody affected by current HEAD confirm that this fixes bootstrap?

Thanks,
Harald

Comments

Jakub Jelinek Aug. 20, 2021, 10:12 a.m. UTC | #1 
On Fri, Aug 20, 2021 at 11:45:33AM +0200, Harald Anlauf wrote:
> Hi Jakob,
> 
> thanks for the detailed explanation!
> 
> > The other much easier but uglier option is to use a temporary buffer:
> >   char buffer[21];
> >   sprintf (buffer, HOST_WIDE_INT_PRINT_DEC, hwint_val);
> >   gfc_error ("... %s ...", ... buffer ...);
> > This works, as it uses the host sprintf i.e. *printf family for which
> > HOST_WIDE_INT_PRINT_DEC macro is designed.
> 
> The attached followup patch implements this.
> 
> Can anybody affected by current HEAD confirm that this fixes bootstrap?

I have verified it fixes i686-linux bootstrap.
But the new testcase doesn't trigger any of those new errors, is something
else in the testsuite covering those or do you have some short snippet that
could verify the errors work properly?

> diff --git a/gcc/fortran/simplify.c b/gcc/fortran/simplify.c
> index 492867e12cb..eaabbffca4d 100644
> --- a/gcc/fortran/simplify.c
> +++ b/gcc/fortran/simplify.c
> @@ -4552,11 +4552,12 @@ substring_has_constant_len (gfc_expr *e)
> 
>    if (istart <= iend)
>      {
> +      char buffer[21];
>        if (istart < 1)
>  	{
> -	  gfc_error ("Substring start index (" HOST_WIDE_INT_PRINT_DEC
> -		     ") at %L below 1",
> -		     istart, &ref->u.ss.start->where);
> +	  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;
>  	}
> 
> @@ -4567,9 +4568,9 @@ substring_has_constant_len (gfc_expr *e)
>  	length = gfc_mpz_get_hwi (ref->u.ss.length->length->value.integer);
>        if (iend > length)
>  	{
> -	  gfc_error ("Substring end index (" HOST_WIDE_INT_PRINT_DEC
> -		     ") at %L exceeds string length",
> -		     iend, &ref->u.ss.end->where);
> +	  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;
>  	}
>        length = iend - istart + 1;


	Jakub
Harald Anlauf Aug. 20, 2021, 10:53 a.m. UTC | #2 
> Gesendet: Freitag, 20. August 2021 um 12:12 Uhr
> Von: "Jakub Jelinek" <jakub@redhat.com>

> I have verified it fixes i686-linux bootstrap.
> But the new testcase doesn't trigger any of those new errors, is something
> else in the testsuite covering those or do you have some short snippet that
> could verify the errors work properly?

The testcase was designed to verify correct simplification of
substring length for a variety of cases.

I played with variations of the testcase by specifying illegal
substring bounds, but all these cases were caught in a different
spot with similar error messages.

I put those checks into substring_has_constant_len as sanity checks.
Do you want me to remove them?

Or are you trying to say that it is important to exercise them?

Thanks,
Harald
Jakub Jelinek Aug. 20, 2021, 10:59 a.m. UTC | #3 
On Fri, Aug 20, 2021 at 12:53:33PM +0200, Harald Anlauf wrote:
> > Gesendet: Freitag, 20. August 2021 um 12:12 Uhr
> > Von: "Jakub Jelinek" <jakub@redhat.com>
> 
> > I have verified it fixes i686-linux bootstrap.
> > But the new testcase doesn't trigger any of those new errors, is something
> > else in the testsuite covering those or do you have some short snippet that
> > could verify the errors work properly?
> 
> The testcase was designed to verify correct simplification of
> substring length for a variety of cases.
> 
> I played with variations of the testcase by specifying illegal
> substring bounds, but all these cases were caught in a different
> spot with similar error messages.
> 
> I put those checks into substring_has_constant_len as sanity checks.
> Do you want me to remove them?
> 
> Or are you trying to say that it is important to exercise them?

Not a big deal, just wanted to verify that it actually works on i686-linux.
Please just go ahead and commit the patch to avoid the bootstrap failures.

Thanks.

	Jakub
Tobias Burnus Aug. 20, 2021, 12:01 p.m. UTC | #4 
On 20.08.21 12:53, Harald Anlauf wrote:

>> I have verified it fixes i686-linux bootstrap.
>> But the new testcase doesn't trigger any of those new errors, is something
>> else in the testsuite covering those or do you have some short snippet that
>> could verify the errors work properly?
> The testcase was designed to verify correct simplification of
> substring length for a variety of cases.
>
> I played with variations of the testcase by specifying illegal
> substring bounds, but all these cases were caught in a different
> spot with similar error messages.

I can confirm this. – I think in order to reduce the clutter, the
diagnostic probably should be removed.

Tobias

-----------------
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
diff mbox series

Patch

diff --git a/gcc/fortran/simplify.c b/gcc/fortran/simplify.c
index 492867e12cb..eaabbffca4d 100644
--- a/gcc/fortran/simplify.c
+++ b/gcc/fortran/simplify.c
@@ -4552,11 +4552,12 @@  substring_has_constant_len (gfc_expr *e)

   if (istart <= iend)
     {
+      char buffer[21];
       if (istart < 1)
 	{
-	  gfc_error ("Substring start index (" HOST_WIDE_INT_PRINT_DEC
-		     ") at %L below 1",
-		     istart, &ref->u.ss.start->where);
+	  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;
 	}

@@ -4567,9 +4568,9 @@  substring_has_constant_len (gfc_expr *e)
 	length = gfc_mpz_get_hwi (ref->u.ss.length->length->value.integer);
       if (iend > length)
 	{
-	  gfc_error ("Substring end index (" HOST_WIDE_INT_PRINT_DEC
-		     ") at %L exceeds string length",
-		     iend, &ref->u.ss.end->where);
+	  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;
 	}
       length = iend - istart + 1;