[PATCH] PR fortran/78240 -- kludge of the day

[PATCH] PR fortran/78240 -- kludge of the day

[Steve Kargl]

The following patch fixes PR fortran/78240.  It seems
to me to be inelegant, but it does pass regression
testing.  The kludgy portion occurs in decl.c.
march_clist_expr is clearly expecting an array with
constant dimension due to gcc_assert.  When -fdec
is used and one takes Gerhard code (see testcase),
the assertion is raised.  The patch checks for 
-fdec and failure of spec_size(), and then goes
to cleanup.  The second part in resolve.c is needed
to avoid a NULL pointer dereference when one forgets
to use -fdec with Gerhard's code.  I did not write a
testcase for this, because the dereference occurs in
the 5th error message emitted.  OK to commit?


2017-11-09  Steven G. Kargl  <kargl@gcc.gnu.org>

	PR fortran/78240
	* decl.c (match_clist_expr): Reject malformed DEC structure.
	* resolve.c (match_clist_expr): Avoid NULL pointer deference.

2017-11-09  Steven G. Kargl  <kargl@gcc.gnu.org>

	PR fortran/78240
	* gfortran.dg/dec_structure_23.f90: new test.

Index: gcc/fortran/decl.c
===================================================================
--- gcc/fortran/decl.c	(revision 254603)
+++ gcc/fortran/decl.c	(working copy)
@@ -735,6 +735,8 @@ match_clist_expr (gfc_expr **result, gfc_typespec *ts,
 
       /* Validate sizes. */
       gcc_assert (gfc_array_size (expr, &size));
+      if (flag_dec && !spec_size (as, &repeat))
+	goto cleanup;
       gcc_assert (spec_size (as, &repeat));
       cmp = mpz_cmp (size, repeat);
       if (cmp < 0)
Index: gcc/fortran/resolve.c
===================================================================
--- gcc/fortran/resolve.c	(revision 254603)
+++ gcc/fortran/resolve.c	(working copy)
@@ -15284,7 +15290,7 @@ check_data_variable (gfc_data_variable *var, locus *wh
       if (!gfc_array_size (e, &size))
 	{
 	  gfc_error ("Nonconstant array section at %L in DATA statement",
-		     &e->where);
+		     where);
 	  mpz_clear (offset);
 	  return false;
 	}
Index: gcc/testsuite/gfortran.dg/dec_structure_23.f90
===================================================================
--- gcc/testsuite/gfortran.dg/dec_structure_23.f90	(nonexistent)
+++ gcc/testsuite/gfortran.dg/dec_structure_23.f90	(working copy)
@@ -0,0 +1,9 @@
+! { dg-do compile }
+! { dg-options "-fdec" }
+! PR fortran/78240
+! Contributed by Gerhard Steinmetz
+program p
+   structure /s/
+      integer x(n) /1/  ! { dg-error "Unclassifiable statement" }
+   end structure
+end

-- 
Steve

Re: [PATCH] PR fortran/78240 -- kludge of the day

[Fritz Reese]

[-- Attachment #1: Type: text/plain, Size: 4131 bytes --]

On Thu, Nov 9, 2017 at 5:02 PM, Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
> The following patch fixes PR fortran/78240.  It seems
> to me to be inelegant, but it does pass regression
> testing. [...] OK to commit?

Upon closer analysis, the patch is insufficient to fix the PR. I will
explain below. At the bottom of this letter I explain and have
attached a new patch which fixes some more subtle issues in the code
which causes the PR.

> [...] The second part in resolve.c is needed
> to avoid a NULL pointer dereference when one forgets
> to use -fdec with Gerhard's code.
[...]
> --- gcc/fortran/resolve.c       (revision 254603)
> +++ gcc/fortran/resolve.c       (working copy)
> @@ -15284,7 +15290,7 @@ check_data_variable (gfc_data_variable *var, locus *wh
>        if (!gfc_array_size (e, &size))
>         {
>           gfc_error ("Nonconstant array section at %L in DATA statement",
> -                    &e->where);
> +                    where);
>           mpz_clear (offset);
>           return false;
>         }

This portion of the patch is correct and fixes one of the issues.

> [...] The kludgy portion occurs in decl.c.
> march_clist_expr is clearly expecting an array with
> constant dimension due to gcc_assert.  When -fdec
> is used and one takes Gerhard code (see testcase),
> the assertion is raised.  The patch checks for
> -fdec and failure of spec_size(), and then goes
> to cleanup.
[...]
> --- gcc/fortran/decl.c  (revision 254603)
> +++ gcc/fortran/decl.c  (working copy)
> @@ -735,6 +735,8 @@ match_clist_expr (gfc_expr **result, gfc_typespec *ts,
>
>        /* Validate sizes. */
>        gcc_assert (gfc_array_size (expr, &size));
> +      if (flag_dec && !spec_size (as, &repeat))
> +       goto cleanup;
>        gcc_assert (spec_size (as, &repeat));
>        cmp = mpz_cmp (size, repeat);
>        if (cmp < 0)

This portion of the patch is insufficient. By removing the assert on
spec_size, the remaining code improperly handles the memory of the two
mpz_integer variables 'size' and 'repeat'. Since gfc_array_size() and
spec_size() both allocate their mpz_integer* arguments,
match_clist_expr() should not allocate them (or clear them on error).
With the original patch, memory corruption occurs in several ways
(double-alloc of repeat in gfc_array_size, double-free of size on
failure of spec_size). Strange that regression testing did not reveal
the memory corruption (I see the corruptions in my own gcc after
manually applying the patch, at least).

I have attached a patch which better handles false return from
spec_size, which occurs when the array-spec for the variable being
initialized is not constant. Proper handling requires careful
management of the two mpz_integer size variables used in
gfc_array_size and spec_size. The patch also includes the fix in
resolve.c.

Thanks for pointing me to this issue and allowing me time to review
it. The new patch passes all regression tests including its two tests
(one for each issue above). OK to commit this one?

---
Fritz Reese



From f2b8262a1a366b072493b129d38465ffa3d265bc Mon Sep 17 00:00:00 2001
From: Fritz Reese <fritzoreese@gmail.com>
Date: Mon, 13 Nov 2017 15:58:26 -0500
Subject: [PATCH] Fix ICE on non-constant array-specs in clist initializers.

        PR fortran/78240
        gcc/fortran/
        * decl.c (match_clist_expr): Replace gcc_assert with proper
        handling of bad result from spec_size().
        * resolve.c (check_data_variable): Avoid NULL dereference when passing
        locus to gfc_error.

        PR fortran/78240
        * gcc/testsuite/gfortran.dg/dec_structure_23.f90: New.
        * gcc/testsuite/gfortran.dg/pr78240.f90: New.
---
 gcc/fortran/decl.c                             | 37 +++++++++++++++++---------
 gcc/fortran/resolve.c                          |  2 +-
 gcc/testsuite/gfortran.dg/dec_structure_23.f90 | 19 +++++++++++++
 gcc/testsuite/gfortran.dg/pr78240.f90          | 12 +++++++++
 4 files changed, 56 insertions(+), 14 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/dec_structure_23.f90
 create mode 100644 gcc/testsuite/gfortran.dg/pr78240.f90

[-- Attachment #2: pr78240.patch --]
[-- Type: application/octet-stream, Size: 5166 bytes --]

From f2b8262a1a366b072493b129d38465ffa3d265bc Mon Sep 17 00:00:00 2001
From: Fritz Reese <fritzoreese@gmail.com>
Date: Mon, 13 Nov 2017 15:58:26 -0500
Subject: [PATCH] Fix ICE on non-constant array-specs in clist initializers.

	PR fortran/78240
	gcc/fortran/
	* decl.c (match_clist_expr): Replace gcc_assert with proper
	handling of bad result from spec_size().
	* resolve.c (check_data_variable): Avoid NULL dereference when passing
	locus to gfc_error.

	PR fortran/78240
	* gcc/testsuite/gfortran.dg/dec_structure_23.f90: New.
	* gcc/testsuite/gfortran.dg/pr78240.f90: New.
---
 gcc/fortran/decl.c                             | 37 +++++++++++++++++---------
 gcc/fortran/resolve.c                          |  2 +-
 gcc/testsuite/gfortran.dg/dec_structure_23.f90 | 19 +++++++++++++
 gcc/testsuite/gfortran.dg/pr78240.f90          | 12 +++++++++
 4 files changed, 56 insertions(+), 14 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/dec_structure_23.f90
 create mode 100644 gcc/testsuite/gfortran.dg/pr78240.f90

diff --git a/gcc/fortran/decl.c b/gcc/fortran/decl.c
index 1a2d8f004ca..120b32c2f4b 100644
--- a/gcc/fortran/decl.c
+++ b/gcc/fortran/decl.c
@@ -632,14 +632,13 @@ match_clist_expr (gfc_expr **result, gfc_typespec *ts, gfc_array_spec *as)
   gfc_expr *expr = NULL;
   match m;
   locus where;
-  mpz_t repeat, size;
+  mpz_t repeat, cons_size, as_size;
   bool scalar;
   int cmp;
 
   gcc_assert (ts);
 
   mpz_init_set_ui (repeat, 0);
-  mpz_init (size);
   scalar = !as || !as->rank;
 
   /* We have already matched '/' - now look for a constant list, as with
@@ -733,16 +732,30 @@ match_clist_expr (gfc_expr **result, gfc_typespec *ts, gfc_array_spec *as)
       expr->rank = as->rank;
       expr->shape = gfc_get_shape (expr->rank);
 
-      /* Validate sizes. */
-      gcc_assert (gfc_array_size (expr, &size));
-      gcc_assert (spec_size (as, &repeat));
-      cmp = mpz_cmp (size, repeat);
-      if (cmp < 0)
-        gfc_error ("Not enough elements in array initializer at %C");
-      else if (cmp > 0)
-        gfc_error ("Too many elements in array initializer at %C");
+      /* Validate sizes.  We built expr ourselves, so cons_size will be
+	 constant (we fail above for non-constant expressions).
+	 We still need to verify that the array-spec has constant size.  */
+      cmp = 0;
+      gcc_assert (gfc_array_size (expr, &cons_size));
+      if (!spec_size (as, &as_size))
+	{
+	  gfc_error ("Expected constant array-spec in initializer list at %L",
+		     as->type == AS_EXPLICIT ? &as->upper[0]->where : &where);
+	  cmp = -1;
+	}
+      else
+	{
+	  /* Make sure the specs are of the same size.  */
+	  cmp = mpz_cmp (cons_size, as_size);
+	  if (cmp < 0)
+	    gfc_error ("Not enough elements in array initializer at %C");
+	  else if (cmp > 0)
+	    gfc_error ("Too many elements in array initializer at %C");
+	  mpz_clear (as_size);
+	}
+      mpz_clear (cons_size);
       if (cmp)
-        goto cleanup;
+	goto cleanup;
     }
 
   /* Make sure scalar types match. */
@@ -754,7 +767,6 @@ match_clist_expr (gfc_expr **result, gfc_typespec *ts, gfc_array_spec *as)
     expr->ts.u.cl->length_from_typespec = 1;
 
   *result = expr;
-  mpz_clear (size);
   mpz_clear (repeat);
   return MATCH_YES;
 
@@ -766,7 +778,6 @@ cleanup:
     expr->value.constructor = NULL;
   gfc_free_expr (expr);
   gfc_constructor_free (array_head);
-  mpz_clear (size);
   mpz_clear (repeat);
   return MATCH_ERROR;
 }
diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index 1dde0d3ce1a..61792590d87 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -15284,7 +15284,7 @@ check_data_variable (gfc_data_variable *var, locus *where)
       if (!gfc_array_size (e, &size))
 	{
 	  gfc_error ("Nonconstant array section at %L in DATA statement",
-		     &e->where);
+		     where);
 	  mpz_clear (offset);
 	  return false;
 	}
diff --git a/gcc/testsuite/gfortran.dg/dec_structure_23.f90 b/gcc/testsuite/gfortran.dg/dec_structure_23.f90
new file mode 100644
index 00000000000..3c68489c4bd
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/dec_structure_23.f90
@@ -0,0 +1,19 @@
+! { dg-do compile }
+! { dg-options "-fdec-structure" }
+!
+! PR fortran/78240
+!
+! Test a regression where an ICE occurred attempting to create array variables
+! with non-constant array-specs in legacy clist initializers.
+!
+
+program p
+  implicit none
+  integer :: nn
+  real :: rr
+  structure /s/
+    integer x(n)    /1/   ! { dg-error "xpected constant" }
+    integer xx(nn)  /1/   ! { dg-error "xpected constant" }
+    integer xxx(rr) /1.0/ ! { dg-error "xpected constant" }
+  end structure
+end
diff --git a/gcc/testsuite/gfortran.dg/pr78240.f90 b/gcc/testsuite/gfortran.dg/pr78240.f90
new file mode 100644
index 00000000000..76542bfbab2
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr78240.f90
@@ -0,0 +1,12 @@
+! { dg-do compile }
+!
+! PR fortran/78240
+!
+! Test a regression where an ICE occurred by passing an invalid reference
+! to the error handling routine for non-constant array-specs in DATA list
+! initializers.
+!
+
+program p
+  integer x(n)    /1/   ! { dg-error "Nonconstant array" }
+end
-- 
2.12.2

Re: [PATCH] PR fortran/78240 -- kludge of the day

[Steve Kargl]

On Mon, Nov 13, 2017 at 04:42:31PM -0500, Fritz Reese wrote:
> On Thu, Nov 9, 2017 at 5:02 PM, Steve Kargl
> <sgk@troutmask.apl.washington.edu> wrote:
> > The following patch fixes PR fortran/78240.  It seems
> > to me to be inelegant, but it does pass regression
> > testing. [...] OK to commit?
> 
> Upon closer analysis, the patch is insufficient to fix the PR. I will
> explain below. At the bottom of this letter I explain and have
> attached a new patch which fixes some more subtle issues in the code
> which causes the PR.

Thanks for taking a look.  You're correct that I should 
have looked more closely at the memory management that
you noted.

> Thanks for pointing me to this issue and allowing me time to review
> it. The new patch passes all regression tests including its two tests
> (one for each issue above). OK to commit this one?

Yes.  Thanks for the patch.

-- 
steve

Re: [PATCH] PR fortran/78240 -- kludge of the day

[Janus Weil]

Hi guys,

I see this new test case failing on x86_64-linux-gnu:

FAIL: gfortran.dg/pr78240.f90   -O  (test for excess errors)


$ gfortran-8 pr78240.f90
pr78240.f90:11:12:

   integer x(n)    /1/   ! { dg-error "Nonconstant array" }
            1
Error: Variable ‘n’ cannot appear in the expression at (1)
pr78240.f90:11:14:

   integer x(n)    /1/   ! { dg-error "Nonconstant array" }
              1
Error: The module or main program array ‘x’ at (1) must have constant shape
pr78240.f90:11:19:

   integer x(n)    /1/   ! { dg-error "Nonconstant array" }
                   1
Error: Nonconstant array section at (1) in DATA statement


Cheers,
Janus




2017-11-14 1:32 GMT+01:00 Steve Kargl <sgk@troutmask.apl.washington.edu>:
> On Mon, Nov 13, 2017 at 04:42:31PM -0500, Fritz Reese wrote:
>> On Thu, Nov 9, 2017 at 5:02 PM, Steve Kargl
>> <sgk@troutmask.apl.washington.edu> wrote:
>> > The following patch fixes PR fortran/78240.  It seems
>> > to me to be inelegant, but it does pass regression
>> > testing. [...] OK to commit?
>>
>> Upon closer analysis, the patch is insufficient to fix the PR. I will
>> explain below. At the bottom of this letter I explain and have
>> attached a new patch which fixes some more subtle issues in the code
>> which causes the PR.
>
> Thanks for taking a look.  You're correct that I should
> have looked more closely at the memory management that
> you noted.
>
>> Thanks for pointing me to this issue and allowing me time to review
>> it. The new patch passes all regression tests including its two tests
>> (one for each issue above). OK to commit this one?
>
> Yes.  Thanks for the patch.
>
> --
> steve

Re: [PATCH] PR fortran/78240 -- kludge of the day

[Fritz Reese]

On Tue, Nov 14, 2017 at 4:58 PM, Janus Weil <janus@gcc.gnu.org> wrote:
> Hi guys,
>
> I see this new test case failing on x86_64-linux-gnu:
>
> FAIL: gfortran.dg/pr78240.f90   -O  (test for excess errors)
>
>
> $ gfortran-8 pr78240.f90
> pr78240.f90:11:12:
>
>    integer x(n)    /1/   ! { dg-error "Nonconstant array" }
>             1
> Error: Variable ‘n’ cannot appear in the expression at (1)
> pr78240.f90:11:14:
>
>    integer x(n)    /1/   ! { dg-error "Nonconstant array" }
>               1
> Error: The module or main program array ‘x’ at (1) must have constant shape
> pr78240.f90:11:19:
>
>    integer x(n)    /1/   ! { dg-error "Nonconstant array" }
>                    1
> Error: Nonconstant array section at (1) in DATA statement
> [...]

... does anyone know how to tell dejagnu to expect multiple errors on
a single line?

---
Fritz Reese

Re: [PATCH] PR fortran/78240 -- kludge of the day

[Janus Weil]

2017-11-14 23:21 GMT+01:00 Fritz Reese <fritzoreese@gmail.com>:
> On Tue, Nov 14, 2017 at 4:58 PM, Janus Weil <janus@gcc.gnu.org> wrote:
>> Hi guys,
>>
>> I see this new test case failing on x86_64-linux-gnu:
>>
>> FAIL: gfortran.dg/pr78240.f90   -O  (test for excess errors)
>>
>>
>> $ gfortran-8 pr78240.f90
>> pr78240.f90:11:12:
>>
>>    integer x(n)    /1/   ! { dg-error "Nonconstant array" }
>>             1
>> Error: Variable ‘n’ cannot appear in the expression at (1)
>> pr78240.f90:11:14:
>>
>>    integer x(n)    /1/   ! { dg-error "Nonconstant array" }
>>               1
>> Error: The module or main program array ‘x’ at (1) must have constant shape
>> pr78240.f90:11:19:
>>
>>    integer x(n)    /1/   ! { dg-error "Nonconstant array" }
>>                    1
>> Error: Nonconstant array section at (1) in DATA statement
>> [...]
>
> ... does anyone know how to tell dejagnu to expect multiple errors on
> a single line?

I think this is covered here:

https://gcc.gnu.org/wiki/TestCaseWriting

Cheers,
Janus

Re: [PATCH] PR fortran/78240 -- kludge of the day

[Steve Kargl]

On Tue, Nov 14, 2017 at 05:21:41PM -0500, Fritz Reese wrote:
> On Tue, Nov 14, 2017 at 4:58 PM, Janus Weil <janus@gcc.gnu.org> wrote:
> > Error: The module or main program array ‘x’ at (1) must have constant shape
> > pr78240.f90:11:19:
> >
> >    integer x(n)    /1/   ! { dg-error "Nonconstant array" }
> >                    1
> > Error: Nonconstant array section at (1) in DATA statement
> > [...]
> 
> ... does anyone know how to tell dejagnu to expect multiple errors on
> a single line?

You can use an OR in the dg-error directive.

  integer x(n) /1/  ! { dg-error "module or main" | "Nonconstant array" }

You can also use a prune directive.

! { dg-prune-output "module or main" }

See https://gcc.gnu.org/wiki/TestCaseWriting and examples
in gfortran.dg.

-- 
Steve
20170425 https://www.youtube.com/watch?v=VWUpyCsUKR4
20161221 https://www.youtube.com/watch?v=IbCHE-hONow

Re: [PATCH] PR fortran/78240 -- kludge of the day

[Steve Kargl]

On Tue, Nov 14, 2017 at 05:21:41PM -0500, Fritz Reese wrote:
> On Tue, Nov 14, 2017 at 4:58 PM, Janus Weil <janus@gcc.gnu.org> wrote:
> > Hi guys,
> >
> > I see this new test case failing on x86_64-linux-gnu:
> >
> > FAIL: gfortran.dg/pr78240.f90   -O  (test for excess errors)
> >
> >
> > $ gfortran-8 pr78240.f90
> > pr78240.f90:11:12:
> >
> >    integer x(n)    /1/   ! { dg-error "Nonconstant array" }
> >             1
> > Error: Variable ‘n’ cannot appear in the expression at (1)
> > pr78240.f90:11:14:
> >
> >    integer x(n)    /1/   ! { dg-error "Nonconstant array" }
> >               1
> > Error: The module or main program array ‘x’ at (1) must have constant shape
> > pr78240.f90:11:19:
> >
> >    integer x(n)    /1/   ! { dg-error "Nonconstant array" }
> >                    1
> > Error: Nonconstant array section at (1) in DATA statement
> > [...]
> 
> ... does anyone know how to tell dejagnu to expect multiple errors on
> a single line?
> 

I've fixed the problem with this patch.

2017-11-15  Steven G. Kargl  <kargl@gcc.gnu.org>

	PR fortran/78240
	gfortran.dg/pr78240.f90: Prune run-on errors.


Index: gcc/testsuite/gfortran.dg/pr78240.f90
===================================================================
--- gcc/testsuite/gfortran.dg/pr78240.f90	(revision 254779)
+++ gcc/testsuite/gfortran.dg/pr78240.f90	(working copy)
@@ -1,4 +1,5 @@
 ! { dg-do compile }
+! { dg-options "-w" }
 !
 ! PR fortran/78240
 !
@@ -8,5 +9,7 @@
 !
 
 program p
-  integer x(n)    /1/   ! { dg-error "Nonconstant array" }
+  integer x(n)    /1/   ! { dg-error "cannot appear in the expression" }
 end
+! { dg-prune-output "module or main program" }
+! { dg-prune-output "Nonconstant array" }

-- 
Steve

Re: [PATCH] PR fortran/78240 -- kludge of the day

[Fritz Reese]

On Wed, Nov 15, 2017 at 1:13 PM, Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
> On Tue, Nov 14, 2017 at 05:21:41PM -0500, Fritz Reese wrote:
>> On Tue, Nov 14, 2017 at 4:58 PM, Janus Weil <janus@gcc.gnu.org> wrote:
>> > Hi guys,
>> >
>> > I see this new test case failing on x86_64-linux-gnu:
>> >
>> > FAIL: gfortran.dg/pr78240.f90   -O  (test for excess errors)
...
>>
>
> I've fixed the problem with this patch.
>
> 2017-11-15  Steven G. Kargl  <kargl@gcc.gnu.org>
>
>         PR fortran/78240
>         gfortran.dg/pr78240.f90: Prune run-on errors.
>
>
> Index: gcc/testsuite/gfortran.dg/pr78240.f90
> ===================================================================
> --- gcc/testsuite/gfortran.dg/pr78240.f90       (revision 254779)
> +++ gcc/testsuite/gfortran.dg/pr78240.f90       (working copy)
> @@ -1,4 +1,5 @@
>  ! { dg-do compile }
> +! { dg-options "-w" }
>  !
>  ! PR fortran/78240
>  !
> @@ -8,5 +9,7 @@
>  !
>
>  program p
> -  integer x(n)    /1/   ! { dg-error "Nonconstant array" }
> +  integer x(n)    /1/   ! { dg-error "cannot appear in the expression" }
>  end
> +! { dg-prune-output "module or main program" }
> +! { dg-prune-output "Nonconstant array" }
>
> --
> Steve


Thanks! I was planning to commit the very same.

---
Fritz Reese