* [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON @ 2015-10-01 0:06 Steve Kargl 2015-10-01 0:07 ` Steve Kargl 0 siblings, 1 reply; 9+ messages in thread From: Steve Kargl @ 2015-10-01 0:06 UTC (permalink / raw) To: fortran, gcc-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON 2015-10-01 0:06 [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON Steve Kargl @ 2015-10-01 0:07 ` Steve Kargl 2015-10-01 12:16 ` Mikael Morin 0 siblings, 1 reply; 9+ messages in thread From: Steve Kargl @ 2015-10-01 0:07 UTC (permalink / raw) To: fortran, gcc-patches [-- 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON 2015-10-01 0:07 ` Steve Kargl @ 2015-10-01 12:16 ` Mikael Morin 2015-10-01 13:29 ` Mikael Morin 0 siblings, 1 reply; 9+ messages in thread From: Mikael Morin @ 2015-10-01 12:16 UTC (permalink / raw) To: Steve Kargl, fortran, gcc-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON 2015-10-01 12:16 ` Mikael Morin @ 2015-10-01 13:29 ` Mikael Morin 2015-10-01 16:30 ` Steve Kargl 0 siblings, 1 reply; 9+ messages in thread From: Mikael Morin @ 2015-10-01 13:29 UTC (permalink / raw) To: Steve Kargl, fortran, gcc-patches [-- 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON 2015-10-01 13:29 ` Mikael Morin @ 2015-10-01 16:30 ` Steve Kargl 2015-10-02 9:28 ` Mikael Morin 0 siblings, 1 reply; 9+ messages in thread From: Steve Kargl @ 2015-10-01 16:30 UTC (permalink / raw) To: Mikael Morin; +Cc: fortran, gcc-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON 2015-10-01 16:30 ` Steve Kargl @ 2015-10-02 9:28 ` Mikael Morin 2015-10-02 16:44 ` Steve Kargl 0 siblings, 1 reply; 9+ messages in thread From: Mikael Morin @ 2015-10-02 9:28 UTC (permalink / raw) To: Steve Kargl; +Cc: fortran, gcc-patches 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) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON 2015-10-02 9:28 ` Mikael Morin @ 2015-10-02 16:44 ` Steve Kargl 2015-10-03 10:13 ` Mikael Morin 0 siblings, 1 reply; 9+ messages in thread From: Steve Kargl @ 2015-10-02 16:44 UTC (permalink / raw) To: Mikael Morin; +Cc: fortran, gcc-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON 2015-10-02 16:44 ` Steve Kargl @ 2015-10-03 10:13 ` Mikael Morin 2015-10-03 19:09 ` Steve Kargl 0 siblings, 1 reply; 9+ messages in thread From: Mikael Morin @ 2015-10-03 10:13 UTC (permalink / raw) To: Steve Kargl; +Cc: fortran, gcc-patches [-- 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON 2015-10-03 10:13 ` Mikael Morin @ 2015-10-03 19:09 ` Steve Kargl 0 siblings, 0 replies; 9+ messages in thread From: Steve Kargl @ 2015-10-03 19:09 UTC (permalink / raw) To: Mikael Morin; +Cc: fortran, gcc-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-10-03 19:09 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-01 0:06 [PATCH] fortran/67758 -- Prevent ICE caused by misplaced COMMON Steve Kargl 2015-10-01 0:07 ` Steve Kargl 2015-10-01 12:16 ` Mikael Morin 2015-10-01 13:29 ` Mikael Morin 2015-10-01 16:30 ` Steve Kargl 2015-10-02 9:28 ` Mikael Morin 2015-10-02 16:44 ` Steve Kargl 2015-10-03 10:13 ` Mikael Morin 2015-10-03 19:09 ` Steve Kargl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).