From: Harald Anlauf <anlauf@gmx.de>
To: Mikael Morin <mikael@gcc.gnu.org>,
fortran@gcc.gnu.org, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] fortran: Ignore use statements on error [PR107426]
Date: Thu, 21 Mar 2024 21:59:19 +0100 [thread overview]
Message-ID: <57f0a554-c6a7-41ad-a5a4-98e49c9b1380@gmx.de> (raw)
In-Reply-To: <20240321162754.1353561-1-mikael@gcc.gnu.org>
Hi Mikael,
this looks all good to me. I wouldn't mind the minor side-effects of
better error recovery, as you are (successfully) trying hard to keep
the namespaces sane. So OK for mainline.
Thanks for the patch!
Harald
On 3/21/24 17:27, Mikael Morin wrote:
> Hello,
>
> here is a fix for an ICE caused by dangling pointers to ISO_C_BINDING's
> C_PTR symbol in the global intrinsic symbol for C_LOC.
> I tried to fix it by making the intrinsic symbol use its own copy of
> C_PTR, but it regressed heavily.
>
> Instead, I propose this which is based on a patch I attached to the PR
> one year ago. It's sufficient to remove the access to freed memory.
>
> However, an underlying problem remains that successive use-associations
> of ISO_C_BINDING's symbols in different scopes cause the return type
> of the C_LOC global intrinsic symbol to be set to the C_PTR from each
> scope successively, with the last one "winning". Not very pretty.
>
> Anyway, there are two changed messages in the testsuite as a side-effect
> of the proposed change. I regard them as acceptable, albeit slightly worse.
> No regression otherwise on x86_64-pc-linux-gnu.
> Ok for 14 master?
>
> Mikael
>
> -- >8 --
>
> This fixes an access to freed memory on the testcase from the PR.
> The problem comes from an invalid subroutine statement in an interface,
> which is ignored and causes the following statements forming the procedure
> body to be rejected. One of them use-associates the intrinsic ISO_C_BINDING
> module, which imports new symbols in a namespace that is freed at the time
> the statement is rejected. However, this creates dangling pointers as
> ISO_C_BINDING is special and its import creates a reference to the imported
> C_PTR symbol in the return type of the global intrinsic symbol for C_LOC
> (see the function create_intrinsic_function).
>
> This change saves and restores the list of use statements, so that rejected
> use statements are removed before they have a chance to be applied to the
> current namespace and create dangling pointers.
>
> PR fortran/107426
>
> gcc/fortran/ChangeLog:
>
> * gfortran.h (gfc_save_module_list, gfc_restore_old_module_list):
> New declarations.
> * module.cc (old_module_list_tail): New global variable.
> (gfc_save_module_list, gfc_restore_old_module_list): New functions.
> (gfc_use_modules): Set module_list and old_module_list_tail.
> * parse.cc (next_statement): Save module_list before doing any work.
> (reject_statement): Restore module_list to its saved value.
>
> gcc/testsuite/ChangeLog:
>
> * gfortran.dg/pr89943_3.f90: Update error pattern.
> * gfortran.dg/pr89943_4.f90: Likewise.
> * gfortran.dg/use_31.f90: New test.
> ---
> gcc/fortran/gfortran.h | 2 ++
> gcc/fortran/module.cc | 31 +++++++++++++++++++++++++
> gcc/fortran/parse.cc | 4 ++++
> gcc/testsuite/gfortran.dg/pr89943_3.f90 | 2 +-
> gcc/testsuite/gfortran.dg/pr89943_4.f90 | 2 +-
> gcc/testsuite/gfortran.dg/use_31.f90 | 25 ++++++++++++++++++++
> 6 files changed, 64 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gfortran.dg/use_31.f90
>
> diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
> index c7039730fad..fec7b53ff1a 100644
> --- a/gcc/fortran/gfortran.h
> +++ b/gcc/fortran/gfortran.h
> @@ -3926,6 +3926,8 @@ void gfc_module_done_2 (void);
> void gfc_dump_module (const char *, int);
> bool gfc_check_symbol_access (gfc_symbol *);
> void gfc_free_use_stmts (gfc_use_list *);
> +void gfc_save_module_list ();
> +void gfc_restore_old_module_list ();
> const char *gfc_dt_lower_string (const char *);
> const char *gfc_dt_upper_string (const char *);
>
> diff --git a/gcc/fortran/module.cc b/gcc/fortran/module.cc
> index d1de53cbdb4..c565b84d61b 100644
> --- a/gcc/fortran/module.cc
> +++ b/gcc/fortran/module.cc
> @@ -195,7 +195,12 @@ static const char *module_name;
> /* The name of the .smod file that the submodule will write to. */
> static const char *submodule_name;
>
> +/* The list of use statements to apply to the current namespace
> + before parsing the non-use statements. */
> static gfc_use_list *module_list;
> +/* The end of the MODULE_LIST list above at the time the recognition
> + of the current statement started. */
> +static gfc_use_list **old_module_list_tail;
>
> /* If we're reading an intrinsic module, this is its ID. */
> static intmod_id current_intmod;
> @@ -7561,6 +7566,8 @@ gfc_use_modules (void)
> gfc_use_module (module_list);
> free (module_list);
> }
> + module_list = NULL;
> + old_module_list_tail = &module_list;
> gfc_rename_list = NULL;
> }
>
> @@ -7584,6 +7591,30 @@ gfc_free_use_stmts (gfc_use_list *use_stmts)
> }
>
>
> +/* Remember the end of the MODULE_LIST list, so that the list can be restored
> + to its previous state if the current statement is erroneous. */
> +
> +void
> +gfc_save_module_list ()
> +{
> + gfc_use_list **tail = &module_list;
> + while (*tail != NULL)
> + tail = &(*tail)->next;
> + old_module_list_tail = tail;
> +}
> +
> +
> +/* Restore the MODULE_LIST list to its previous value and free the use
> + statements that are no longer part of the list. */
> +
> +void
> +gfc_restore_old_module_list ()
> +{
> + gfc_free_use_stmts (*old_module_list_tail);
> + *old_module_list_tail = NULL;
> +}
> +
> +
> void
> gfc_module_init_2 (void)
> {
> diff --git a/gcc/fortran/parse.cc b/gcc/fortran/parse.cc
> index a2bf328f681..79c810c86ba 100644
> --- a/gcc/fortran/parse.cc
> +++ b/gcc/fortran/parse.cc
> @@ -1800,6 +1800,7 @@ next_statement (void)
> locus old_locus;
>
> gfc_enforce_clean_symbol_state ();
> + gfc_save_module_list ();
>
> gfc_new_block = NULL;
>
> @@ -3104,6 +3105,9 @@ reject_statement (void)
>
> gfc_reject_data (gfc_current_ns);
>
> + /* Don't queue use-association of a module if we reject the use statement. */
> + gfc_restore_old_module_list ();
> +
> gfc_new_block = NULL;
> gfc_undo_symbols ();
> gfc_clear_warning ();
> diff --git a/gcc/testsuite/gfortran.dg/pr89943_3.f90 b/gcc/testsuite/gfortran.dg/pr89943_3.f90
> index 38b723e2458..84a9fb74741 100644
> --- a/gcc/testsuite/gfortran.dg/pr89943_3.f90
> +++ b/gcc/testsuite/gfortran.dg/pr89943_3.f90
> @@ -22,7 +22,7 @@ submodule(Foo_mod) Foo_smod
> module subroutine runFoo4C(ndim) bind(C, name="runFu") ! { dg-error "Mismatch in BIND" }
> use, intrinsic :: iso_c_binding ! { dg-error "Unexpected USE statement" }
> implicit none ! { dg-error "Unexpected IMPLICIT NONE statement" }
> - integer(c_int32_t) , intent(in) :: ndim ! { dg-error "Unexpected data declaration" }
> + integer(c_int32_t) , intent(in) :: ndim ! { dg-error "Symbol 'c_int32_t' at .1. has no IMPLICIT type" }
> end subroutine runFoo4C ! { dg-error " Expecting END SUBMODULE" }
>
> end submodule Foo_smod
> diff --git a/gcc/testsuite/gfortran.dg/pr89943_4.f90 b/gcc/testsuite/gfortran.dg/pr89943_4.f90
> index 8eba2eda171..cb955d01c88 100644
> --- a/gcc/testsuite/gfortran.dg/pr89943_4.f90
> +++ b/gcc/testsuite/gfortran.dg/pr89943_4.f90
> @@ -23,7 +23,7 @@ submodule(Foo_mod) Foo_smod
> module function runFoo4C(ndim) bind(C, name="runFu") ! { dg-error "Mismatch in BIND" }
> use, intrinsic :: iso_c_binding ! { dg-error "Unexpected USE statement in" }
> implicit none ! { dg-error "Unexpected IMPLICIT NONE statement" }
> - integer(c_int32_t) , intent(in) :: ndim ! { dg-error "Unexpected data declaration" }
> + integer(c_int32_t) , intent(in) :: ndim ! { dg-error "Symbol 'c_int32_t' at .1. has no IMPLICIT type" }
> end function runFoo4C ! { dg-error "Expecting END SUBMODULE" }
>
> end submodule Foo_smod
> diff --git a/gcc/testsuite/gfortran.dg/use_31.f90 b/gcc/testsuite/gfortran.dg/use_31.f90
> new file mode 100644
> index 00000000000..818f2e30b09
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/use_31.f90
> @@ -0,0 +1,25 @@
> +! { dg-do compile }
> +!
> +! PR fortran/107426
> +! This example used to generate an ICE, caused by the use stmt from the nested
> +! procedure declaration wrongly applying to the host procedure and overwriting
> +! the symbols there.
> +!
> +! Contributed by Gerhard Steinmetz <gscfq@t-online.de>
> +
> +module m
> +contains
> + subroutine p() bind(c)
> + use, intrinsic :: iso_c_binding
> + integer, target :: a = 1
> + type(c_ptr) :: z
> + interface
> + subroutine s(x) bind(cc) ! { dg-error "Missing closing paren" }
> + use, intrinsic :: iso_c_binding ! { dg-error "Unexpected USE statement in INTERFACE block" }
> + integer(c_int), value :: x ! { dg-error "Parameter 'c_int' at .1. has not been declared" }
> + end ! { dg-error "END INTERFACE statement expected" }
> + end interface
> + z = c_loc(a)
> + call s(z)
> + end
> +end
WARNING: multiple messages have this Message-ID
From: Harald Anlauf <anlauf@gmx.de>
To: gcc-patches@gcc.gnu.org
Cc: fortran@gcc.gnu.org
Subject: Re: [PATCH] fortran: Ignore use statements on error [PR107426]
Date: Thu, 21 Mar 2024 21:59:19 +0100 [thread overview]
Message-ID: <57f0a554-c6a7-41ad-a5a4-98e49c9b1380@gmx.de> (raw)
Message-ID: <20240321205919.BrNsSSdAiqjtNVtVRdSlLN1Ef_lRIdbn_h3dOGT_-Lg@z> (raw)
In-Reply-To: <20240321162754.1353561-1-mikael@gcc.gnu.org>
Hi Mikael,
this looks all good to me. I wouldn't mind the minor side-effects of
better error recovery, as you are (successfully) trying hard to keep
the namespaces sane. So OK for mainline.
Thanks for the patch!
Harald
On 3/21/24 17:27, Mikael Morin wrote:
> Hello,
>
> here is a fix for an ICE caused by dangling pointers to ISO_C_BINDING's
> C_PTR symbol in the global intrinsic symbol for C_LOC.
> I tried to fix it by making the intrinsic symbol use its own copy of
> C_PTR, but it regressed heavily.
>
> Instead, I propose this which is based on a patch I attached to the PR
> one year ago. It's sufficient to remove the access to freed memory.
>
> However, an underlying problem remains that successive use-associations
> of ISO_C_BINDING's symbols in different scopes cause the return type
> of the C_LOC global intrinsic symbol to be set to the C_PTR from each
> scope successively, with the last one "winning". Not very pretty.
>
> Anyway, there are two changed messages in the testsuite as a side-effect
> of the proposed change. I regard them as acceptable, albeit slightly worse.
> No regression otherwise on x86_64-pc-linux-gnu.
> Ok for 14 master?
>
> Mikael
>
> -- >8 --
>
> This fixes an access to freed memory on the testcase from the PR.
> The problem comes from an invalid subroutine statement in an interface,
> which is ignored and causes the following statements forming the procedure
> body to be rejected. One of them use-associates the intrinsic ISO_C_BINDING
> module, which imports new symbols in a namespace that is freed at the time
> the statement is rejected. However, this creates dangling pointers as
> ISO_C_BINDING is special and its import creates a reference to the imported
> C_PTR symbol in the return type of the global intrinsic symbol for C_LOC
> (see the function create_intrinsic_function).
>
> This change saves and restores the list of use statements, so that rejected
> use statements are removed before they have a chance to be applied to the
> current namespace and create dangling pointers.
>
> PR fortran/107426
>
> gcc/fortran/ChangeLog:
>
> * gfortran.h (gfc_save_module_list, gfc_restore_old_module_list):
> New declarations.
> * module.cc (old_module_list_tail): New global variable.
> (gfc_save_module_list, gfc_restore_old_module_list): New functions.
> (gfc_use_modules): Set module_list and old_module_list_tail.
> * parse.cc (next_statement): Save module_list before doing any work.
> (reject_statement): Restore module_list to its saved value.
>
> gcc/testsuite/ChangeLog:
>
> * gfortran.dg/pr89943_3.f90: Update error pattern.
> * gfortran.dg/pr89943_4.f90: Likewise.
> * gfortran.dg/use_31.f90: New test.
> ---
> gcc/fortran/gfortran.h | 2 ++
> gcc/fortran/module.cc | 31 +++++++++++++++++++++++++
> gcc/fortran/parse.cc | 4 ++++
> gcc/testsuite/gfortran.dg/pr89943_3.f90 | 2 +-
> gcc/testsuite/gfortran.dg/pr89943_4.f90 | 2 +-
> gcc/testsuite/gfortran.dg/use_31.f90 | 25 ++++++++++++++++++++
> 6 files changed, 64 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gfortran.dg/use_31.f90
>
> diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
> index c7039730fad..fec7b53ff1a 100644
> --- a/gcc/fortran/gfortran.h
> +++ b/gcc/fortran/gfortran.h
> @@ -3926,6 +3926,8 @@ void gfc_module_done_2 (void);
> void gfc_dump_module (const char *, int);
> bool gfc_check_symbol_access (gfc_symbol *);
> void gfc_free_use_stmts (gfc_use_list *);
> +void gfc_save_module_list ();
> +void gfc_restore_old_module_list ();
> const char *gfc_dt_lower_string (const char *);
> const char *gfc_dt_upper_string (const char *);
>
> diff --git a/gcc/fortran/module.cc b/gcc/fortran/module.cc
> index d1de53cbdb4..c565b84d61b 100644
> --- a/gcc/fortran/module.cc
> +++ b/gcc/fortran/module.cc
> @@ -195,7 +195,12 @@ static const char *module_name;
> /* The name of the .smod file that the submodule will write to. */
> static const char *submodule_name;
>
> +/* The list of use statements to apply to the current namespace
> + before parsing the non-use statements. */
> static gfc_use_list *module_list;
> +/* The end of the MODULE_LIST list above at the time the recognition
> + of the current statement started. */
> +static gfc_use_list **old_module_list_tail;
>
> /* If we're reading an intrinsic module, this is its ID. */
> static intmod_id current_intmod;
> @@ -7561,6 +7566,8 @@ gfc_use_modules (void)
> gfc_use_module (module_list);
> free (module_list);
> }
> + module_list = NULL;
> + old_module_list_tail = &module_list;
> gfc_rename_list = NULL;
> }
>
> @@ -7584,6 +7591,30 @@ gfc_free_use_stmts (gfc_use_list *use_stmts)
> }
>
>
> +/* Remember the end of the MODULE_LIST list, so that the list can be restored
> + to its previous state if the current statement is erroneous. */
> +
> +void
> +gfc_save_module_list ()
> +{
> + gfc_use_list **tail = &module_list;
> + while (*tail != NULL)
> + tail = &(*tail)->next;
> + old_module_list_tail = tail;
> +}
> +
> +
> +/* Restore the MODULE_LIST list to its previous value and free the use
> + statements that are no longer part of the list. */
> +
> +void
> +gfc_restore_old_module_list ()
> +{
> + gfc_free_use_stmts (*old_module_list_tail);
> + *old_module_list_tail = NULL;
> +}
> +
> +
> void
> gfc_module_init_2 (void)
> {
> diff --git a/gcc/fortran/parse.cc b/gcc/fortran/parse.cc
> index a2bf328f681..79c810c86ba 100644
> --- a/gcc/fortran/parse.cc
> +++ b/gcc/fortran/parse.cc
> @@ -1800,6 +1800,7 @@ next_statement (void)
> locus old_locus;
>
> gfc_enforce_clean_symbol_state ();
> + gfc_save_module_list ();
>
> gfc_new_block = NULL;
>
> @@ -3104,6 +3105,9 @@ reject_statement (void)
>
> gfc_reject_data (gfc_current_ns);
>
> + /* Don't queue use-association of a module if we reject the use statement. */
> + gfc_restore_old_module_list ();
> +
> gfc_new_block = NULL;
> gfc_undo_symbols ();
> gfc_clear_warning ();
> diff --git a/gcc/testsuite/gfortran.dg/pr89943_3.f90 b/gcc/testsuite/gfortran.dg/pr89943_3.f90
> index 38b723e2458..84a9fb74741 100644
> --- a/gcc/testsuite/gfortran.dg/pr89943_3.f90
> +++ b/gcc/testsuite/gfortran.dg/pr89943_3.f90
> @@ -22,7 +22,7 @@ submodule(Foo_mod) Foo_smod
> module subroutine runFoo4C(ndim) bind(C, name="runFu") ! { dg-error "Mismatch in BIND" }
> use, intrinsic :: iso_c_binding ! { dg-error "Unexpected USE statement" }
> implicit none ! { dg-error "Unexpected IMPLICIT NONE statement" }
> - integer(c_int32_t) , intent(in) :: ndim ! { dg-error "Unexpected data declaration" }
> + integer(c_int32_t) , intent(in) :: ndim ! { dg-error "Symbol 'c_int32_t' at .1. has no IMPLICIT type" }
> end subroutine runFoo4C ! { dg-error " Expecting END SUBMODULE" }
>
> end submodule Foo_smod
> diff --git a/gcc/testsuite/gfortran.dg/pr89943_4.f90 b/gcc/testsuite/gfortran.dg/pr89943_4.f90
> index 8eba2eda171..cb955d01c88 100644
> --- a/gcc/testsuite/gfortran.dg/pr89943_4.f90
> +++ b/gcc/testsuite/gfortran.dg/pr89943_4.f90
> @@ -23,7 +23,7 @@ submodule(Foo_mod) Foo_smod
> module function runFoo4C(ndim) bind(C, name="runFu") ! { dg-error "Mismatch in BIND" }
> use, intrinsic :: iso_c_binding ! { dg-error "Unexpected USE statement in" }
> implicit none ! { dg-error "Unexpected IMPLICIT NONE statement" }
> - integer(c_int32_t) , intent(in) :: ndim ! { dg-error "Unexpected data declaration" }
> + integer(c_int32_t) , intent(in) :: ndim ! { dg-error "Symbol 'c_int32_t' at .1. has no IMPLICIT type" }
> end function runFoo4C ! { dg-error "Expecting END SUBMODULE" }
>
> end submodule Foo_smod
> diff --git a/gcc/testsuite/gfortran.dg/use_31.f90 b/gcc/testsuite/gfortran.dg/use_31.f90
> new file mode 100644
> index 00000000000..818f2e30b09
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/use_31.f90
> @@ -0,0 +1,25 @@
> +! { dg-do compile }
> +!
> +! PR fortran/107426
> +! This example used to generate an ICE, caused by the use stmt from the nested
> +! procedure declaration wrongly applying to the host procedure and overwriting
> +! the symbols there.
> +!
> +! Contributed by Gerhard Steinmetz <gscfq@t-online.de>
> +
> +module m
> +contains
> + subroutine p() bind(c)
> + use, intrinsic :: iso_c_binding
> + integer, target :: a = 1
> + type(c_ptr) :: z
> + interface
> + subroutine s(x) bind(cc) ! { dg-error "Missing closing paren" }
> + use, intrinsic :: iso_c_binding ! { dg-error "Unexpected USE statement in INTERFACE block" }
> + integer(c_int), value :: x ! { dg-error "Parameter 'c_int' at .1. has not been declared" }
> + end ! { dg-error "END INTERFACE statement expected" }
> + end interface
> + z = c_loc(a)
> + call s(z)
> + end
> +end
next prev parent reply other threads:[~2024-03-21 20:59 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-21 16:27 Mikael Morin
2024-03-21 20:59 ` Harald Anlauf [this message]
2024-03-21 20:59 ` Harald Anlauf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=57f0a554-c6a7-41ad-a5a4-98e49c9b1380@gmx.de \
--to=anlauf@gmx.de \
--cc=fortran@gcc.gnu.org \
--cc=gcc-patches@gcc.gnu.org \
--cc=mikael@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).