From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x62e.google.com (mail-pl1-x62e.google.com [IPv6:2607:f8b0:4864:20::62e]) by sourceware.org (Postfix) with ESMTPS id A6CA3385C417; Sat, 8 Jan 2022 16:32:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A6CA3385C417 Received: by mail-pl1-x62e.google.com with SMTP id i6so7933701pla.0; Sat, 08 Jan 2022 08:32:33 -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:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=NJn6xPkVZIRJlXv7eK7xEPB8W9XLMXMhOYGergAeQf0=; b=QTFMoD56v2phZJPLIleVgnQRIFhapErB9UPdM9PQY4dPYvyTnYPs6Yh+W4MlzUo3NC 7uqlkUdCVoEz+P9EBJskkVYzuSBIs/0I59LdP0KM/00eWq8PqhHtuOB3eXLVrbHZGo0t qw8ekJSmLn+KIeQFMi5uLc/4qoSYRcCnkvWpoqwWzmx8z58mMqIDu3GwSTlmFxQA1GII 5ApIQDjuPDPDlfuNQN18B7uAQgYeTK1/9Xp8sd4XWuVVNCfa7gk1qvcuPEnIinSYiJON 3wPv51w6DUgraTYiXrxqxpUF2T6evV/qxhhFwBTxXOgM3TPoyCy4HMGBsFOjwEOXHjfP nQ9Q== X-Gm-Message-State: AOAM531iHvRqs94LV0PGHiorIuZDGntlMTXEr0tOZVtfz2xdxZSChmdW Y7OEpSsUJ6KuViq3VqG1neo= X-Google-Smtp-Source: ABdhPJyTseo43uIqjeEBhAQSFpvvrBOT0p0imDV4+60QeWy9gVNlq500MaNUFKtmA12Mso6A06DBzQ== X-Received: by 2002:a63:8c:: with SMTP id 134mr15401584pga.599.1641659552589; Sat, 08 Jan 2022 08:32:32 -0800 (PST) Received: from [192.168.1.15] (65-130-85-11.slkc.qwest.net. [65.130.85.11]) by smtp.gmail.com with ESMTPSA id w13sm1969334pgm.5.2022.01.08.08.32.29 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 08 Jan 2022 08:32:32 -0800 (PST) Message-ID: Date: Sat, 8 Jan 2022 09:32:29 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Subject: Re: [PATCH] gcc: pass-manager: Fix memory leak. [PR jit/63854] Content-Language: en-US To: David Malcolm , =?UTF-8?Q?Marc_Nieper-Wi=c3=9fkirchen?= , gcc-patches@gcc.gnu.org Cc: jit@gcc.gnu.org References: <20211219213010.17113-1-marc@nieper-wisskirchen.de> <2a54fac9b37d87afb009b8eb339d5ad6927454dd.camel@redhat.com> From: Jeff Law In-Reply-To: <2a54fac9b37d87afb009b8eb339d5ad6927454dd.camel@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3.3 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, 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: Sat, 08 Jan 2022 16:32:35 -0000 On 1/6/2022 6:53 AM, David Malcolm via Gcc-patches wrote: > 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. My concern (and what I hadn't had time to dig into) was we initially used nofree_string_hash -- I wanted to make sure there wasn't any path where the name came from the stack (can't be free'd), was saved elsewhere (danging pointer) and the like.  ie, why were we using nofree_string_hash to begin with?  I've never really mucked around with these bits, so the analysis side kept falling off the daily todo list. If/once you're comfortable with the patch David, then go ahead and apply it on Marc's behalf. jeff