From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.gmx.net (mout.gmx.net [212.227.15.15]) by sourceware.org (Postfix) with ESMTPS id 359633858D38; Mon, 11 Sep 2023 18:50:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 359633858D38 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmx.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmx.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.de; s=s31663417; t=1694458248; x=1695063048; i=anlauf@gmx.de; bh=3XcRTfkrOGnK0if7U+EFZJE7lnji/GCvIZrUWpKARZc=; h=X-UI-Sender-Class:Date:Subject:To:References:From:In-Reply-To; b=Nsr+m0llR3HVt6QXE2fr5hEOwKvSuhsVVu3x9HabNFxj6rXgT6lniVdAdaHEq5I7gbOgFN1 Z08oDKbPIiilk57zxg6pw0CYvKizW1wOM++JvU+OL+csuQGaTwDIBfWdvVS/HBsHp3fJV/lF9 rXCz0wlo/RsNJtli9HVgW2x7m+1/xXr9zWNsJGtyd4QpDV15H3PnAnlaeis8zzhfYdLhzOcxS ifcEuSRb9/eBm17hm5ehkDneSJIKCexdUxePuzWA8Ss5p95pLdAgQsYfueOV3t9nDfqgpp/Bb NN/grVTRhuVMtuEyxPQGVnf6UuIzP5N6DRYPGxCBxwCbT3rFOtjQ== X-UI-Sender-Class: 724b4f7f-cbec-4199-ad4e-598c01a50d3a Received: from [192.168.178.29] ([79.232.145.141]) by mail.gmx.net (mrgmx004 [212.227.17.190]) with ESMTPSA (Nemesis) id 1MsHns-1pmJ1p3SIM-00timH; Mon, 11 Sep 2023 20:50:47 +0200 Message-ID: <364c6a4a-b06d-41a0-8cdb-c446221672ee@gmx.de> Date: Mon, 11 Sep 2023 20:50:47 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] fortran: Undo new symbols in all namespaces [PR110996] To: Mikael Morin , gcc-patches@gcc.gnu.org, fortran@gcc.gnu.org Newsgroups: gmane.comp.gcc.patches,gmane.comp.gcc.fortran References: <20230911123845.1870133-1-mikael@gcc.gnu.org> Content-Language: en-US From: Harald Anlauf In-Reply-To: <20230911123845.1870133-1-mikael@gcc.gnu.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-Provags-ID: V03:K1:/YfXCNsOs+RcelNo979kpWF2x4VN0ySbarPQ94R+TZ47iaf/p7P 0YJjMqDMhg1eN3nWMD1ipzjqYrtDOlQWUowbzpDSBZM5T9bQa1jUMnTjvYB+SkZwHr60i3S PmZAoe3xSh/LfBZq2BnAVzAWPXk0ojYZMXtowrcm5M/SpdWjWRtBPxT+qACdbf4scRxI8zZ HM1RUq/eL051Srk83xcug== UI-OutboundReport: notjunk:1;M01:P0:LNeVU5cHm58=;SjM5Xq7cQQKBtnC3oembG6j5zML XneBwkiigV5dIogqh7XYQsw/wCvTtgGsBc3VUyuy6ymnqJUWpM1ou7Hpv3EXJHa+TtsQLOzFf x/5Nu+asXGpTEePbf8TyRhDDWbbDKLMpNhP3iyNV0znPZOFKJiwKa10mQHARLHdXXItHxIkhZ ngrWKk7lEzRyRkEwa+R3vUBinX1zpfTX+7ol/oKc0oI0KGxQAKQ4ZVeJC91OmmEDeWkLFFVMh ls5XuZJEILvfPMYSHZPKE5cluIJ8WIzv7CIgE6IK/kAx7oISEj1SLrpWIato38tru4wV67UEz iAs3n8iZuIZbAN6lsFyWLdOzmDOWgKLT5R7zF+GkTZXIwbhYSPQ/4aTWlqGqiLjKPnzPtQk91 aV+BvqwVOhIsFGyrtsXnxbyMtARHP3DeLF9BYMM+6g3Dc6ipDbanGL77d5fj3Mlp+V70+ApL+ 4L6rrFyY5cQY/nLrgQ0ukypG/zjiqZskUfDjN7Q6GlewTawQHvwXNjdSPWHU9a0aAH02TgGHu HG4EkVk4ggSzjsHmczRMzPlFpV72eqiA0yWXqfiMrSOSuCPh0yC4STpfGnMaxCcVEAbvvT9GR Vf6M1NMgF/zrgCoTcGj0aBCEYbaCPsOCHtYm1K0v6GJcT/Cwa81AeFiJoDI0hZNHsLEbJjmr0 k52ewV43e3sGnt7hOagZpVB9iGHlmoyuAewl+xY0xxRApLPkNVexiP85SxL7Pyv6TYjwvGAhw l1oad/GAwkKGauCOXjTiRm3877+sRoqnDkhR+z2hlKJ++M2D5Qgppl/JAl56q/Ywt7GourXjF sQGXGOMbQ1op2aCU5N1/cspUQdYhjx4AHb51orJ0Ld8GvM3fijFqK3uynp+qs8kI9vZxvSQnP zoBfYAAZOpmQ7mpq6xSi8ldIEzXjIfJV2V3KxjFEtH7vte7t7S4EVR9mpcefJyJUddyAZLzHU fm9WKp6rF6UJAGADop5IRlXZcDY= X-Spam-Status: No, score=-10.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,TXREP 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: Hi Mikael, On 9/11/23 14:38, Mikael Morin via Gcc-patches wrote: > Hello, > > this fixes a memory management issue in the fortran frontend. > I have included the (reduced) testcase from the PR, even if it wasn't fa= iling > here on x86_64 with the test harness. > Tested on x86_64-pc-linux-gnu and manually checked the testcase with > valgrind. > OK for master? nice fix! This is OK for mainline. Thanks for the patch! Harald > -- >8 -- > > Remove new symbols from all namespaces they may have been added to in ca= se a > statement mismatches in the end and all the symbols referenced in it hav= e to > be reverted. > > This fixes memory errors and random internal compiler errors caused by > a new symbol left with dangling pointers but not properly removed from t= he > namespace at statement rejection. > > Before this change, new symbols were removed from their own namespace > (their ns field) only. This change additionally removes them from the > namespaces pointed to by respectively the gfc_current_ns global variable= , > and the symbols' formal_ns field. > > PR fortran/110996 > > gcc/fortran/ChangeLog: > > * gfortran.h (gfc_release_symbol): Set return type to bool. > * symbol.cc (gfc_release_symbol): Ditto. Return whether symbol was > freed. > (delete_symbol_from_ns): New, outline code from... > (gfc_restore_last_undo_checkpoint): ... here. Delete new symbols > from two more namespaces. > > gcc/testsuite/ChangeLog: > > * gfortran.dg/pr110996.f90: New test. > --- > gcc/fortran/gfortran.h | 2 +- > gcc/fortran/symbol.cc | 57 ++++++++++++++++++++------ > gcc/testsuite/gfortran.dg/pr110996.f90 | 16 ++++++++ > 3 files changed, 62 insertions(+), 13 deletions(-) > create mode 100644 gcc/testsuite/gfortran.dg/pr110996.f90 > > diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h > index 371f8743312..f4a1c106cea 100644 > --- a/gcc/fortran/gfortran.h > +++ b/gcc/fortran/gfortran.h > @@ -3514,7 +3514,7 @@ gfc_symtree *gfc_get_unique_symtree (gfc_namespace= *); > gfc_user_op *gfc_get_uop (const char *); > gfc_user_op *gfc_find_uop (const char *, gfc_namespace *); > void gfc_free_symbol (gfc_symbol *&); > -void gfc_release_symbol (gfc_symbol *&); > +bool gfc_release_symbol (gfc_symbol *&); > gfc_symbol *gfc_new_symbol (const char *, gfc_namespace *); > gfc_symtree* gfc_find_symtree_in_proc (const char *, gfc_namespace *); > int gfc_find_symbol (const char *, gfc_namespace *, int, gfc_symbol **= ); > diff --git a/gcc/fortran/symbol.cc b/gcc/fortran/symbol.cc > index 2cba2ea0bed..a6078bc608a 100644 > --- a/gcc/fortran/symbol.cc > +++ b/gcc/fortran/symbol.cc > @@ -3105,13 +3105,14 @@ gfc_free_symbol (gfc_symbol *&sym) > } > > > -/* Decrease the reference counter and free memory when we reach zero. = */ > +/* Decrease the reference counter and free memory when we reach zero. > + Returns true if the symbol has been freed, false otherwise. */ > > -void > +bool > gfc_release_symbol (gfc_symbol *&sym) > { > if (sym =3D=3D NULL) > - return; > + return false; > > if (sym->formal_ns !=3D NULL && sym->refs =3D=3D 2 && sym->formal_ns= !=3D sym->ns > && (!sym->attr.entry || !sym->module)) > @@ -3125,10 +3126,11 @@ gfc_release_symbol (gfc_symbol *&sym) > > sym->refs--; > if (sym->refs > 0) > - return; > + return false; > > gcc_assert (sym->refs =3D=3D 0); > gfc_free_symbol (sym); > + return true; > } > > > @@ -3649,6 +3651,29 @@ gfc_drop_last_undo_checkpoint (void) > } > > > +/* Remove the reference to the symbol SYM in the symbol tree held by NS > + and free SYM if the last reference to it has been removed. > + Returns whether the symbol has been freed. */ > + > +static bool > +delete_symbol_from_ns (gfc_symbol *sym, gfc_namespace *ns) > +{ > + if (ns =3D=3D nullptr) > + return false; > + > + /* The derived type is saved in the symtree with the first > + letter capitalized; the all lower-case version to the > + derived type contains its associated generic function. */ > + const char *sym_name =3D gfc_fl_struct (sym->attr.flavor) > + ? gfc_dt_upper_string (sym->name) > + : sym->name; > + > + gfc_delete_symtree (&ns->sym_root, sym_name); > + > + return gfc_release_symbol (sym); > +} > + > + > /* Undoes all the changes made to symbols since the previous checkpoin= t. > This subroutine is made simpler due to the fact that attributes are > never removed once added. */ > @@ -3703,15 +3728,23 @@ gfc_restore_last_undo_checkpoint (void) > } > if (p->gfc_new) > { > - /* The derived type is saved in the symtree with the first > - letter capitalized; the all lower-case version to the > - derived type contains its associated generic function. */ > - if (gfc_fl_struct (p->attr.flavor)) > - gfc_delete_symtree (&p->ns->sym_root,gfc_dt_upper_string (p->name)= ); > - else > - gfc_delete_symtree (&p->ns->sym_root, p->name); > + bool freed =3D delete_symbol_from_ns (p, p->ns); > > - gfc_release_symbol (p); > + /* If the symbol is a procedure (function or subroutine), remove > + it from the procedure body namespace as well as from the outer > + namespace. */ > + if (!freed > + && p->formal_ns !=3D p->ns) > + freed =3D delete_symbol_from_ns (p, p->formal_ns); > + > + /* If the formal_ns field has not been set yet, the previous > + conditional does nothing. In that case, we can assume that > + gfc_current_ns is the procedure body namespace, and remove the > + symbol from there. */ > + if (!freed > + && gfc_current_ns !=3D p->ns > + && gfc_current_ns !=3D p->formal_ns) > + freed =3D delete_symbol_from_ns (p, gfc_current_ns); > } > else > restore_old_symbol (p); > diff --git a/gcc/testsuite/gfortran.dg/pr110996.f90 b/gcc/testsuite/gfor= tran.dg/pr110996.f90 > new file mode 100644 > index 00000000000..0e7551059a3 > --- /dev/null > +++ b/gcc/testsuite/gfortran.dg/pr110996.f90 > @@ -0,0 +1,16 @@ > +! { dg-do compile } > +! > +! PR fortran/110996 > +! This example used to result in memory errors and sometimes internal c= ompiler > +! errors, because the rejection of the subroutine statement was causing= the > +! symbol D to be freed without also freeing the symbol C which remained= in the > +! namespace with a dangling pointer to D. > +! > +! Original testcase from Jeremy Bennett > + > +PROGRAM p > +CONTAINS > + SUBROUTINE c(d) e { dg-error "Syntax error" } > + SUBROUTINE f > + END > +END