Re: [PATCH] Fortran: improve attribute conflict checking [PR93635]
Le 23/05/2024 à 09:49, Mikael Morin a écrit : Le 13/05/2024 à 09:25, Mikael Morin a écrit : Le 10/05/2024 à 21:56, Harald Anlauf a écrit : Am 10.05.24 um 21:48 schrieb Harald Anlauf: Hi Mikael, Am 10.05.24 um 11:45 schrieb Mikael Morin: Le 09/05/2024 à 22:30, Harald Anlauf a écrit : I'll stop here... Thanks. Go figure, I have no problem reproducing today. It's PR99798 (and there is even a patch for it). this patch has rotten a bit: the type of gfc_reluease_symbol has changed to bool, this can be fixed. Unfortunately, applying the patch does not remove the ICEs here... Oops, I take that back! There was an error on my side applying the patch; and now it does fix the ICEs after correcting that hickup Now the PR99798 patch is ready to be pushed, but I won't be available for a few days. We can finish our discussion on this topic afterwards. Hello, I'm coming back to this. I think either one of Steve's patch or your variant in the PR is a better fix for the ICE as a first step; they seem less fragile at least. Then we can look at a possible reordering of conflict checks as with the patch you originally submitted in this thread. Replying to myself... It's not a great plan if we want to avoid unnecessary churn in the testsuite.
Re: [PATCH] Fortran: improve attribute conflict checking [PR93635]
Le 13/05/2024 à 09:25, Mikael Morin a écrit : Le 10/05/2024 à 21:56, Harald Anlauf a écrit : Am 10.05.24 um 21:48 schrieb Harald Anlauf: Hi Mikael, Am 10.05.24 um 11:45 schrieb Mikael Morin: Le 09/05/2024 à 22:30, Harald Anlauf a écrit : I'll stop here... Thanks. Go figure, I have no problem reproducing today. It's PR99798 (and there is even a patch for it). this patch has rotten a bit: the type of gfc_reluease_symbol has changed to bool, this can be fixed. Unfortunately, applying the patch does not remove the ICEs here... Oops, I take that back! There was an error on my side applying the patch; and now it does fix the ICEs after correcting that hickup Now the PR99798 patch is ready to be pushed, but I won't be available for a few days. We can finish our discussion on this topic afterwards. Hello, I'm coming back to this. I think either one of Steve's patch or your variant in the PR is a better fix for the ICE as a first step; they seem less fragile at least. Then we can look at a possible reordering of conflict checks as with the patch you originally submitted in this thread. Mikael
Re: [PATCH] Fortran: improve attribute conflict checking [PR93635]
Le 10/05/2024 à 21:56, Harald Anlauf a écrit : Am 10.05.24 um 21:48 schrieb Harald Anlauf: Hi Mikael, Am 10.05.24 um 11:45 schrieb Mikael Morin: Le 09/05/2024 à 22:30, Harald Anlauf a écrit : I'll stop here... Thanks. Go figure, I have no problem reproducing today. It's PR99798 (and there is even a patch for it). this patch has rotten a bit: the type of gfc_reluease_symbol has changed to bool, this can be fixed. Unfortunately, applying the patch does not remove the ICEs here... Oops, I take that back! There was an error on my side applying the patch; and now it does fix the ICEs after correcting that hickup Now the PR99798 patch is ready to be pushed, but I won't be available for a few days. We can finish our discussion on this topic afterwards. We currently do not recover well from errors, and the prevention of corrupted namespaces is apparently a goal we aim at. Yes, and we are not there yet. But at least there is a sensible error message before the crash. True. But having a sensible error before ICEing does not improve user experience either. Are you planning to work again on PR99798? Cheers, Harald Cheers, Harald The patch therefore does not require any testsuite update and should not give any other surprises, so it should be very safe. The plan is also to leave the PR open for the time being. Regtesting on x86_64-pc-linux-gnu. OK for mainline? Thanks, Harald
Re: [PATCH] Fortran: improve attribute conflict checking [PR93635]
Am 10.05.24 um 21:48 schrieb Harald Anlauf: Hi Mikael, Am 10.05.24 um 11:45 schrieb Mikael Morin: Le 09/05/2024 à 22:30, Harald Anlauf a écrit : I'll stop here... Thanks. Go figure, I have no problem reproducing today. It's PR99798 (and there is even a patch for it). this patch has rotten a bit: the type of gfc_reluease_symbol has changed to bool, this can be fixed. Unfortunately, applying the patch does not remove the ICEs here... Oops, I take that back! There was an error on my side applying the patch; and now it does fix the ICEs after correcting that hickup We currently do not recover well from errors, and the prevention of corrupted namespaces is apparently a goal we aim at. Yes, and we are not there yet. But at least there is a sensible error message before the crash. True. But having a sensible error before ICEing does not improve user experience either. Are you planning to work again on PR99798? Cheers, Harald Cheers, Harald The patch therefore does not require any testsuite update and should not give any other surprises, so it should be very safe. The plan is also to leave the PR open for the time being. Regtesting on x86_64-pc-linux-gnu. OK for mainline? Thanks, Harald
Re: [PATCH] Fortran: improve attribute conflict checking [PR93635]
Hi Mikael, Am 10.05.24 um 11:45 schrieb Mikael Morin: Le 09/05/2024 à 22:30, Harald Anlauf a écrit : I'll stop here... Thanks. Go figure, I have no problem reproducing today. It's PR99798 (and there is even a patch for it). this patch has rotten a bit: the type of gfc_reluease_symbol has changed to bool, this can be fixed. Unfortunately, applying the patch does not remove the ICEs here... We currently do not recover well from errors, and the prevention of corrupted namespaces is apparently a goal we aim at. Yes, and we are not there yet. But at least there is a sensible error message before the crash. True. But having a sensible error before ICEing does not improve user experience either. Are you planning to work again on PR99798? Cheers, Harald Cheers, Harald The patch therefore does not require any testsuite update and should not give any other surprises, so it should be very safe. The plan is also to leave the PR open for the time being. Regtesting on x86_64-pc-linux-gnu. OK for mainline? Thanks, Harald
Re: [PATCH] Fortran: improve attribute conflict checking [PR93635]
Le 09/05/2024 à 22:30, Harald Anlauf a écrit : Hi Mikael, Am 09.05.24 um 21:51 schrieb Mikael Morin: Hello, Le 06/05/2024 à 21:33, Harald Anlauf a écrit : Dear all, I've been contemplating whether to submit the attached patch. It addresses an ICE-on-invalid as reported in the PR, and also fixes an accepts-invalid (see testcase), plus maybe some more, related due to incomplete checking of symbol attribute conflicts. The fix does not fully address the general issue, which is analyzed by Steve: some of the checks do depend on the selected Fortran standard, and under circumstances such as in the testcase the checking of other, standard-version-independent conflicts simply does not occur. Steve's solution would fix that, but unfortunately leads to issues with error recovery in notoriously fragile parts of the FE: e.g. testcase pr87907.f90 needs adjusting, and minor variations of it will lead to various other horrendous ICEs that remind of existing PRs where parsing or resolution goes sideways. I therefore propose a much simpler approach: move - if possible - selected of the standard-version-dependent checks after the version-independent ones. I think this could help in getting more consistent error reporting and recovery. However, I did *not* move those checks that are critical when processing interfaces. (-> pr87907.f90 / (sub)modules) Your patch looks clean, but I'm concerned that the order of the checks should be the important ones first, regardless of their standard version. I'm trying to look at the ICE caused by your other tentative patch at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93635#c6 but I can't reproduce the problem. Do you by any chance have around some of the variations causing "horrendous" ICEs? Oh, that's easy. Just move the block conf_std (allocatable, dummy, GFC_STD_F2003); conf_std (allocatable, function, GFC_STD_F2003); conf_std (allocatable, result, GFC_STD_F2003); towards the end of the gfc_check_conflict before the return true. While the error messages for the original gfortran.dg/pr87907.f90 look harmless, commenting out the main program p I get: pr87907.f90:15:18: 15 | subroutine g(x) ! { dg-error "mismatch in argument" } | 1 Error: FUNCTION attribute conflicts with SUBROUTINE attribute in 'g' at (1) f951: internal compiler error: Segmentation fault 0x13b8ec2 crash_signal ../../gcc-trunk/gcc/toplev.cc:319 0xba530e free_sym_tree ../../gcc-trunk/gcc/fortran/symbol.cc:4026 0xba5319 free_sym_tree ../../gcc-trunk/gcc/fortran/symbol.cc:4026 0xba5319 free_sym_tree ../../gcc-trunk/gcc/fortran/symbol.cc:4026 0xba5319 free_sym_tree ../../gcc-trunk/gcc/fortran/symbol.cc:4026 0xba5609 gfc_free_namespace(gfc_namespace*&) ../../gcc-trunk/gcc/fortran/symbol.cc:4168 0xba39c1 gfc_free_symbol(gfc_symbol*&) ../../gcc-trunk/gcc/fortran/symbol.cc:3173 0xba3b89 gfc_release_symbol(gfc_symbol*&) ../../gcc-trunk/gcc/fortran/symbol.cc:3216 0xba5339 free_sym_tree ../../gcc-trunk/gcc/fortran/symbol.cc:4029 0xba5609 gfc_free_namespace(gfc_namespace*&) ../../gcc-trunk/gcc/fortran/symbol.cc:4168 0xba58ef gfc_symbol_done_2() ../../gcc-trunk/gcc/fortran/symbol.cc:4236 0xb12ec8 gfc_done_2() ../../gcc-trunk/gcc/fortran/misc.cc:387 0xb4ac7f clean_up_modules ../../gcc-trunk/gcc/fortran/parse.cc:7057 0xb4af02 translate_all_program_units ../../gcc-trunk/gcc/fortran/parse.cc:7122 0xb4b735 gfc_parse_file() ../../gcc-trunk/gcc/fortran/parse.cc:7413 0xbb626f gfc_be_parse_file ../../gcc-trunk/gcc/fortran/f95-lang.cc:241 Restoring the main program but simply adding "end subroutine g" where it is naively expected gives: pr87907.f90:15:18: 15 | subroutine g(x) ! { dg-error "mismatch in argument" } | 1 Error: FUNCTION attribute conflicts with SUBROUTINE attribute in 'g' at (1) pr87907.f90:16:9: 16 | end subroutine g | 1 Error: Expecting END SUBMODULE statement at (1) pr87907.f90:20:7: 20 | use m ! { dg-error "has a type" } | 1 21 | integer :: x = 3 22 | call g(x) ! { dg-error "which is not consistent with" } | 2 Error: 'g' at (1) has a type, which is not consistent with the CALL at (2) f951: internal compiler error: in gfc_free_namespace, at fortran/symbol.cc:4164 0xba55e1 gfc_free_namespace(gfc_namespace*&) ../../gcc-trunk/gcc/fortran/symbol.cc:4164 0xba39c1 gfc_free_symbol(gfc_symbol*&) ../../gcc-trunk/gcc/fortran/symbol.cc:3173 0xba3b89 gfc_release_symbol(gfc_symbol*&) ../../gcc-trunk/gcc/fortran/symbol.cc:3216 0xba5339 free_sym_tree ../../gcc-trunk/gcc/fortran/symbol.cc:4029 0xba5609 gfc_free_namespace(gfc_namespace*&) ../../gcc-trunk/gcc/fortran/symbol.cc:4168 0xba58ef gfc_symbol_done_2() ../../gcc-trunk/gcc/fortran/symbol.c
Re: [PATCH] Fortran: improve attribute conflict checking [PR93635]
Hi Mikael, Am 09.05.24 um 21:51 schrieb Mikael Morin: Hello, Le 06/05/2024 à 21:33, Harald Anlauf a écrit : Dear all, I've been contemplating whether to submit the attached patch. It addresses an ICE-on-invalid as reported in the PR, and also fixes an accepts-invalid (see testcase), plus maybe some more, related due to incomplete checking of symbol attribute conflicts. The fix does not fully address the general issue, which is analyzed by Steve: some of the checks do depend on the selected Fortran standard, and under circumstances such as in the testcase the checking of other, standard-version-independent conflicts simply does not occur. Steve's solution would fix that, but unfortunately leads to issues with error recovery in notoriously fragile parts of the FE: e.g. testcase pr87907.f90 needs adjusting, and minor variations of it will lead to various other horrendous ICEs that remind of existing PRs where parsing or resolution goes sideways. I therefore propose a much simpler approach: move - if possible - selected of the standard-version-dependent checks after the version-independent ones. I think this could help in getting more consistent error reporting and recovery. However, I did *not* move those checks that are critical when processing interfaces. (-> pr87907.f90 / (sub)modules) Your patch looks clean, but I'm concerned that the order of the checks should be the important ones first, regardless of their standard version. I'm trying to look at the ICE caused by your other tentative patch at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93635#c6 but I can't reproduce the problem. Do you by any chance have around some of the variations causing "horrendous" ICEs? Oh, that's easy. Just move the block conf_std (allocatable, dummy, GFC_STD_F2003); conf_std (allocatable, function, GFC_STD_F2003); conf_std (allocatable, result, GFC_STD_F2003); towards the end of the gfc_check_conflict before the return true. While the error messages for the original gfortran.dg/pr87907.f90 look harmless, commenting out the main program p I get: pr87907.f90:15:18: 15 | subroutine g(x) ! { dg-error "mismatch in argument" } | 1 Error: FUNCTION attribute conflicts with SUBROUTINE attribute in 'g' at (1) f951: internal compiler error: Segmentation fault 0x13b8ec2 crash_signal ../../gcc-trunk/gcc/toplev.cc:319 0xba530e free_sym_tree ../../gcc-trunk/gcc/fortran/symbol.cc:4026 0xba5319 free_sym_tree ../../gcc-trunk/gcc/fortran/symbol.cc:4026 0xba5319 free_sym_tree ../../gcc-trunk/gcc/fortran/symbol.cc:4026 0xba5319 free_sym_tree ../../gcc-trunk/gcc/fortran/symbol.cc:4026 0xba5609 gfc_free_namespace(gfc_namespace*&) ../../gcc-trunk/gcc/fortran/symbol.cc:4168 0xba39c1 gfc_free_symbol(gfc_symbol*&) ../../gcc-trunk/gcc/fortran/symbol.cc:3173 0xba3b89 gfc_release_symbol(gfc_symbol*&) ../../gcc-trunk/gcc/fortran/symbol.cc:3216 0xba5339 free_sym_tree ../../gcc-trunk/gcc/fortran/symbol.cc:4029 0xba5609 gfc_free_namespace(gfc_namespace*&) ../../gcc-trunk/gcc/fortran/symbol.cc:4168 0xba58ef gfc_symbol_done_2() ../../gcc-trunk/gcc/fortran/symbol.cc:4236 0xb12ec8 gfc_done_2() ../../gcc-trunk/gcc/fortran/misc.cc:387 0xb4ac7f clean_up_modules ../../gcc-trunk/gcc/fortran/parse.cc:7057 0xb4af02 translate_all_program_units ../../gcc-trunk/gcc/fortran/parse.cc:7122 0xb4b735 gfc_parse_file() ../../gcc-trunk/gcc/fortran/parse.cc:7413 0xbb626f gfc_be_parse_file ../../gcc-trunk/gcc/fortran/f95-lang.cc:241 Restoring the main program but simply adding "end subroutine g" where it is naively expected gives: pr87907.f90:15:18: 15 | subroutine g(x) ! { dg-error "mismatch in argument" } | 1 Error: FUNCTION attribute conflicts with SUBROUTINE attribute in 'g' at (1) pr87907.f90:16:9: 16 | end subroutine g | 1 Error: Expecting END SUBMODULE statement at (1) pr87907.f90:20:7: 20 |use m! { dg-error "has a type" } | 1 21 |integer :: x = 3 22 |call g(x)! { dg-error "which is not consistent with" } | 2 Error: 'g' at (1) has a type, which is not consistent with the CALL at (2) f951: internal compiler error: in gfc_free_namespace, at fortran/symbol.cc:4164 0xba55e1 gfc_free_namespace(gfc_namespace*&) ../../gcc-trunk/gcc/fortran/symbol.cc:4164 0xba39c1 gfc_free_symbol(gfc_symbol*&) ../../gcc-trunk/gcc/fortran/symbol.cc:3173 0xba3b89 gfc_release_symbol(gfc_symbol*&) ../../gcc-trunk/gcc/fortran/symbol.cc:3216 0xba5339 free_sym_tree ../../gcc-trunk/gcc/fortran/symbol.cc:4029 0xba5609 gfc_free_namespace(gfc_namespace*&) ../../gcc-trunk/gcc/fortran/symbol.cc:4168 0xba58ef gfc_symbol_done_2() ../../gcc-trunk/gcc/fortran/symbol.cc:4236 0xb12ec8 gfc_done_2() ../../gcc-trunk/gcc/fortran/misc.cc:387 0xb4ac7f
Re: [PATCH] Fortran: improve attribute conflict checking [PR93635]
Hello, Le 06/05/2024 à 21:33, Harald Anlauf a écrit : Dear all, I've been contemplating whether to submit the attached patch. It addresses an ICE-on-invalid as reported in the PR, and also fixes an accepts-invalid (see testcase), plus maybe some more, related due to incomplete checking of symbol attribute conflicts. The fix does not fully address the general issue, which is analyzed by Steve: some of the checks do depend on the selected Fortran standard, and under circumstances such as in the testcase the checking of other, standard-version-independent conflicts simply does not occur. Steve's solution would fix that, but unfortunately leads to issues with error recovery in notoriously fragile parts of the FE: e.g. testcase pr87907.f90 needs adjusting, and minor variations of it will lead to various other horrendous ICEs that remind of existing PRs where parsing or resolution goes sideways. I therefore propose a much simpler approach: move - if possible - selected of the standard-version-dependent checks after the version-independent ones. I think this could help in getting more consistent error reporting and recovery. However, I did *not* move those checks that are critical when processing interfaces. (-> pr87907.f90 / (sub)modules) Your patch looks clean, but I'm concerned that the order of the checks should be the important ones first, regardless of their standard version. I'm trying to look at the ICE caused by your other tentative patch at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93635#c6 but I can't reproduce the problem. Do you by any chance have around some of the variations causing "horrendous" ICEs? The patch therefore does not require any testsuite update and should not give any other surprises, so it should be very safe. The plan is also to leave the PR open for the time being. Regtesting on x86_64-pc-linux-gnu. OK for mainline? Thanks, Harald
[PATCH] Fortran: improve attribute conflict checking [PR93635]
Dear all, I've been contemplating whether to submit the attached patch. It addresses an ICE-on-invalid as reported in the PR, and also fixes an accepts-invalid (see testcase), plus maybe some more, related due to incomplete checking of symbol attribute conflicts. The fix does not fully address the general issue, which is analyzed by Steve: some of the checks do depend on the selected Fortran standard, and under circumstances such as in the testcase the checking of other, standard-version-independent conflicts simply does not occur. Steve's solution would fix that, but unfortunately leads to issues with error recovery in notoriously fragile parts of the FE: e.g. testcase pr87907.f90 needs adjusting, and minor variations of it will lead to various other horrendous ICEs that remind of existing PRs where parsing or resolution goes sideways. I therefore propose a much simpler approach: move - if possible - selected of the standard-version-dependent checks after the version-independent ones. I think this could help in getting more consistent error reporting and recovery. However, I did *not* move those checks that are critical when processing interfaces. (-> pr87907.f90 / (sub)modules) The patch therefore does not require any testsuite update and should not give any other surprises, so it should be very safe. The plan is also to leave the PR open for the time being. Regtesting on x86_64-pc-linux-gnu. OK for mainline? Thanks, Harald From c55cb36a6ad00996b5efb33c0c5357fc5fa9919c Mon Sep 17 00:00:00 2001 From: Harald Anlauf Date: Mon, 6 May 2024 20:57:29 +0200 Subject: [PATCH] Fortran: improve attribute conflict checking [PR93635] gcc/fortran/ChangeLog: PR fortran/93635 * symbol.cc (gfc_check_conflict): Move some attribute conflict checks that depend on the selected version of the Fortran standard so that error reporting gets more consistent. gcc/testsuite/ChangeLog: PR fortran/93635 * gfortran.dg/pr93635.f90: New test. --- gcc/fortran/symbol.cc | 30 --- gcc/testsuite/gfortran.dg/pr93635.f90 | 19 + 2 files changed, 32 insertions(+), 17 deletions(-) create mode 100644 gcc/testsuite/gfortran.dg/pr93635.f90 diff --git a/gcc/fortran/symbol.cc b/gcc/fortran/symbol.cc index 8f7deac1d1e..ed17291c53e 100644 --- a/gcc/fortran/symbol.cc +++ b/gcc/fortran/symbol.cc @@ -459,22 +459,6 @@ gfc_check_conflict (symbol_attribute *attr, const char *name, locus *where) if (where == NULL) where = &gfc_current_locus; - if (attr->pointer && attr->intent != INTENT_UNKNOWN) -{ - a1 = pointer; - a2 = intent; - standard = GFC_STD_F2003; - goto conflict_std; -} - - if (attr->in_namelist && (attr->allocatable || attr->pointer)) -{ - a1 = in_namelist; - a2 = attr->allocatable ? allocatable : pointer; - standard = GFC_STD_F2003; - goto conflict_std; -} - /* Check for attributes not allowed in a BLOCK DATA. */ if (gfc_current_state () == COMP_BLOCK_DATA) { @@ -579,10 +563,12 @@ gfc_check_conflict (symbol_attribute *attr, const char *name, locus *where) return false; conf (allocatable, pointer); + + /* Moving these checks past the function/subroutine conflict check may + cause trouble with minor variations of testcase pr87907.f90. */ conf_std (allocatable, dummy, GFC_STD_F2003); conf_std (allocatable, function, GFC_STD_F2003); conf_std (allocatable, result, GFC_STD_F2003); - conf_std (elemental, recursive, GFC_STD_F2018); conf (in_common, dummy); conf (in_common, allocatable); @@ -911,6 +897,16 @@ gfc_check_conflict (symbol_attribute *attr, const char *name, locus *where) break; } + /* Conflict checks depending on the selected version of the Fortran + standard are preferably applied after standard-independent ones, so + that one gets more consistent error reporting and recovery. */ + if (attr->pointer && attr->intent != INTENT_UNKNOWN) +conf_std (pointer, intent, GFC_STD_F2003); + + conf_std (in_namelist, allocatable, GFC_STD_F2003); + conf_std (in_namelist, pointer, GFC_STD_F2003); + conf_std (elemental, recursive, GFC_STD_F2018); + return true; conflict: diff --git a/gcc/testsuite/gfortran.dg/pr93635.f90 b/gcc/testsuite/gfortran.dg/pr93635.f90 new file mode 100644 index 000..4ef33fecf2b --- /dev/null +++ b/gcc/testsuite/gfortran.dg/pr93635.f90 @@ -0,0 +1,19 @@ +! { dg-do compile } +! PR fortran/93635 +! +! Test that some attribute conflicts are properly diagnosed + +program p + implicit none + character(len=:),allocatable :: r,s + namelist /args/ r,s + equivalence(r,s) ! { dg-error "EQUIVALENCE attribute conflicts with ALLOCATABLE" } + allocate(character(len=1024) :: r) +end + +subroutine sub (p, q) + implicit none + real, pointer, intent(inout) :: p(:), q(:) + namelist /nml/ p,q + equivalence(p,q) ! { dg-error "EQUIVALENCE attribute conflicts with DUMMY" } +end -- 2.35.3