From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mo4-p00-ob.smtp.rzone.de (mo4-p00-ob.smtp.rzone.de [85.215.255.23]) by sourceware.org (Postfix) with ESMTPS id EEE5E3858D28; Mon, 31 Jan 2022 11:42:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EEE5E3858D28 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=nieper-wisskirchen.de Authentication-Results: sourceware.org; spf=none smtp.mailfrom=nieper-wisskirchen.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; t=1643629371; s=strato-dkim-0002; d=nieper-wisskirchen.de; h=Cc:To:Subject:Message-ID:Date:From:In-Reply-To:References:Cc:Date: From:Subject:Sender; bh=o+oFLe1FXxP8Af+H737B04Ecf2IIfyZFqR007H3wDRc=; b=s4p4/jv/vyal533A/N7WJe8Ooh9T+6W6C4fmxPPAi2o9uw8oQ3Jc8v1ELvJBZiKIa+ m/hrOnarA6xoDGdFmjVmMxK5FiFJAB6dNQdXnj8Fnw3xO7RqhZjhmKi8/EvG5/dO0Iz1 RY6AjNh3cG7ZejJUaqrdoxtie16ko8euV1tLUK0N6LPV+xOG5EvnhKmAUmWdRTFcfYLj 9yl9/w0DFG0haw9Q/GbNpjF5TJt7EBoGfR5FgcxZKv7d5iseNvwFMSVDJwGDixCPEFeS kMBINV5E0c5txWArnGsSW+AJOGnKOKTUO9c61DP5ypW9MmUYyO6GG7Wi876Oxrk0NSol Zzgg== Authentication-Results: strato.com; dkim=none X-RZG-AUTH: ":IW0WdmCmcvpIrP2+VJuPtIhjJvc4Ig+QdhX22iZVwSDOx4Kp3cYsBVGy6CZgmO/guIaKV8N5653qcA==" X-RZG-CLASS-ID: mo00 Received: from mail-yb1-f181.google.com by smtp.strato.de (RZmta 47.38.0 AUTH) with ESMTPSA id 262004y0VBgp4Io (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256 bits)) (Client did not present a certificate); Mon, 31 Jan 2022 12:42:51 +0100 (CET) Received: by mail-yb1-f181.google.com with SMTP id k17so39483462ybk.6; Mon, 31 Jan 2022 03:42:51 -0800 (PST) X-Gm-Message-State: AOAM533BJwoATZKDS+v76T9Axb8aYZpr6RdUXdl5ey0UxEzlHLkJ1Fcl 6rmrM1C8FtVPrEZ09dSU0CSmBvB7gxIfJxrFF5g= X-Google-Smtp-Source: ABdhPJxILSzugBI/fJ4ZsD3qrkV9uTflhlZlnK7ouaUnnEHY/yYBGVlZ+WwaAvOMnj7mqCBlQR5BSez6KT5ga4Gc7dI= X-Received: by 2002:a25:a081:: with SMTP id y1mr29092036ybh.17.1643629370262; Mon, 31 Jan 2022 03:42:50 -0800 (PST) MIME-Version: 1.0 References: <20211219213010.17113-1-marc@nieper-wisskirchen.de> <2a54fac9b37d87afb009b8eb339d5ad6927454dd.camel@redhat.com> In-Reply-To: From: =?UTF-8?Q?Marc_Nieper=2DWi=C3=9Fkirchen?= Date: Mon, 31 Jan 2022 12:42:38 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] gcc: pass-manager: Fix memory leak. [PR jit/63854] To: Jeff Law Cc: David Malcolm , gcc-patches@gcc.gnu.org, =?UTF-8?Q?Marc_Nieper=2DWi=C3=9Fkirchen_via_Jit?= Content-Type: multipart/mixed; boundary="000000000000c1f59005d6df48bb" X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, HTML_MESSAGE, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE 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-Content-Filtered-By: Mailman/MimeDel 2.1.29 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: Mon, 31 Jan 2022 11:42:55 -0000 --000000000000c1f59005d6df48bb Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Attached to this email is the patch updated to the recent renaming from *.c to *.cc. Am So., 23. Jan. 2022 um 14:18 Uhr schrieb Marc Nieper-Wi=C3=9Fkirchen < marc@nieper-wisskirchen.de>: > Am Sa., 15. Jan. 2022 um 14:56 Uhr schrieb Marc Nieper-Wi=C3=9Fkirchen > : > > > > Jeff, David, do you need any more input from my side? > > > > -- Marc > > > > Am Sa., 8. Jan. 2022 um 17:32 Uhr schrieb Jeff Law < > jeffreyalaw@gmail.com>: > > > > > > > > > > > > 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=C3=9Fkirchen wrot= e: > > > >> This patch fixes a memory leak in the pass manager. In the existin= g > > > >> 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 ha= sh > > > >> trait for > > > >> string hashes that are to be freed when the corresponding hash ent= ry > > > >> is removed. > > > >> > > > >> This fix is particularly relevant for using GCC as a library throu= gh > > > >> 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 sui= te > > > >> 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 fro= m > > > > const char * to char *, to convey that the map "owns" the name? Th= at > > > > 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 pat= h > > > 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 wi= th > > > these bits, so the analysis side kept falling off the daily todo list= . > > The only occurrences of m_name_to_pass_map are in pass-manager.h > (where it is defined as a private field of the class pass_manager) and > in passes.cc. There is just one instance where a name is added to the > map in passes.cc, namely through the put method. There, the name has > been xstrdup'ed. > > The name (as a const char *) escapes the pass map in > pass_manager::create_pass_tab through the call to > m_name_pass_map->traverse. This inserts the name into the pass_tab, > which is a static vec of const char *s. The pass_tab does not escape > the translation unit of passes.c. It is used in dump_one_pass where > the name is used as an argument to fprintf. The important point is > that it is not freed and not further copied. > > > > > > > If/once you're comfortable with the patch David, then go ahead and > apply > > > it on Marc's behalf. > > > > > > jeff > > > > --000000000000c1f59005d6df48bb Content-Type: text/x-patch; charset="US-ASCII"; name="0001-gcc-pass-manager-Fix-memory-leak.-PR-jit-63854.patch" Content-Disposition: attachment; filename="0001-gcc-pass-manager-Fix-memory-leak.-PR-jit-63854.patch" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_kz2mha0a0 RnJvbSBiN2IyZTQ5MTEzMjhmNmIyNzViYWFmZTM5NWExZDI0ODVlMTU4MTk1IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiA9P1VURi04P3E/TWFyYz0yME5pZXBlci1XaT1DMz05RmtpcmNo ZW4/PSA8bWFyY0BuaWVwZXItd2lzc2tpcmNoZW4uZGU+CkRhdGU6IFN1biwgMTkgRGVjIDIwMjEg MjI6MzA6MTAgKzAxMDAKU3ViamVjdDogW1BBVENIXSBnY2M6IHBhc3MtbWFuYWdlcjogRml4IG1l bW9yeSBsZWFrLiBbUFIgaml0LzYzODU0XQpUbzogZ2NjLXBhdGNoZXNAZ2NjLmdudS5vcmcKClRo aXMgcGF0Y2ggZml4ZXMgYSBtZW1vcnkgbGVhayBpbiB0aGUgcGFzcyBtYW5hZ2VyLiBJbiB0aGUg ZXhpc3RpbmcgY29kZSwKdGhlIG1fbmFtZV90b19wYXNzX21hcCBpcyBhbGxvY2F0ZWQgaW4gcGFz c19tYW5hZ2VyOjpyZWdpc3Rlcl9wYXNzX25hbWUsIGJ1dApuZXZlciBkZWFsbG9jYXRlZC4gIFRo aXMgaXMgZml4ZWQgYnkgYWRkaW5nIGEgZGVsZXRpb24gaW4KcGFzc19tYW5hZ2VyOjp+cGFzc19t YW5hZ2VyLiAgTW9yZW92ZXIgdGhlIHN0cmluZyBrZXlzIGluIG1fbmFtZV90b19wYXNzX21hcCBh cmUKYWxsIGR5bmFtaWNhbGx5IGFsbG9jYXRlZC4gIFRvIGZyZWUgdGhlbSwgdGhpcyBwYXRjaCBh ZGRzIGEgbmV3IGhhc2ggdHJhaXQgZm9yCnN0cmluZyBoYXNoZXMgdGhhdCBhcmUgdG8gYmUgZnJl ZWQgd2hlbiB0aGUgY29ycmVzcG9uZGluZyBoYXNoIGVudHJ5IGlzIHJlbW92ZWQuCgpUaGlzIGZp eCBpcyBwYXJ0aWN1bGFybHkgcmVsZXZhbnQgZm9yIHVzaW5nIEdDQyBhcyBhIGxpYnJhcnkgdGhy b3VnaCBsaWJnY2NqaXQuClRoZSBtZW1vcnkgbGVhayBhbHNvIG9jY3VycyB3aGVuIGxpYmdjY2pp dCBpcyBpbnN0cnVjdGVkIHRvIHVzZSBhbiBleHRlcm5hbApkcml2ZXIuCgpCZWZvcmUgdGhlIHBh dGNoLCBjb21waWxpbmcgdGhlIGhlbGxvIHdvcmxkIGV4YW1wbGUgb2YgbGliZ2Njaml0IHdpdGgK dGhlIGV4dGVybmFsIGRyaXZlciB1bmRlciBWYWxncmluZCBzaG93cyBhIGxvc3Mgb2YgMTIsNjEx ICg0OCBkaXJlY3QpCmJ5dGVzLiAgQWZ0ZXIgdGhlIHBhdGNoLCBubyBtZW1vcnkgbGVha3MgYXJl IHJlcG9ydGVkIGFueW1vcmUuCihNZW1vcnkgbGVha3Mgb2NjdXJyaW5nIHdoZW4gdXNpbmcgdGhl IGludGVybmFsIGRyaXZlciBhcmUgbW9zdGx5IGluCnRoZSBkcml2ZXIgY29kZSBpbiBnY2MvZ2Nj LmMgYW5kIGhhdmUgdG8gYmUgZml4ZWQgc2VwYXJhdGVseS4pCgpUaGUgcGF0Y2ggaGFzIGJlZW4g dGVzdGVkIGJ5IGZ1bGx5IGJvb3RzdHJhcHBpbmcgdGhlIGNvbXBpbGVyIHdpdGggdGhlCmZyb250 ZW5kcyBDLCBDKyssIEZvcnRyYW4sIExUTywgT2JqQywgSklUIGFuZCBydW5uaW5nIHRoZSB0ZXN0 IHN1aXRlCnVuZGVyIGEgeDg2XzY0LXBjLWxpbnV4LWdudSBob3N0LgoKZ2NjL0NoYW5nZUxvZzoK CglQUiBqaXQvNjM4NTQKCSogaGFzaC10cmFpdHMuaCAoc3RydWN0IHR5cGVkX2NvbnN0X2ZyZWVf cmVtb3ZlKTogTmV3LgoJKHN0cnVjdCBmcmVlX3N0cmluZ19oYXNoKTogTmV3LgoJKiBwYXNzX21h bmFnZXIuaDogVXNlIGZyZWVfc3RyaW5nX2hhc2guCgkqIHBhc3Nlcy5jYyAocGFzc19tYW5hZ2Vy OjpyZWdpc3Rlcl9wYXNzX25hbWUpOiBVc2UgZnJlZV9zdHJpbmdfaGFzaC4KICAgICAgICAocGFz c19tYW5hZ2VyOjp+cGFzc19tYW5hZ2VyKTogRGVsZXRlIGFsbG9jYXRlZCBtX25hbWVfdG9fcGFz c19tYXAuCi0tLQogZ2NjL2hhc2gtdHJhaXRzLmggIHwgMTcgKysrKysrKysrKysrKysrKysKIGdj Yy9wYXNzX21hbmFnZXIuaCB8ICAzICstLQogZ2NjL3Bhc3Nlcy5jYyAgICAgIHwgIDUgKysrLS0K IDMgZmlsZXMgY2hhbmdlZCwgMjEgaW5zZXJ0aW9ucygrKSwgNCBkZWxldGlvbnMoLSkKCmRpZmYg LS1naXQgYS9nY2MvaGFzaC10cmFpdHMuaCBiL2djYy9oYXNoLXRyYWl0cy5oCmluZGV4IGIxNjk3 NWNmZWIzLi5iZWYwYmQ0MmQwNCAxMDA2NDQKLS0tIGEvZ2NjL2hhc2gtdHJhaXRzLmgKKysrIGIv Z2NjL2hhc2gtdHJhaXRzLmgKQEAgLTI4LDYgKzI4LDExIEBAIHN0cnVjdCB0eXBlZF9mcmVlX3Jl bW92ZQogICBzdGF0aWMgaW5saW5lIHZvaWQgcmVtb3ZlIChUeXBlICpwKTsKIH07CiAKK3RlbXBs YXRlIDx0eXBlbmFtZSBUeXBlPgorc3RydWN0IHR5cGVkX2NvbnN0X2ZyZWVfcmVtb3ZlCit7Cisg IHN0YXRpYyBpbmxpbmUgdm9pZCByZW1vdmUgKGNvbnN0IFR5cGUgKnApOworfTsKIAogLyogUmVt b3ZlIHdpdGggZnJlZS4gICovCiAKQEAgLTM4LDYgKzQzLDEzIEBAIHR5cGVkX2ZyZWVfcmVtb3Zl IDxUeXBlPjo6cmVtb3ZlIChUeXBlICpwKQogICBmcmVlIChwKTsKIH0KIAordGVtcGxhdGUgPHR5 cGVuYW1lIFR5cGU+CitpbmxpbmUgdm9pZAordHlwZWRfY29uc3RfZnJlZV9yZW1vdmUgPFR5cGU+ OjpyZW1vdmUgKGNvbnN0IFR5cGUgKnApCit7CisgIGZyZWUgKGNvbnN0X2Nhc3QgPFR5cGUgKj4g KHApKTsKK30KKwogLyogSGVscGZ1bCB0eXBlIGZvciByZW1vdmluZyB3aXRoIGRlbGV0ZS4gICov CiAKIHRlbXBsYXRlIDx0eXBlbmFtZSBUeXBlPgpAQCAtMzA1LDYgKzMxNywxMSBAQCBzdHJ1Y3Qg Z2djX3B0cl9oYXNoIDogcG9pbnRlcl9oYXNoIDxUPiwgZ2djX3JlbW92ZSA8VCAqPiB7fTsKIHRl bXBsYXRlIDx0eXBlbmFtZSBUPgogc3RydWN0IGdnY19jYWNoZV9wdHJfaGFzaCA6IHBvaW50ZXJf aGFzaCA8VD4sIGdnY19jYWNoZV9yZW1vdmUgPFQgKj4ge307CiAKKy8qIFRyYWl0cyBmb3Igc3Ry aW5nIGVsZW1lbnRzIHRoYXQgc2hvdWxkIGJlIGZyZWVkIHdoZW4gYW4gZWxlbWVudCBpcworICAg ZGVsZXRlZC4gICovCisKK3N0cnVjdCBmcmVlX3N0cmluZ19oYXNoIDogc3RyaW5nX2hhc2gsIHR5 cGVkX2NvbnN0X2ZyZWVfcmVtb3ZlIDxjaGFyPiB7fTsKKwogLyogVHJhaXRzIGZvciBzdHJpbmcg ZWxlbWVudHMgdGhhdCBzaG91bGQgbm90IGJlIGZyZWVkIHdoZW4gYW4gZWxlbWVudAogICAgaXMg ZGVsZXRlZC4gICovCiAKZGlmZiAtLWdpdCBhL2djYy9wYXNzX21hbmFnZXIuaCBiL2djYy9wYXNz X21hbmFnZXIuaAppbmRleCA2NDg0ZDJjOGViYy4uNDM0ODRkYTI3MDAgMTAwNjQ0Ci0tLSBhL2dj Yy9wYXNzX21hbmFnZXIuaAorKysgYi9nY2MvcGFzc19tYW5hZ2VyLmgKQEAgLTEwNiw3ICsxMDYs NyBAQCBwcml2YXRlOgogCiBwcml2YXRlOgogICBjb250ZXh0ICptX2N0eHQ7Ci0gIGhhc2hfbWFw PG5vZnJlZV9zdHJpbmdfaGFzaCwgb3B0X3Bhc3MgKj4gKm1fbmFtZV90b19wYXNzX21hcDsKKyAg aGFzaF9tYXA8ZnJlZV9zdHJpbmdfaGFzaCwgb3B0X3Bhc3MgKj4gKm1fbmFtZV90b19wYXNzX21h cDsKIAogICAvKiBSZWZlcmVuY2VzIHRvIGFsbCBvZiB0aGUgaW5kaXZpZHVhbCBwYXNzZXMuCiAg ICAgIFRoZXNlIGZpZWxkcyBhcmUgZ2VuZXJhdGVkIHZpYSBtYWNybyBleHBhbnNpb24uCkBAIC0x NDYsNCArMTQ2LDMgQEAgcHJpdmF0ZToKIH0gLy8gbmFtZXNwYWNlIGdjYwogCiAjZW5kaWYgLyog ISBHQ0NfUEFTU19NQU5BR0VSX0ggKi8KLQpkaWZmIC0tZ2l0IGEvZ2NjL3Bhc3Nlcy5jYyBiL2dj Yy9wYXNzZXMuY2MKaW5kZXggMDBmODU2ZTA3NTQuLjM2ZTViNGFjNDVmIDEwMDY0NAotLS0gYS9n Y2MvcGFzc2VzLmNjCisrKyBiL2djYy9wYXNzZXMuY2MKQEAgLTkwMyw3ICs5MDMsNyBAQCB2b2lk CiBwYXNzX21hbmFnZXI6OnJlZ2lzdGVyX3Bhc3NfbmFtZSAob3B0X3Bhc3MgKnBhc3MsIGNvbnN0 IGNoYXIgKm5hbWUpCiB7CiAgIGlmICghbV9uYW1lX3RvX3Bhc3NfbWFwKQotICAgIG1fbmFtZV90 b19wYXNzX21hcCA9IG5ldyBoYXNoX21hcDxub2ZyZWVfc3RyaW5nX2hhc2gsIG9wdF9wYXNzICo+ ICgyNTYpOworICAgIG1fbmFtZV90b19wYXNzX21hcCA9IG5ldyBoYXNoX21hcDxmcmVlX3N0cmlu Z19oYXNoLCBvcHRfcGFzcyAqPiAoMjU2KTsKIAogICBpZiAobV9uYW1lX3RvX3Bhc3NfbWFwLT5n ZXQgKG5hbWUpKQogICAgIHJldHVybjsgLyogSWdub3JlIHBsdWdpbiBwYXNzZXMuICAqLwpAQCAt MTY3NCw2ICsxNjc0LDcgQEAgcGFzc19tYW5hZ2VyOjp+cGFzc19tYW5hZ2VyICgpCiAgIEdDQ19Q QVNTX0xJU1RTCiAjdW5kZWYgREVGX1BBU1NfTElTVAogCisgIGRlbGV0ZSBtX25hbWVfdG9fcGFz c19tYXA7CiB9CiAKIC8qIElmIHdlIGFyZSBpbiBJUEEgbW9kZSAoaS5lLiwgY3VycmVudF9mdW5j dGlvbl9kZWNsIGlzIE5VTEwpLCBjYWxsCkBAIC0xOTQzLDcgKzE5NDQsNyBAQCBwYXNzX21hbmFn ZXI6OmR1bXBfcHJvZmlsZV9yZXBvcnQgKCkgY29uc3QKIAkgICAiICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgfGluIGNvdW50ICAgICB8b3V0IHByb2IgICAgICIKIAkgICAifGluIGNv dW50ICAgICAgICAgICAgICAgICAgfG91dCBwcm9iICAgICAgICAgICAgICAgICAgIgogCSAgICJ8 c2l6ZSAgICAgICAgICAgICAgIHx0aW1lICAgICAgICAgICAgICAgICAgICAgIHxcbiIpOwotCSAg IAorCiAgIGZvciAoaW50IGkgPSAxOyBpIDwgcGFzc2VzX2J5X2lkX3NpemU7IGkrKykKICAgICBp ZiAocHJvZmlsZV9yZWNvcmRbaV0ucnVuKQogICAgICAgewotLSAKMi4zMi4wCgo= --000000000000c1f59005d6df48bb--