Re: [PATCH] fortran: Ignore use statements on error [PR107426]

[Harald Anlauf <anlauf@gmx.de>]

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