Started between 20190818 and 20190825 : $ cat z1.f90 module m stop end program p call m end $ cat z2.f90 module m continue end program p call m end $ cat z3.f90 module m do end program p call m end $ gfortran-12-20211128 -c z1.f90 z1.f90:2:4: 2 | stop | 1 Error: Unexpected STOP statement in MODULE at (1) z1.f90:5:6: 1 | module m | 2 ...... 5 | call m | 1 Error: Global name 'm' at (1) is already being used as a MODULE at (2) f951: internal compiler error: in gfc_free_namespace, at fortran/symbol.c:4039 0x87bf15 gfc_free_namespace(gfc_namespace*&) ../../gcc/fortran/symbol.c:4039 0x87c459 gfc_symbol_done_2() ../../gcc/fortran/symbol.c:4105 0x8140f8 gfc_done_2() ../../gcc/fortran/misc.c:382 0x83152a clean_up_modules ../../gcc/fortran/parse.c:6596 0x8314ec clean_up_modules ../../gcc/fortran/parse.c:6586 0x83d9d1 translate_all_program_units ../../gcc/fortran/parse.c:6661 0x83d9d1 gfc_parse_file() ../../gcc/fortran/parse.c:6925 0x88b37f gfc_be_parse_file ../../gcc/fortran/f95-lang.c:216
Started with r10-2798-ge68a35ae4a65d2b3.
Created attachment 52048 [details] don't assert() if errors have already occurred. If errors have already been emitted, it is possible that the namespaces are FUBAR. So don't assert().
GCC 10.4 is being released, retargeting bugs to GCC 10.5.
A patch to address this issue was submitted 7 months ago.
I found that the attached patch does not work. At the point of assertion many of the other functions to free memory have null pointers which leads to segfaults all along the way. The following approach appears to work, however, obviously there is a lot of memory left not freed, so we would rely on the OS to clean it up. Maybe this is ok, not sure. +++ b/gcc/fortran/symbol.cc @@ -4032,7 +4032,7 @@ void gfc_free_namespace (gfc_namespace *&ns) { gfc_namespace *p, *q; - int i; + int i, ecnt; gfc_was_finalized *f; if (ns == NULL) @@ -4042,6 +4042,12 @@ gfc_free_namespace (gfc_namespace *&ns) if (ns->refs > 0) return; + /* In the presence of errors, the namespace may be + corrupted or non-sense. Don't assert() and bail out. */ + gfc_get_errors (NULL, &ecnt); + if (ecnt > 0) + return; + gcc_assert (ns->refs == 0); gfc_free_statements (ns->code);
On Thu, Jan 26, 2023 at 02:56:02AM +0000, jvdelisle at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103506 > > --- Comment #5 from Jerry DeLisle <jvdelisle at gcc dot gnu.org> --- > I found that the attached patch does not work. At the point of assertion many > of the other functions to free memory have null pointers which leads to > segfaults all along the way. > > The following approach appears to work, however, obviously there is a lot of > memory left not freed, so we would rely on the OS to clean it up. Maybe this is > ok, not sure. > I think that it is ok. A lot of the gfortran FE code assumes that it is dealing with a conforming Fortran program. Many failing test cases are constructed from invalid Fortran code (see any of a number of Gerhard's PR), which takes torturous paths through the Fortran FE. The only other option is to check the pointers, but this is going to get ugly with some of the structs we have. Something like if (a->b->c->d) free(...) becomes if (a && a->b && a->b->c && a->b->c->d) free(...)
The only other way would be some sort of built in memory management scheme that would guarantee all "objects" are freed implicitly. Of course gfortran itself implements this type of thing as does I think C++ and Rust. Well we sure are not going to completely rewrite gfortran. Sometimes I think we just ought to accept ice-on-invalid and just toss out all those bug reports as the effort to fix them is not worth the benefit from doing so. I would like to see if we can get a list out of Bugzilla for at least the ones we have marked as ice-on-invalid and just push those priorities to the back of the line.
Doing the search in bugzilla, 137 bugs are marked as ic-on-invalid-code. I suggest we make all of these P5 or Wont fix. As my time and others is scarce, I plan to focus on the valid-code bugs. This one was a good one for me to study, but that is the only value I got out of it.
There are 162 marked as ice-on-valid-code.
On Fri, Jan 27, 2023 at 02:06:12AM +0000, jvdelisle at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103506 > > --- Comment #7 from Jerry DeLisle <jvdelisle at gcc dot gnu.org> --- > > Well we sure are not going to completely rewrite gfortran. Sometimes I think > we just ought to accept ice-on-invalid and just toss out all those bug reports > as the effort to fix them is not worth the benefit from doing so. > A number of these would go away if gfortran set -fmax-error=1 as the default.
(In reply to Jerry DeLisle from comment #8) > Doing the search in bugzilla, 137 bugs are marked as ic-on-invalid-code. I > suggest we make all of these P5 or Wont fix. Please don't make them wont-fix. An ICE is always a bug, as we used to explain everybody who asked, and I still consider it that way. If you must, make them P5, but didn't we have P5 for enhancements, or (missing/wrong) diagnostics? Also, an ICE-on-invalid input that confuses the parser points at a different part of the compiler than an ICE that happens during resolution, simplification, frontend-optimization, translation. In several cases, an ICE in one of the trans*.cc was caused by an issue much earlier in the process. One problem is that there are lots of PRs that are - either seemingly or likely - related. A good (better?) classification of bugs to find those that might be connected or near-duplicates would be helpful. I once tried to edit the summary of some bugs that were e.g. coarray-related, or OOP; not sure if that was appreciated. (We could more aggressively mark PRs as F2018 or F2023.) Also, there are several bugs pertaining only to CLASS. Some of those would be addressed along with the fix for PR106856. Tobias' patch plus some minor fixup to it seems to solve many of them. However, that patch creates a regression in one ("invalid") testcase because of a corruption of one list, leading to a segfault in restore_old_symbol. I think it is not the right way to try to make the cleanup more robust; this would only sweep the actual issues under the rug. It's the cause of corruption of lists etc. that should matter. Recovering memory or garbage collection should really be lower on our list.
(In reply to anlauf from comment #11) > (In reply to Jerry DeLisle from comment #8) > > Doing the search in bugzilla, 137 bugs are marked as ic-on-invalid-code. I > > suggest we make all of these P5 or Wont fix. > > Please don't make them wont-fix. I concur. While it is annoying to have that many bugs, everybody has their own priorities, and if somebody wants to spend their time fixing them, there is no reason not to, and certainly no reason to remove them from the search (which is what marking them as wont-fix would do). > An ICE is always a bug, as we used to explain everybody who asked, > and I still consider it that way. Agreed. > If you must, make them P5, but didn't we have P5 for enhancements, > or (missing/wrong) diagnostics? Not sure about that. I hardly look at priorites, anyway, except for the really high ones. > Also, an ICE-on-invalid input that confuses the parser points at a > different part of the compiler than an ICE that happens during resolution, > simplification, frontend-optimization, translation. > > In several cases, an ICE in one of the trans*.cc was caused by an issue > much earlier in the process. > > One problem is that there are lots of PRs that are - either seemingly or > likely - related. A good (better?) classification of bugs to find those > that might be connected or near-duplicates would be helpful. > > I once tried to edit the summary of some bugs that were e.g. coarray-related, > or OOP; not sure if that was appreciated. We have some meta-bugs for coarray-related stuff. Coarry-related bugs can be set as blocking Coarray (aka PR83700). > (We could more aggressively mark PRs as F2018 or F2023.) There is the F2018 meta-bug, aka 85836, and I have just created F2023, aka PR108577. This is probably the best way to track these PRs - just mark them as blocking the relevant standard PR. People who are interested in following those can just put themselves on the CC list. > Also, there are several bugs pertaining only to CLASS. Some of those > would be addressed along with the fix for PR106856. Tobias' patch plus > some minor fixup to it seems to solve many of them. I don't think we have a class meta-bug, but I'm not sure.
(In reply to anlauf from comment #11) > (In reply to Jerry DeLisle from comment #8) > > Doing the search in bugzilla, 137 bugs are marked as ic-on-invalid-code. I > > suggest we make all of these P5 or Wont fix. > > Please don't make them wont-fix. I would not do anything like that unilaterally. I am exploring how we can sort these things out by priority. I am also aware that different contributors may have different priorities. Regarding ice-on-invalid being possible indicators of a problem elsewhere. This very bug is one of those. I have a variation on the patch I posted in comment #5 where the initial error occurs. There is a loop in the parsing there that bothers me. I am not done yet.
This is interesting. Simply doing the following eliminates the ice. diff --git a/gcc/fortran/parse.cc b/gcc/fortran/parse.cc index 0fb19cc9f0f..a9e538cc2a1 100644 --- a/gcc/fortran/parse.cc +++ b/gcc/fortran/parse.cc @@ -6544,8 +6544,6 @@ loop: default: gfc_error ("Unexpected %s statement in MODULE at %C", gfc_ascii_statement (st)); - - error = true; reject_statement (); st = next_statement (); goto loop; I discovered this studying other code that was rejecting statements and found the following existing function: /* Generic complaint about an out of order statement. We also do whatever is necessary to clean up. */ static void unexpected_statement (gfc_statement st) { gfc_error ("Unexpected %s statement at %C", gfc_ascii_statement (st)); reject_statement (); } I tried it at the location of the patch just above and it worked. The error message is generic but no ice. In fact the only difference is setting error=true. With the generic error message there are a couple of existing test cases that don't pass but these would be minor edits. All this begs the question about why set error=true. Right after this the code on parse.cc has: /* Make sure not to free the namespace twice on error. */ if (!error) s->ns = gfc_current_ns; Well, setting it true s->ns is not getting set at all. Meaning it is left uninitialized. The comment about freeing the namespace is also bogus here. There is no freeing of anything in this chunk of code.
The master branch has been updated by Jerry DeLisle <jvdelisle@gcc.gnu.org>: https://gcc.gnu.org/g:8011fbba7baa46947341ca8069b5a327163a68d5 commit r13-5485-g8011fbba7baa46947341ca8069b5a327163a68d5 Author: Jerry DeLisle <jvdelisle@gcc.gnu.org> Date: Sat Jan 28 20:00:34 2023 -0800 ICE in gfc_free_namespace. ice-on-invalid. PR fortran/103506 gcc/fortran/ChangeLog: * parse.cc (parse_module): Remove use of a bool error value that prevented proper setting of the namespace pointer. gcc/testsuite/ChangeLog: * gfortran.dg/pr103506_1.f90: New test.
Fixed for gcc-13 so far.
The releases/gcc-12 branch has been updated by Jerry DeLisle <jvdelisle@gcc.gnu.org>: https://gcc.gnu.org/g:3d2860f933cc01668272e0aaa354aa96899b3a82 commit r12-9319-g3d2860f933cc01668272e0aaa354aa96899b3a82 Author: Jerry DeLisle <jvdelisle@gcc.gnu.org> Date: Sat Jan 28 20:00:34 2023 -0800 Fortran: ICE in gfc_free_namespace. ice-on-invalid. PR fortran/103506 gcc/fortran/ChangeLog: * parse.cc (parse_module): Remove use of a bool error value that prevented proper setting of the namespace pointer. gcc/testsuite/ChangeLog: * gfortran.dg/pr103506_1.f90: New test. (cherry picked from commit 8011fbba7baa46947341ca8069b5a327163a68d5)
GCC 10 branch is being closed.
The releases/gcc-11 branch has been updated by Jerry DeLisle <jvdelisle@gcc.gnu.org>: https://gcc.gnu.org/g:238bce471c4391c47ffd4cd3f261f40006993f0e commit r11-10936-g238bce471c4391c47ffd4cd3f261f40006993f0e Author: Jerry DeLisle <jvdelisle@gcc.gnu.org> Date: Sat Jan 28 20:00:34 2023 -0800 ICE in gfc_free_namespace. ice-on-invalid. PR fortran/103506 gcc/fortran/ChangeLog: * parse.c (parse_module): Remove use of a bool error value that prevented proper setting of the namespace pointer. gcc/testsuite/ChangeLog: * gfortran.dg/pr103506_1.f90: New test. (cherry picked from commit 8011fbba7baa46947341ca8069b5a327163a68d5)
Backports complete, closing.