From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id B0771385842F for ; Thu, 6 Jan 2022 13:53:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B0771385842F Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-624-p0v8GXt1MwyJvzjoqhQtmg-1; Thu, 06 Jan 2022 08:53:04 -0500 X-MC-Unique: p0v8GXt1MwyJvzjoqhQtmg-1 Received: by mail-qv1-f70.google.com with SMTP id g2-20020a0562141cc200b004123b0abe18so2138533qvd.2 for ; Thu, 06 Jan 2022 05:53:04 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=mM+1AmCfjkSRiP+M8yGk/dF+ZuBmcdEkYuwVBZWc3rs=; b=FIOqLKq1NblT7Mr4Urk17MPdnUB4GLijnp8Md6NQmx3u5aZe2LGl31H4j5tn6b7FoX lnu/6tBVoeuuapfmmVxRh6CFivr70aa3MYcOb6xt0WNtRU7AH3zughqAavsepUIIi685 QHewPIHoOXeaE+4RS1cVMP+EHauIrSd+W5vIwX02/shKlsdCt/FR01bGMqNhFZKqvdtl d3vyi6m8UAK4cBGODnCvauc8U7QSMv0xA9Pv3q3ECDxTYO+GIb3Oem8lJ7NwEXd4X0Ja aGCJJ4dTolkKhye3bv5pSTo+vf4iNMYL1TAUKU5xoMrYZlKUHGXmZeDY5So0UWteFZQ8 8vHA== X-Gm-Message-State: AOAM531UfzAqWWoKHxdD6W7qTUrVaW3o9VcYfLlEaZu5oU2Lu8n00xbe FaVop1nRt7sBu7HhNb3rXrgFWlaNvb6jbhE5C3gi2CCww+8UfkdMGVuVzfMBPG1nIV4YDI3Q4XU IwIYAm2w= X-Received: by 2002:ac8:584a:: with SMTP id h10mr52729499qth.615.1641477183766; Thu, 06 Jan 2022 05:53:03 -0800 (PST) X-Google-Smtp-Source: ABdhPJwSKkCm0QA3JK3QxUkoHWTmHqsbXfoRj9jJ8RhCU4jqpC+APgP/481g+XWkwfGpAsvB9LvGTw== X-Received: by 2002:ac8:584a:: with SMTP id h10mr52729487qth.615.1641477183405; Thu, 06 Jan 2022 05:53:03 -0800 (PST) Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id g5sm307459qtb.97.2022.01.06.05.53.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 06 Jan 2022 05:53:02 -0800 (PST) Message-ID: <2a54fac9b37d87afb009b8eb339d5ad6927454dd.camel@redhat.com> Subject: Re: [PATCH] gcc: pass-manager: Fix memory leak. [PR jit/63854] From: David Malcolm To: Marc =?ISO-8859-1?Q?Nieper-Wi=DFkirchen?= , gcc-patches@gcc.gnu.org Cc: jit@gcc.gnu.org Date: Thu, 06 Jan 2022 08:53:01 -0500 In-Reply-To: <20211219213010.17113-1-marc@nieper-wisskirchen.de> References: <20211219213010.17113-1-marc@nieper-wisskirchen.de> User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: jit@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Jit mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Jan 2022 13:53:08 -0000 On Sun, 2021-12-19 at 22:30 +0100, Marc Nieper-Wißkirchen wrote: > This patch fixes a memory leak in the pass manager. In the existing > code, > the m_name_to_pass_map is allocated in > pass_manager::register_pass_name, but > never deallocated.  This is fixed by adding a deletion in > pass_manager::~pass_manager.  Moreover the string keys in > m_name_to_pass_map are > all dynamically allocated.  To free them, this patch adds a new hash > trait for > string hashes that are to be freed when the corresponding hash entry > is removed. > > This fix is particularly relevant for using GCC as a library through > libgccjit. > The memory leak also occurs when libgccjit is instructed to use an > external > driver. > > Before the patch, compiling the hello world example of libgccjit with > the external driver under Valgrind shows a loss of 12,611 (48 direct) > bytes.  After the patch, no memory leaks are reported anymore. > (Memory leaks occurring when using the internal driver are mostly in > the driver code in gcc/gcc.c and have to be fixed separately.) > > The patch has been tested by fully bootstrapping the compiler with > the > frontends C, C++, Fortran, LTO, ObjC, JIT and running the test suite > under a x86_64-pc-linux-gnu host. Thanks for the patch. It looks correct to me, given that pass_manager::register_pass_name does an xstrdup and puts the result in the map. That said: - I'm not officially a reviewer for this part of gcc (though I probably touched this code last) - is it cleaner to instead change m_name_to_pass_map's key type from const char * to char *, to convey that the map "owns" the name? That way we probably wouldn't need struct typed_const_free_remove, and (I hope) works better with the type system. Dave > > gcc/ChangeLog: > >         PR jit/63854 >         * hash-traits.h (struct typed_const_free_remove): New. >         (struct free_string_hash): New. >         * pass_manager.h: Use free_string_hash. >         * passes.c (pass_manager::register_pass_name): Use > free_string_hash. >         (pass_manager::~pass_manager): Delete allocated > m_name_to_pass_map. > --- >  gcc/hash-traits.h  | 17 +++++++++++++++++ >  gcc/pass_manager.h |  3 +-- >  gcc/passes.c       |  5 +++-- >  3 files changed, 21 insertions(+), 4 deletions(-) > > diff --git a/gcc/hash-traits.h b/gcc/hash-traits.h > index 6f0373ec27f..1c08d6874ab 100644 > --- a/gcc/hash-traits.h > +++ b/gcc/hash-traits.h > @@ -28,6 +28,11 @@ struct typed_free_remove >    static inline void remove (Type *p); >  }; >   > +template > +struct typed_const_free_remove > +{ > +  static inline void remove (const Type *p); > +}; >   >  /* Remove with free.  */ >   > @@ -38,6 +43,13 @@ typed_free_remove ::remove (Type *p) >    free (p); >  } >   > +template > +inline void > +typed_const_free_remove ::remove (const Type *p) > +{ > +  free (const_cast (p)); > +} > + >  /* Helpful type for removing with delete.  */ >   >  template > @@ -305,6 +317,11 @@ struct ggc_ptr_hash : pointer_hash , > ggc_remove {}; >  template >  struct ggc_cache_ptr_hash : pointer_hash , ggc_cache_remove > {}; >   > +/* Traits for string elements that should be freed when an element > is > +   deleted.  */ > + > +struct free_string_hash : string_hash, typed_const_free_remove > {}; > + >  /* Traits for string elements that should not be freed when an > element >     is deleted.  */ >   > diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h > index aaf72cf6803..f5615e1fda8 100644 > --- a/gcc/pass_manager.h > +++ b/gcc/pass_manager.h > @@ -106,7 +106,7 @@ private: >   >  private: >    context *m_ctxt; > -  hash_map *m_name_to_pass_map; > +  hash_map *m_name_to_pass_map; >   >    /* References to all of the individual passes. >       These fields are generated via macro expansion. > @@ -146,4 +146,3 @@ private: >  } // namespace gcc >   >  #endif /* ! GCC_PASS_MANAGER_H */ > - > diff --git a/gcc/passes.c b/gcc/passes.c > index 4bea6ae5b6a..0c70ece5321 100644 > --- a/gcc/passes.c > +++ b/gcc/passes.c > @@ -903,7 +903,7 @@ void >  pass_manager::register_pass_name (opt_pass *pass, const char *name) >  { >    if (!m_name_to_pass_map) > -    m_name_to_pass_map = new hash_map *> (256); > +    m_name_to_pass_map = new hash_map > (256); >   >    if (m_name_to_pass_map->get (name)) >      return; /* Ignore plugin passes.  */ > @@ -1674,6 +1674,7 @@ pass_manager::~pass_manager () >    GCC_PASS_LISTS >  #undef DEF_PASS_LIST >   > +  delete m_name_to_pass_map; >  } >   >  /* If we are in IPA mode (i.e., current_function_decl is NULL), call > @@ -1943,7 +1944,7 @@ pass_manager::dump_profile_report () const >            "                                 |in count     |out > prob     " >            "|in count                  |out prob                  " >            "|size               |time                      |\n"); > -          > + >    for (int i = 1; i < passes_by_id_size; i++) >      if (profile_record[i].run) >        {