From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ciao.gmane.io (ciao.gmane.io [116.202.254.214]) by sourceware.org (Postfix) with ESMTPS id 942C03858292 for ; Thu, 21 Mar 2024 20:59:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 942C03858292 Authentication-Results: sourceware.org; dmarc=fail (p=quarantine dis=none) header.from=gmx.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=m.gmane-mx.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 942C03858292 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=116.202.254.214 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1711054775; cv=none; b=S1W4h2Fc+qXDX/RnIR0T0pKgXIkchYKZ4jC0NTbBslSlqxQXKUgCye2owyjtrKI7VuYHdMlRWMFOOofdjVJ10MrARjtgALpcBo33WTpep/qc3XdxhO2NRODjxePF1BbzV5t1SzeZus505IODZTcDKizu7YnCd2u/KQJ5sb4w3mI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1711054775; c=relaxed/simple; bh=4nOhEcvn5PIa8pzeQ8c9f7lLR0fBcSgOVtFtT5eyNWA=; h=To:From:Subject:Date:Message-ID:Mime-Version; b=Yhlj/7DztzIjjyUSjvRhccaCeXpBNuKBaJmg19vZCvYZT9FuiE7u7AAOfTNSmU12N1MvHV9yxqdrGB/sTsBpbPz+Rb2mId4k0+0DFldTrqI2cg/83IeIp/CvSNaDgKnH7wgPhVPtddeUKuxedy4PMAuMQBYVfMT4dJIAstuacCQ= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from list by ciao.gmane.io with local (Exim 4.92) (envelope-from ) id 1rnPVY-0007YI-IH for gcc-patches@gcc.gnu.org; Thu, 21 Mar 2024 21:59:28 +0100 X-Injected-Via-Gmane: http://gmane.org/ To: gcc-patches@gcc.gnu.org From: Harald Anlauf Subject: Re: [PATCH] fortran: Ignore use statements on error [PR107426] Date: Thu, 21 Mar 2024 21:59:19 +0100 Message-ID: <57f0a554-c6a7-41ad-a5a4-98e49c9b1380@gmx.de> References: <20240321162754.1353561-1-mikael@gcc.gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit User-Agent: Mozilla Thunderbird Content-Language: en-US In-Reply-To: <20240321162754.1353561-1-mikael@gcc.gnu.org> Cc: fortran@gcc.gnu.org X-Spam-Status: No, score=-8.4 required=5.0 tests=BAYES_00,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,SPF_HELO_NONE,SPF_PASS,TXREP,URIBL_BLACK autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Message-ID: <20240321205919.BrNsSSdAiqjtNVtVRdSlLN1Ef_lRIdbn_h3dOGT_-Lg@z> 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 > + > +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