| Message ID | 20180905145732.404-29-rep.dot.nop@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FORTRAN,01/29] gdbinit: break on gfc_internal_error | expand |
ping [Rebased, re-regtested cleanly. Ok for trunk?] On Wed, 5 Sep 2018 14:57:31 +0000 Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > From: Bernhard Reutner-Fischer <aldot@gcc.gnu.org> > > compiling gfortran.dg/typebound_proc_31.f90 leaked the type-bound > structs: > > 56 bytes in 1 blocks are definitely lost. > at 0x4C2CC05: calloc (vg_replace_malloc.c:711) > by 0x151EA90: xcalloc (xmalloc.c:162) > by 0x8E3E4F: gfc_get_typebound_proc(gfc_typebound_proc*) (symbol.c:4945) > by 0x84C095: match_procedure_in_type (decl.c:10486) > by 0x84C095: gfc_match_procedure() (decl.c:6696) > ... > > gcc/fortran/ChangeLog: > > 2017-12-06 Bernhard Reutner-Fischer <aldot@gcc.gnu.org> > > * symbol.c (free_tb_tree): Free type-bound procedure struct. > (gfc_get_typebound_proc): Use explicit memcpy for clarity. > --- > gcc/fortran/symbol.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c > index 53c760a6c38..cde34c67482 100644 > --- a/gcc/fortran/symbol.c > +++ b/gcc/fortran/symbol.c > @@ -3845,7 +3845,7 @@ free_tb_tree (gfc_symtree *t) > > /* TODO: Free type-bound procedure structs themselves; probably needs some > sort of ref-counting mechanism. */ > - > + free (t->n.tb); > free (t); > } > > @@ -5052,7 +5052,7 @@ gfc_get_typebound_proc (gfc_typebound_proc *tb0) > > result = XCNEW (gfc_typebound_proc); > if (tb0) > - *result = *tb0; > + memcpy (result, tb0, sizeof (gfc_typebound_proc));; > result->error = 1; > > latest_undo_chgset->tbps.safe_push (result);
Looks good and simple. Proceed. Thanks Jerry On 10/28/21 5:05 PM, Bernhard Reutner-Fischer via Fortran wrote: > ping > [Rebased, re-regtested cleanly. Ok for trunk?] > On Wed, 5 Sep 2018 14:57:31 +0000 > Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > >> From: Bernhard Reutner-Fischer <aldot@gcc.gnu.org> >> >> compiling gfortran.dg/typebound_proc_31.f90 leaked the type-bound >> structs: >> >> 56 bytes in 1 blocks are definitely lost. >> at 0x4C2CC05: calloc (vg_replace_malloc.c:711) >> by 0x151EA90: xcalloc (xmalloc.c:162) >> by 0x8E3E4F: gfc_get_typebound_proc(gfc_typebound_proc*) (symbol.c:4945) >> by 0x84C095: match_procedure_in_type (decl.c:10486) >> by 0x84C095: gfc_match_procedure() (decl.c:6696) >> ... >> >> gcc/fortran/ChangeLog: >> >> 2017-12-06 Bernhard Reutner-Fischer <aldot@gcc.gnu.org> >> >> * symbol.c (free_tb_tree): Free type-bound procedure struct. >> (gfc_get_typebound_proc): Use explicit memcpy for clarity. >> --- >> gcc/fortran/symbol.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c >> index 53c760a6c38..cde34c67482 100644 >> --- a/gcc/fortran/symbol.c >> +++ b/gcc/fortran/symbol.c >> @@ -3845,7 +3845,7 @@ free_tb_tree (gfc_symtree *t) >> >> /* TODO: Free type-bound procedure structs themselves; probably needs some >> sort of ref-counting mechanism. */ >> - >> + free (t->n.tb); >> free (t); >> } >> >> @@ -5052,7 +5052,7 @@ gfc_get_typebound_proc (gfc_typebound_proc *tb0) >> >> result = XCNEW (gfc_typebound_proc); >> if (tb0) >> - *result = *tb0; >> + memcpy (result, tb0, sizeof (gfc_typebound_proc));; >> result->error = 1; >> >> latest_undo_chgset->tbps.safe_push (result);
On Fri, 29 Oct 2021 07:54:21 -0700
Jerry D <jvdelisle2@gmail.com> wrote:
> Looks good and simple. Proceed. Thanks
Committed as 7883a7f07c1ad9c8aaccc5bbd96e0ae1fa230c89
Thanks!
Maybe somebody could eyeball and ACK "Fix memory leak in finalization
wrappers"
https://gcc.gnu.org/pipermail/fortran/2021-October/056838.html
We were generating (and emitting to modules) finalization wrapper
needlessly, i.e. even when they were not called for.
This 1) leaked like shown in the initial submission and
2) polluted module files with unwarranted (wrong) mention of
finalization wrappers even when compiling without any coarray stuff.
E.g. a modified udr10.f90 (from libgomp) has the following diff in the
module which illustrates the positive side-effect of the fix:
-26 'array' '' '' 25 ((VARIABLE INOUT UNKNOWN-PROC UNKNOWN UNKNOWN 0 0
-ARTIFICIAL DIMENSION CONTIGUOUS DUMMY) () (DERIVED 3 0 0 0 DERIVED ()) 0
-0 () (0 0 ASSUMED_RANK) 0 () () () 0 0)
-27 'byte_stride' '' '' 25 ((VARIABLE UNKNOWN-INTENT UNKNOWN-PROC UNKNOWN
-UNKNOWN 0 0 ARTIFICIAL VALUE DUMMY) () (INTEGER 8 0 0 0 INTEGER ()) 0 0
-() () 0 () () () 0 0)
-28 'fini_coarray' '' '' 25 ((VARIABLE UNKNOWN-INTENT UNKNOWN-PROC
-UNKNOWN UNKNOWN 0 0 ARTIFICIAL VALUE DUMMY) () (LOGICAL 1 0 0 0 LOGICAL
-()) 0 0 () () 0 () () () 0 0)
thanks,
Dear Bernhard, all, Am 29.10.21 um 02:05 schrieb Bernhard Reutner-Fischer via Gcc-patches: >> diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c >> index 53c760a6c38..cde34c67482 100644 >> --- a/gcc/fortran/symbol.c >> +++ b/gcc/fortran/symbol.c >> @@ -5052,7 +5052,7 @@ gfc_get_typebound_proc (gfc_typebound_proc *tb0) >> >> result = XCNEW (gfc_typebound_proc); >> if (tb0) >> - *result = *tb0; >> + memcpy (result, tb0, sizeof (gfc_typebound_proc));; >> result->error = 1; >> >> latest_undo_chgset->tbps.safe_push (result); > > please forgive me, but frankly speaking, I don't like this change. It seems to serve no obvious purpose other than obfuscating the code and defeating the compiler's ability to detect type mismatches. I would not have OKed that part of the patch. Harald
On Fri, 29 Oct 2021 21:36:26 +0200 Harald Anlauf via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > Dear Bernhard, all, > > Am 29.10.21 um 02:05 schrieb Bernhard Reutner-Fischer via Gcc-patches: > > >> diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c > >> index 53c760a6c38..cde34c67482 100644 > >> --- a/gcc/fortran/symbol.c > >> +++ b/gcc/fortran/symbol.c > > >> @@ -5052,7 +5052,7 @@ gfc_get_typebound_proc (gfc_typebound_proc *tb0) > >> > >> result = XCNEW (gfc_typebound_proc); > >> if (tb0) > >> - *result = *tb0; > >> + memcpy (result, tb0, sizeof (gfc_typebound_proc));; > >> result->error = 1; > >> > >> latest_undo_chgset->tbps.safe_push (result); > > > > > > please forgive me, but frankly speaking, I don't like this change. > > It seems to serve no obvious purpose other than obfuscating the code > and defeating the compiler's ability to detect type mismatches. mhm okay. IIRC these are folded to memcpy early on and in some projects with certain optimization levels results in an unobvious call to memcpy (which poses trouble if you want to avoid relocations at all cost which this might trigger if pulling in memcpy unexpectedly). f951 of course is not in the camp to bother much about this so i admit the change might stem from a tinfoil-hat moment of mine and might not be appropriate here. Although i don't buy the argument of the possibility of papering over type-mismatches in this particular case (the incoming arg is typed gfc_typebound_proc*, the result is typed gfc_typebound_proc*, the allocation is casted to gfc_typebound_proc*) we can certainly revert that hunk if folks prefer. > > I would not have OKed that part of the patch. For reference: gcc/fortran/symbol.c gfc_typebound_proc* gfc_get_typebound_proc (gfc_typebound_proc *tb0) { gfc_typebound_proc *result; result = XCNEW (gfc_typebound_proc); if (tb0) memcpy (result, tb0, sizeof (gfc_typebound_proc)); result->error = 1; latest_undo_chgset->tbps.safe_push (result); return result; } And i did - *result = *tb0; + memcpy (result, tb0, sizeof (gfc_typebound_proc)); > > Harald >
On Fri, 29 Oct 2021 22:09:07 +0200 Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > On Fri, 29 Oct 2021 21:36:26 +0200 > Harald Anlauf via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > Dear Bernhard, all, > > > > Am 29.10.21 um 02:05 schrieb Bernhard Reutner-Fischer via Gcc-patches: > > > > >> diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c > > >> index 53c760a6c38..cde34c67482 100644 > > >> --- a/gcc/fortran/symbol.c > > >> +++ b/gcc/fortran/symbol.c > > > > >> @@ -5052,7 +5052,7 @@ gfc_get_typebound_proc (gfc_typebound_proc *tb0) > > >> > > >> result = XCNEW (gfc_typebound_proc); > > >> if (tb0) > > >> - *result = *tb0; > > >> + memcpy (result, tb0, sizeof (gfc_typebound_proc));; > > >> result->error = 1; > > >> > > >> latest_undo_chgset->tbps.safe_push (result); > > > > > > > > > > please forgive me, but frankly speaking, I don't like this change. > > > > It seems to serve no obvious purpose other than obfuscating the code > > and defeating the compiler's ability to detect type mismatches. > > mhm okay. > > > > I would not have OKed that part of the patch. I reverted this hunk. thanks,
diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c index 53c760a6c38..cde34c67482 100644 --- a/gcc/fortran/symbol.c +++ b/gcc/fortran/symbol.c @@ -3845,7 +3845,7 @@ free_tb_tree (gfc_symtree *t) /* TODO: Free type-bound procedure structs themselves; probably needs some sort of ref-counting mechanism. */ - + free (t->n.tb); free (t); } @@ -5052,7 +5052,7 @@ gfc_get_typebound_proc (gfc_typebound_proc *tb0) result = XCNEW (gfc_typebound_proc); if (tb0) - *result = *tb0; + memcpy (result, tb0, sizeof (gfc_typebound_proc));; result->error = 1; latest_undo_chgset->tbps.safe_push (result);
From: Bernhard Reutner-Fischer <aldot@gcc.gnu.org> compiling gfortran.dg/typebound_proc_31.f90 leaked the type-bound structs: 56 bytes in 1 blocks are definitely lost. at 0x4C2CC05: calloc (vg_replace_malloc.c:711) by 0x151EA90: xcalloc (xmalloc.c:162) by 0x8E3E4F: gfc_get_typebound_proc(gfc_typebound_proc*) (symbol.c:4945) by 0x84C095: match_procedure_in_type (decl.c:10486) by 0x84C095: gfc_match_procedure() (decl.c:6696) ... gcc/fortran/ChangeLog: 2017-12-06 Bernhard Reutner-Fischer <aldot@gcc.gnu.org> * symbol.c (free_tb_tree): Free type-bound procedure struct. (gfc_get_typebound_proc): Use explicit memcpy for clarity. --- gcc/fortran/symbol.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)