[PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON

[PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON

[Steve Kargl]

Patch built and regression tested on x86_64-*-freebsd.
OK to commit?

The patch prevents the dereferencing of a NULL pointer
by jumping out of the cleanup of a list of COMMON blocks.

2015-09-30  Steven G. Kargl  <kargl@gcc.gnu.org>

	* symbol.c (gfc_restore_last_undo_checkpoint): Prevent ICE during
	cleanup of a misplaced COMMON.

2015-09-30  Steven G. Kargl  <kargl@gcc.gnu.org>

	* gfortran.dg/pr67758.f: New test.

-- 
Steve

Re: [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON

[Steve Kargl]

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

On Wed, Sep 30, 2015 at 05:06:30PM -0700, Steve Kargl wrote:
> Patch built and regression tested on x86_64-*-freebsd.
> OK to commit?
> 
> The patch prevents the dereferencing of a NULL pointer
> by jumping out of the cleanup of a list of COMMON blocks.
> 
> 2015-09-30  Steven G. Kargl  <kargl@gcc.gnu.org>
> 
> 	* symbol.c (gfc_restore_last_undo_checkpoint): Prevent ICE during
> 	cleanup of a misplaced COMMON.
> 
> 2015-09-30  Steven G. Kargl  <kargl@gcc.gnu.org>
> 
> 	* gfortran.dg/pr67758.f: New test.
> 

Now with the patch attached!

-- 
Steve

[-- Attachment #2: pr67758.diff --]
[-- Type: text/x-diff, Size: 1022 bytes --]

Index: fortran/symbol.c
===================================================================
--- fortran/symbol.c	(revision 228306)
+++ fortran/symbol.c	(working copy)
@@ -3211,6 +3211,11 @@ gfc_restore_last_undo_checkpoint (void)
 
 	      while (csym != p)
 		{
+		  if (!csym)
+		    {
+		      gfc_error ("Unexpected COMMON at %C");
+		      goto error;
+		    }
 		  cparent = csym;
 		  csym = csym->common_next;
 		}
@@ -3237,6 +3242,8 @@ gfc_restore_last_undo_checkpoint (void)
 	restore_old_symbol (p);
     }
 
+error:
+
   latest_undo_chgset->syms.truncate (0);
   latest_undo_chgset->tbps.truncate (0);
 
Index: testsuite/gfortran.dg/pr67758.f
===================================================================
--- testsuite/gfortran.dg/pr67758.f	(revision 0)
+++ testsuite/gfortran.dg/pr67758.f	(working copy)
@@ -0,0 +1,6 @@
+c { dg-do compile }
+c PR fortran/67758
+      COMMON /FMCOM / X(80 000 000)
+      CALL T(XX(A))
+      COMMON /FMCOM / XX(80 000 000) ! { dg-error "Unexpected COMMON" }
+      END

Re: [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON

[Mikael Morin]

Le 01/10/2015 02:07, Steve Kargl a écrit :
> On Wed, Sep 30, 2015 at 05:06:30PM -0700, Steve Kargl wrote:
>> Patch built and regression tested on x86_64-*-freebsd.
>> OK to commit?
>>
>> The patch prevents the dereferencing of a NULL pointer
>> by jumping out of the cleanup of a list of COMMON blocks.
>>
Hold on, I believe p should be present in the common symbol list pointed 
by p->common.
And by the way, if we are in gfc_restore_last_undo_checkpoint, we have 
found something bogus enough to backtrack, so hopefully an error has 
already been prepared (but maybe not emitted).
I will investigate more.

Mikael

Re: [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON

[Mikael Morin]

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

Le 01/10/2015 14:16, Mikael Morin a écrit :
> Le 01/10/2015 02:07, Steve Kargl a écrit :
>> On Wed, Sep 30, 2015 at 05:06:30PM -0700, Steve Kargl wrote:
>>> Patch built and regression tested on x86_64-*-freebsd.
>>> OK to commit?
>>>
>>> The patch prevents the dereferencing of a NULL pointer
>>> by jumping out of the cleanup of a list of COMMON blocks.
>>>
> Hold on, I believe p should be present in the common symbol list pointed
> by p->common.
s/p->common/p->common_block/
> And by the way, if we are in gfc_restore_last_undo_checkpoint, we have
> found something bogus enough to backtrack, so hopefully an error has
> already been prepared (but maybe not emitted).
> I will investigate more.
>
It seems the error [1] is reported in gfc_add_in_common, between the 
time the symbol's common_block pointer is set and the time the symbol is 
added to the list.
As the program goes straight to clean-up/return upon error, this interim 
state is not fixed and poses problem.

So we need to reduce the interim time to zero or fix the state upon error.
I propose the following, which delays setting the common_block after 
error_checking (I believe it is not used in that time).

Regression-tested on x86_64-unknown-linux-gnu. OK for trunk?

Mikael


[1] Error: PROCEDURE attribute conflicts with COMMON attribute in 'xx' 
at (1)


[-- Attachment #2: pr67758_v2.CL --]
[-- Type: text/plain, Size: 262 bytes --]

2015-10-01  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/67758
	* match.c (gfc_match_common): Delay the common_block pointer
	assignment after error checking.

2015-10-01  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/67758
	* gfortran.dg/common_24.f: New.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: pr67758_v2.diff --]
[-- Type: text/x-patch; name="pr67758_v2.diff", Size: 935 bytes --]

Index: match.c
===================================================================
--- match.c	(révision 228170)
+++ match.c	(copie de travail)
@@ -4330,10 +4330,6 @@ gfc_match_common (void)
 	  if (m == MATCH_NO)
 	    goto syntax;
 
-          /* Store a ref to the common block for error checking.  */
-          sym->common_block = t;
-          sym->common_block->refs++;
-
           /* See if we know the current common block is bind(c), and if
              so, then see if we can check if the symbol is (which it'll
              need to be).  This can happen if the bind(c) attr stmt was
@@ -4379,6 +4375,10 @@ gfc_match_common (void)
 	  if (!gfc_add_in_common (&sym->attr, sym->name, NULL))
 	    goto cleanup;
 
+          /* Store a ref to the common block for error checking.  */
+          sym->common_block = t;
+          sym->common_block->refs++;
+
 	  if (tail != NULL)
 	    tail->common_next = sym;
 	  else


[-- Attachment #4: common_24.f --]
[-- Type: text/x-fortran, Size: 302 bytes --]

c { dg-do compile }
c PR fortran/67758
c
c Check the absence of ICE after emitting the error message
c
c Contributed by Ilya Enkovich <ienkovich@gcc.gnu.org>

      COMMON /FMCOM / X(80 000 000)
      CALL T(XX(A))
      COMMON /FMCOM / XX(80 000 000) ! { dg-error "conflicts with COMMON" }
      END

Re: [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON

[Steve Kargl]

On Thu, Oct 01, 2015 at 03:29:05PM +0200, Mikael Morin wrote:
> Le 01/10/2015 14:16, Mikael Morin a écrit :
> > Le 01/10/2015 02:07, Steve Kargl a écrit :
> >> On Wed, Sep 30, 2015 at 05:06:30PM -0700, Steve Kargl wrote:
> >>> Patch built and regression tested on x86_64-*-freebsd.
> >>> OK to commit?
> >>>
> >>> The patch prevents the dereferencing of a NULL pointer
> >>> by jumping out of the cleanup of a list of COMMON blocks.
> >>>
> > Hold on, I believe p should be present in the common symbol list pointed
> > by p->common.
> s/p->common/p->common_block/
> > And by the way, if we are in gfc_restore_last_undo_checkpoint, we have
> > found something bogus enough to backtrack, so hopefully an error has
> > already been prepared (but maybe not emitted).
> > I will investigate more.
> >
> It seems the error [1] is reported in gfc_add_in_common, between the 
> time the symbol's common_block pointer is set and the time the symbol is 
> added to the list.
> As the program goes straight to clean-up/return upon error, this interim 
> state is not fixed and poses problem.
> 
> So we need to reduce the interim time to zero or fix the state upon error.
> I propose the following, which delays setting the common_block after 
> error_checking (I believe it is not used in that time).
> 
> Regression-tested on x86_64-unknown-linux-gnu. OK for trunk?
> 

I'm fine with your patch, although I find the error message
to be somewhat confusing as no procedure appears in COMMON.
The call-stmt in the code is the start of an execution-construct.
A common-stmt is not allowed in an execution-construct.  At
least, that's how I intepret the BNF in 2.1 of F2008.

-- 
Steve

Re: [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON

[Mikael Morin]

Le 01/10/2015 18:30, Steve Kargl a écrit :
> I'm fine with your patch, although I find the error message
> to be somewhat confusing as no procedure appears in COMMON.

Well, XX is implicitly a procedure.

> The call-stmt in the code is the start of an execution-construct.
> A common-stmt is not allowed in an execution-construct.  At
> least, that's how I intepret the BNF in 2.1 of F2008.
>
The error message appears too soon, before we finish parsing the common 
statement.  If it's delayed, as with the following additional patch, the 
common statements is properly rejected:

common_24.f:10:72:

        COMMON /FMCOM / XX(80 000 000) ! { dg-error "conflicts with 
COMMON" }
                                                                         1
Error: Unexpected COMMON statement at (1)
common_24.f:8:72:

Error: PROCEDURE attribute conflicts with COMMON attribute in ‘xx’ at (1)

This needs a little more polishing (location missing in the second error 
message), then let's see how the testsuite likes it.

Mikael


Index: match.c
===================================================================
--- match.c	(révision 228170)
+++ match.c	(copie de travail)
@@ -4376,9 +4376,6 @@ gfc_match_common (void)
  		goto cleanup;
  	    }

-	  if (!gfc_add_in_common (&sym->attr, sym->name, NULL))
-	    goto cleanup;
-
  	  if (tail != NULL)
  	    tail->common_next = sym;
  	  else
Index: resolve.c
===================================================================
--- resolve.c	(révision 228170)
+++ resolve.c	(copie de travail)
@@ -918,6 +918,9 @@ resolve_common_vars (gfc_symbol *sym, bool named_c

    for (; csym; csym = csym->common_next)
      {
+      if (!gfc_add_in_common (&csym->attr, csym->name, NULL))
+	continue;
+
        if (csym->value || csym->attr.data)
  	{
  	  if (!csym->ns->is_block_data)

Re: [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON

[Steve Kargl]

On Fri, Oct 02, 2015 at 11:28:06AM +0200, Mikael Morin wrote:
> Le 01/10/2015 18:30, Steve Kargl a écrit :
> > I'm fine with your patch, although I find the error message
> > to be somewhat confusing as no procedure appears in COMMON.
> 
> Well, XX is implicitly a procedure.
> 

Yes, I understamf what gfortran is doing.  However, I
less fluent Fortran programmer who expects XX to be 
an array may be confused by error message.

> > The call-stmt in the code is the start of an execution-construct.
> > A common-stmt is not allowed in an execution-construct.  At
> > least, that's how I intepret the BNF in 2.1 of F2008.
> >
> The error message appears too soon, before we finish parsing the common 
> statement.  If it's delayed, as with the following additional patch, the 
> common statements is properly rejected:
> 
> common_24.f:10:72:
> 
>         COMMON /FMCOM / XX(80 000 000) ! { dg-error "conflicts with 
> COMMON" }
>                                                                          1
> Error: Unexpected COMMON statement at (1)
> common_24.f:8:72:
> 
> Error: PROCEDURE attribute conflicts with COMMON attribute in ???xx??? at (1)
> 
> This needs a little more polishing (location missing in the second error 
> message), then let's see how the testsuite likes it.
> 

While I prefer the first error message above, if it requires
too much polish, then at least commit your first patch to cure
the ICE.  We can worry about polish later.

-- 
Steve

Re: [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON

[Mikael Morin]

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

Le 02/10/2015 18:44, Steve Kargl a écrit :
> On Fri, Oct 02, 2015 at 11:28:06AM +0200, Mikael Morin wrote:
>> Le 01/10/2015 18:30, Steve Kargl a écrit :
>>> The call-stmt in the code is the start of an execution-construct
>>> A common-stmt is not allowed in an execution-construct.  At
>>> least, that's how I intepret the BNF in 2.1 of F2008.
>>>
>> The error message appears too soon, before we finish parsing the common
>> statement.  If it's delayed, as with the following additional patch, the
>> common statements is properly rejected:
>>
>> common_24.f:10:72:
>>
>>          COMMON /FMCOM / XX(80 000 000) ! { dg-error "conflicts with
>> COMMON" }
>>                                                                           1
>> Error: Unexpected COMMON statement at (1)
>> common_24.f:8:72:
>>
>> Error: PROCEDURE attribute conflicts with COMMON attribute in ???xx??? at (1)
>>
>> This needs a little more polishing (location missing in the second error
>> message), then let's see how the testsuite likes it.
>>
>
> While I prefer the first error message above, if it requires
> too much polish, then at least commit your first patch to cure
> the ICE.  We can worry about polish later.
>
I have finally managed to find a patch that doesn't regress in the 
testsuite.

The gfc_add_in_common call in gfc_match_common is delayed after the 
array spec handling and without return value check, so that errors are 
ignored.  Another gfc_add_in_common call is necessary to report errors 
again during resolution.  This is patch number 2.
The error location for the second call is grabbed from the common block 
struct, which is made accessible in the function by patch number 1.

No regression on x86-unknown-linux-gnu, OK for trunk?

Mikael


[-- Attachment #2: pr67758_v6-1.diff --]
[-- Type: text/x-patch, Size: 1408 bytes --]

2015-10-02  Mikael Morin  <mikael@gcc.gnu.org>

	* resolve.c (resolve_common_vars): Move access to the common
	block's head symbol inside the function.
	(resolve_common_blocks, resolve_types): Update callers.
	  
diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index 7363e06..582c3f9 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -912,9 +912,9 @@ resolve_entries (gfc_namespace *ns)
 
 /* Resolve common variables.  */
 static void
-resolve_common_vars (gfc_symbol *sym, bool named_common)
+resolve_common_vars (gfc_common_head *common_block, bool named_common)
 {
-  gfc_symbol *csym = sym;
+  gfc_symbol *csym = common_block->head;
 
   for (; csym; csym = csym->common_next)
     {
@@ -972,7 +972,7 @@ resolve_common_blocks (gfc_symtree *common_root)
   if (common_root->right)
     resolve_common_blocks (common_root->right);
 
-  resolve_common_vars (common_root->n.common->head, true);
+  resolve_common_vars (common_root->n.common, true);
 
   /* The common name is a global name - in Fortran 2003 also if it has a
      C binding name, since Fortran 2008 only the C binding name is a global
@@ -15202,7 +15202,7 @@ resolve_types (gfc_namespace *ns)
 
   resolve_entries (ns);
 
-  resolve_common_vars (ns->blank_common.head, false);
+  resolve_common_vars (&ns->blank_common, false);
   resolve_common_blocks (ns->common_root);
 
   resolve_contained_functions (ns);



[-- Attachment #3: pr67758_v6-2.diff --]
[-- Type: text/x-patch, Size: 2740 bytes --]

2015-10-02  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/67758
	* match.c (gfc_match_common): Delay the common_block pointer
	assignment after error checking.
	Delay the call to gfc_add_in_common attribute after the handling
	of array specs.
	* resolve.c (resolve_common_vars): Call gfc_add_in_common again.

2015-10-02  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/67758
	* gfortran.dg/common_24.f: New.

diff --git a/gcc/fortran/match.c b/gcc/fortran/match.c
index 523e9b2..a63c731 100644
--- a/gcc/fortran/match.c
+++ b/gcc/fortran/match.c
@@ -4330,10 +4330,6 @@ gfc_match_common (void)
 	  if (m == MATCH_NO)
 	    goto syntax;
 
-          /* Store a ref to the common block for error checking.  */
-          sym->common_block = t;
-          sym->common_block->refs++;
-
           /* See if we know the current common block is bind(c), and if
              so, then see if we can check if the symbol is (which it'll
              need to be).  This can happen if the bind(c) attr stmt was
@@ -4376,8 +4372,8 @@ gfc_match_common (void)
 		goto cleanup;
 	    }
 
-	  if (!gfc_add_in_common (&sym->attr, sym->name, NULL))
-	    goto cleanup;
+	  sym->common_block = t;
+	  sym->common_block->refs++;
 
 	  if (tail != NULL)
 	    tail->common_next = sym;
@@ -4416,6 +4412,10 @@ gfc_match_common (void)
 
 	    }
 
+	  /* Add the in_common attribute, but ignore the reported errors
+	     if any, and continue matching.  */
+	  gfc_add_in_common (&sym->attr, sym->name, NULL);
+
 	  sym->common_head = t;
 
 	  /* Check to see if the symbol is already in an equivalence group.
diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index 582c3f9..6e61250 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -918,6 +918,12 @@ resolve_common_vars (gfc_common_head *common_block, bool named_common)
 
   for (; csym; csym = csym->common_next)
     {
+      /* gfc_add_in_common may have been called before, but the reported errors
+	 have been ignored to continue parsing.
+	 We do the checks again here.  */
+      if (!csym->attr.use_assoc)
+	gfc_add_in_common (&csym->attr, csym->name, &common_block->where);
+
       if (csym->value || csym->attr.data)
 	{
 	  if (!csym->ns->is_block_data)
diff --git a/gcc/testsuite/gfortran.dg/common_24.f b/gcc/testsuite/gfortran.dg/common_24.f
new file mode 100644
index 0000000..ea37c2a
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/common_24.f
@@ -0,0 +1,11 @@
+c { dg-do compile }
+c PR fortran/67758
+c
+c Check the absence of ICE after emitting the error message
+c
+c Contributed by Ilya Enkovich <ienkovich@gcc.gnu.org>
+
+      COMMON /FMCOM / X(80 000 000)
+      CALL T(XX(A))
+      COMMON /FMCOM / XX(80 000 000) ! { dg-error "Unexpected COMMON" }
+      END

Re: [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON

[Steve Kargl]

On Sat, Oct 03, 2015 at 12:13:12PM +0200, Mikael Morin wrote:
>
> I have finally managed to find a patch that doesn't regress in the 
> testsuite.
> 
> The gfc_add_in_common call in gfc_match_common is delayed after the 
> array spec handling and without return value check, so that errors are 
> ignored.  Another gfc_add_in_common call is necessary to report errors 
> again during resolution.  This is patch number 2.
> The error location for the second call is grabbed from the common block 
> struct, which is made accessible in the function by patch number 1.
> 
> No regression on x86-unknown-linux-gnu, OK for trunk?
> 

The patch is OK.  Thanks for taking over PR.

-- 
Steve