From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x633.google.com (mail-pl1-x633.google.com [IPv6:2607:f8b0:4864:20::633]) by sourceware.org (Postfix) with ESMTPS id 92AF43858D1E; Fri, 15 Sep 2023 09:16:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 92AF43858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-pl1-x633.google.com with SMTP id d9443c01a7336-1c0db66af1bso14865285ad.2; Fri, 15 Sep 2023 02:16:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1694769410; x=1695374210; darn=gcc.gnu.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=hIDYboJN509PEYrp8ewHROaxEj9OnQQaEPHll/YZvX0=; b=dGJe4JZ0cXTmVlprqkbPbdMRGj8puv3cnnTWWxcjgLj+LrmcZlhd5drzazPXwh46XM wha4xgGhapJILiSkfE6Era651OcyLlPN193rQMyImd0kiMu8Pbn9eOdBwjlAq7rkY0/u bQPnY0YGc9GLoC2gQWWWbggpK7mmg9Og+kcNcefUxcTcsvl1sRi/VX9U+vBPu6YFKV72 rRDYu8RJaXv7zPON1bDk7Ni1d09eNIoalBjN6CamxaLvHXc56WTHbwgCsqa9xZNFh0tz m4GcpEFtQVytVzrJ5XLw5ZK2lcMv01ZwlcLgVqbqyLxAmDLgsM+3zEEcd9nuJ70ABGM1 Tm2Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694769410; x=1695374210; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=hIDYboJN509PEYrp8ewHROaxEj9OnQQaEPHll/YZvX0=; b=fBTWg1aH96AaOhp7kxPvZV6TFIXvOHiHiP4zUEGUwODvLiMnN/TS6NccvljpuAx74Q xTfD0B3Hf8nMXJKc8pv8EJeY7WL8rm351H3aRhU0TDg45JYxFFaU6Tuy0rblMRQpAnw9 PAbZDEez4thJCroWBAsYGJCpve6ts7bY5+va1A/Yu4LWk2PluDjwcZrSOczT0Rjy0pAc 4kZeh9DX+NFKbqkCyraB13XW85XUqs+EVGnqf2p49fW+lkco8RlGNEpU0UnMesuhuvoN ODXYG+dOEyAwiwGZtfgq1Cwjv9sOSbVj5ph5Iw4USCdwS7RSAVZUlOvnIsLfVzNeqQ/W wrHA== X-Gm-Message-State: AOJu0YywIs14PpRds992oLlEkM59SjEfgla0r/XPs/SrAop2ShJFAch3 wA+V/mltj6/ASbq7q3Ioma0HsgErbTU5EGb8LSBbzjw+ X-Google-Smtp-Source: AGHT+IHjDeukgFx9yA/eYJGwCAicaY3gWKMeVXf6VH6b+f2mri+YI7YXGkrVKy/KzEwDD48UTV9+gUZNzbIC65nkK34= X-Received: by 2002:a17:90b:4894:b0:267:fe44:86c3 with SMTP id ii20-20020a17090b489400b00267fe4486c3mr941177pjb.31.1694769410026; Fri, 15 Sep 2023 02:16:50 -0700 (PDT) MIME-Version: 1.0 References: <20230915071847.135585-1-mikael@gcc.gnu.org> In-Reply-To: <20230915071847.135585-1-mikael@gcc.gnu.org> From: Paul Richard Thomas Date: Fri, 15 Sep 2023 10:16:38 +0100 Message-ID: Subject: Re: [PATCH] fortran: Remove reference count update [PR108957] To: Mikael Morin Cc: gcc-patches@gcc.gnu.org, fortran@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-6.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,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: Hi Mikael, The comment is very welcome! Looks good to me. OK for mainline. Thanks for the patch. Paul On Fri, 15 Sept 2023 at 08:19, Mikael Morin via Fortran wrote: > > Hello, > > Harald reminded me recently that there was a working patch attached to the PR. > I added a documentation comment with the hope that it may help avoid > making the same mistake in the future. > Regression tested on x86_64-pc-linux-gnu. > OK for master? > > -- >8 -- > > Remove one reference count incrementation following the assignment of a > symbol pointer to a local variable. Most symbol pointers are "weak" pointer > and don't need any reference count update when they are assigned, and it is > especially the case of local variables. > > This fixes a memory leak with the testcase from the PR (not included). > > PR fortran/108957 > > gcc/fortran/ChangeLog: > > * gfortran.h (gfc_symbol): Add comment documenting reference counting. > * parse.cc (parse_interface): Remove reference count incrementation. > --- > gcc/fortran/gfortran.h | 20 ++++++++++++++++++++ > gcc/fortran/parse.cc | 3 --- > 2 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h > index f4a1c106cea..6caf7765ac6 100644 > --- a/gcc/fortran/gfortran.h > +++ b/gcc/fortran/gfortran.h > @@ -1944,7 +1944,27 @@ typedef struct gfc_symbol > according to the Fortran standard. */ > unsigned pass_as_value:1; > > + /* Reference counter, used for memory management. > + > + Some symbols may be present in more than one namespace, for example > + function and subroutine symbols are present both in the outer namespace and > + the procedure body namespace. Freeing symbols with the namespaces they are > + in would result in double free for those symbols. This field counts > + references and is used to delay the memory release until the last reference > + to the symbol is removed. > + > + Not every symbol pointer is accounted for reference counting. Fields > + gfc_symtree::n::sym are, and gfc_finalizer::proc_sym as well. But most of > + them (dummy arguments, generic list elements, etc) are "weak" pointers; > + the reference count isn't updated when they are assigned, and they are > + ignored when the surrounding structure memory is released. This is not a > + problem because there is always a namespace as surrounding context and > + symbols have a name they can be referred with in that context, so the > + namespace keeps the symbol from being freed, keeping the pointer valid. > + When the namespace ceases to exist, and the symbols with it, the other > + structures referencing symbols cease to exist as well. */ > int refs; > + > struct gfc_namespace *ns; /* namespace containing this symbol */ > > tree backend_decl; > diff --git a/gcc/fortran/parse.cc b/gcc/fortran/parse.cc > index 8f09ddf753c..58386805ffe 100644 > --- a/gcc/fortran/parse.cc > +++ b/gcc/fortran/parse.cc > @@ -4064,9 +4064,6 @@ loop: > accept_statement (st); > prog_unit = gfc_new_block; > prog_unit->formal_ns = gfc_current_ns; > - if (prog_unit == prog_unit->formal_ns->proc_name > - && prog_unit->ns != prog_unit->formal_ns) > - prog_unit->refs++; > > decl: > /* Read data declaration statements. */ > -- > 2.40.1 >