From patchwork Thu May 2 01:34:54 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nathaniel Shead X-Patchwork-Id: 1930461 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20230601 header.b=WEW4TOiu; dkim-atps=neutral Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=server2.sourceware.org; envelope-from=gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=patchwork.ozlabs.org) Received: from server2.sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4VVGhw6gFWz1ydX for ; Thu, 2 May 2024 11:35:26 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 09611384AB59 for ; Thu, 2 May 2024 01:35:23 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-pl1-x62d.google.com (mail-pl1-x62d.google.com [IPv6:2607:f8b0:4864:20::62d]) by sourceware.org (Postfix) with ESMTPS id 5AAF43858D34 for ; Thu, 2 May 2024 01:35:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5AAF43858D34 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 5AAF43858D34 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::62d ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714613704; cv=none; b=hzoWKrpJS77A3+xCfK4M0TwRN9mSQUN0w0zgJS+vZEL4nRstkPV080jouN+Cfv26JiKlZ+Ff8ha8qP/pHogZkm1ziWqRyOnB0OtZHqKScDONydtGZdQncp/kvtvMOzEbweoW0J8FywfspsnF3hgEDvR8gUxjH+vrK3uSvsHyJsI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714613704; c=relaxed/simple; bh=45B9wiWyCJb9xqVD5+UtvsBKZNWZEAnNVlzkSh0aRPs=; h=DKIM-Signature:Message-ID:Date:From:To:Subject:MIME-Version; b=XlS0LHUszt2kYnqO9iV+ve4g6LU8Qaq0AToHS9I8aM8gWHGtGmzcc4o68GDEqRcF4jiasHafpVMkodTbNmRbLSuKgFORcmXvp27tmE+SzgCLNN2EMfbuhz2a+MkKyj86j+ykV+psty7sZC9kDovXz0We5cVYgJb3OxcVFu8Avi4= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pl1-x62d.google.com with SMTP id d9443c01a7336-1e8bbcbc2b7so67107475ad.0 for ; Wed, 01 May 2024 18:35:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1714613700; x=1715218500; darn=gcc.gnu.org; h=in-reply-to:content-disposition:mime-version:references:subject:cc :to:from:date:message-id:from:to:cc:subject:date:message-id:reply-to; bh=GeXdzY69CNim/M3eit++CDmduUN7IvXEqoc7ACE97B0=; b=WEW4TOiuvOayRXKF0zRNi3jl4jNM4GjUGqpKeg9fdXdlCRyZXK9o35bE+EngiiW6LL YOnkhRUm/LEkXUSxReVKiKrMpl+QjS+2pDsYyZjm3stI//IhPFBDpoft6qXw0EgZavbK NbDOD+Jl0JHJQuBnxYADaTU9bfneCEeBPNIxYNxUtf4DpH0kfp/aiS/06x4MePLDf5lm OR3dUYTAK+Jk2xSZNwnsrOFYYuxL6zhDKgARb61ePjUItWiXNL9iPh5bNET/65GPcebp 6Vk80oyx0xJxNqvQkm4ax4XaKIu6UjkJw66mKJMJq5lSSNl6H1bbVsyAqkhtP64kIzie 4i5Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714613700; x=1715218500; h=in-reply-to:content-disposition:mime-version:references:subject:cc :to:from:date:message-id:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=GeXdzY69CNim/M3eit++CDmduUN7IvXEqoc7ACE97B0=; b=v7GXXXsX1qIa6AdhZK9P7yCz62JeiTicIp9AWWKqEGD6iOtYSgRmjTAtk9k0mXO5oC 15+AUFAXBZfwwVGxQTSwnLgaRdyTCuBmIA0T6FpcFfTtIIwUViaOk1LOmnwcE68Qy58w WoOaMKKAuWNEpC/JP9muru3RNIiUgXN5zEza3Np5xtaXFfWUDgPfMSe7JukeLQu2uhUb 7QHXOwcq6M3K0Ob3XX61GnhdeiUnKU5xJGBb9Vwp8iUeBsinycLi2r0H6iDK6eNHCYfj RkOSpMUOk/dMWIhVGiZ+AwSZUqf9vW16PdXKLIuJJSSUQNXY1kHaIsVy7Geta8sr/F4r WdMQ== X-Gm-Message-State: AOJu0YwrTLWaoMC7lOs/Jk2DdxmJxMI2MbJZjdVauD+7iQpi2RHGQ4j7 Ix3LzVa9nnK0x4Tfwj73REiMtx8fm+gpm0WnGVEXjflBKAK8McMK X-Google-Smtp-Source: AGHT+IH2ZJShS4QFe+MplnTaIOAZhUznFOXAbDsRNIa6bJYWwA3HQcGKMWjFrFItf3/+N0w+OD9DvA== X-Received: by 2002:a17:903:32c5:b0:1e2:81c1:b35e with SMTP id i5-20020a17090332c500b001e281c1b35emr753421plr.54.1714613699756; Wed, 01 May 2024 18:34:59 -0700 (PDT) Received: from Thaum. (121-44-11-123.tpgi.com.au. [121.44.11.123]) by smtp.gmail.com with ESMTPSA id m8-20020a1709026bc800b001e0af9928casm15679plt.55.2024.05.01.18.34.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 01 May 2024 18:34:58 -0700 (PDT) Message-ID: <6632edc2.170a0220.9abdd.00d3@mx.google.com> X-Google-Original-Message-ID: Date: Thu, 2 May 2024 11:34:54 +1000 From: Nathaniel Shead To: Patrick Palka Cc: gcc-patches@gcc.gnu.org, Jason Merrill , Nathan Sidwell Subject: [PATCH v2] c++/modules: Fix dangling pointer with imported_temploid_friends References: <6631c581.170a0220.3b821.1e9d@mx.google.com> <66324e95.170a0220.20975.b0cb@mx.google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <66324e95.170a0220.20975.b0cb@mx.google.com> X-Spam-Status: No, score=-11.2 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 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org On Thu, May 02, 2024 at 12:15:44AM +1000, Nathaniel Shead wrote: > On Wed, May 01, 2024 at 09:57:38AM -0400, Patrick Palka wrote: > > > > On Wed, 1 May 2024, Nathaniel Shead wrote: > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk (and > > > later 14.2)? I don't think making it a GTY root is necessary but I felt > > > perhaps better to be safe than sorry. > > > > > > Potentially another approach would be to use DECL_UID instead like how > > > entity_map does; would that be preferable? > > > > > > -- >8 -- > > > > > > I got notified by Linaro CI and by checking testresults that there seems > > > to be some occasional failures in tpl-friend-4_b.C on some architectures > > > and standards modes since r15-59-gb5f6a56940e708. I haven't been able > > > to reproduce but looking at the backtrace I suspect the issue is that > > > we're adding to the 'imported_temploid_friend' map a decl that is > > > ultimately discarded, which then has its address reused by a later decl > > > causing a failure in the assert in 'set_originating_module'. > > > > > > This patch attempts to fix the issue in two ways: by ensuring that we > > > only store the decl if we know it's a new decl (and hence won't be > > > discarded), and by making the imported_temploid_friends map a GTY root > > > so that even if the decl does get discarded later the address isn't > > > reused. > > > > > > gcc/cp/ChangeLog: > > > > > > * module.cc (imported_temploid_friends): Mark GTY, and... > > > (init_modules): ...allocate from GGC. > > > (trees_in::decl_value): Only write to imported_temploid_friends > > > for new decls. > > > > > > Signed-off-by: Nathaniel Shead > > > --- > > > gcc/cp/module.cc | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > > > index 5b8ff5bc483..37d38bb9654 100644 > > > --- a/gcc/cp/module.cc > > > +++ b/gcc/cp/module.cc > > > @@ -2731,7 +2731,7 @@ static keyed_map_t *keyed_table; > > > need to be attached to the same module as the temploid. This maps > > > these decls to the temploid they are instantiated them, as there is > > > no other easy way to get this information. */ > > > -static hash_map *imported_temploid_friends; > > > +static GTY(()) hash_map *imported_temploid_friends; > > > > > > /********************************************************************/ > > > /* Tree streaming. The tree streaming is very specific to the tree > > > @@ -8327,7 +8327,8 @@ trees_in::decl_value () > > > if (TREE_CODE (inner) == FUNCTION_DECL > > > || TREE_CODE (inner) == TYPE_DECL) > > > if (tree owner = tree_node ()) > > > - imported_temploid_friends->put (decl, owner); > > > + if (is_new) > > > + imported_temploid_friends->put (decl, owner); > > > > Hmm, I'm not seeing this code path getting reached for tpl-friend-4_b.C. > > It seems we're instead adding to imported_temploid_friends from > > propagate_defining_module, during tsubst_friend_function. > > > > What seems to be happening is that we we first tsubst_friend_function > > 'foo' from TPL, and then we tsubst_friend_function 'foo' from DEF, > > which ends up calling duplicate_decls, which ggc_frees this 'foo' > > redeclaration that is still present in the imported_temploid_friends map. > > > > So I don't think marking imported_temploid_friends as a GC root would > > help with this situation. If we want to keep imported_temploid_friends > > as a tree -> tree map, I think we just need to ensure that a decl > > is removed from the map upon getting ggc_free'd from e.g. duplicate_decls. > > > > But it seems simpler to use DECL_UID as the key instead, since those > > never get reused even after the decl gets ggc_free'd IIUC. > > > > Ah right, thanks for digging into that further. Yup OK, I think > probably the DECL_UID route feels safer to me then in case there are > other places where a decl might be explicitly freed. > > Looking at tree.cc it looks like the relevant function is > 'allocate_decl_uid' which shouldn't reuse UIDs until 2^32 decls have > been created, so we should be safe on the reuse front. > > I'll draft and test a patch for that tomorrow morning. > Here's that patch. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/14.2? -- >8 -- I got notified by Linaro CI and by checking testresults that there seems to be some occasional failures in tpl-friend-4_b.C on some architectures and standards modes since r15-59-gb5f6a56940e708. I haven't been able to reproduce but looking at the backtrace I suspect the issue is that we're adding to the 'imported_temploid_friend' map a decl that is ultimately discarded, which then has its address reused by a later decl causing a failure in the assert in 'set_originating_module'. This patch fixes the problem by using DECL_UID as the map key instead of the tree directly, much like with entity_map, since even if a declaration gets deallocated the DECL_UID should not be reused by a later declaration. gcc/cp/ChangeLog: * module.cc (imported_temploid_friends): Map from DECL_UID instead of tree. (get_originating_module_decl): Likewise. (propagate_defining_module): Likewise. (trees_out::decl_value): Likewise. (trees_in::decl_value): Likewise. And only enter into the map for new declarations. (init_modules): Update type of imported_temploid_friends. Signed-off-by: Nathaniel Shead --- gcc/cp/module.cc | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index fac0301d80e..ceb383ba509 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -2730,8 +2730,13 @@ static keyed_map_t *keyed_table; /* Instantiations of temploid friends imported from another module need to be attached to the same module as the temploid. This maps these decls to the temploid they are instantiated them, as there is - no other easy way to get this information. */ -static hash_map *imported_temploid_friends; + no other easy way to get this information. We map the DECL_UID + instead of the tree directly to handle keys that get freed and + reused. */ +typedef hash_map, tree> + > temploid_map_t; +static temploid_map_t *imported_temploid_friends; /********************************************************************/ /* Tree streaming. The tree streaming is very specific to the tree @@ -7829,7 +7834,7 @@ trees_out::decl_value (tree decl, depset *dep) /* But don't consider imported temploid friends as attached, since importers will need to merge this decl even if it was attached to a different module. */ - if (imported_temploid_friends->get (decl)) + if (imported_temploid_friends->get (DECL_UID (decl))) is_attached = false; bits.b (is_attached); @@ -8014,7 +8019,7 @@ trees_out::decl_value (tree decl, depset *dep) { /* Write imported temploid friends so that importers can reconstruct this information on stream-in. */ - tree* slot = imported_temploid_friends->get (decl); + tree* slot = imported_temploid_friends->get (DECL_UID (decl)); tree_node (slot ? *slot : NULL_TREE); } @@ -8327,7 +8332,8 @@ trees_in::decl_value () if (TREE_CODE (inner) == FUNCTION_DECL || TREE_CODE (inner) == TYPE_DECL) if (tree owner = tree_node ()) - imported_temploid_friends->put (decl, owner); + if (is_new) + imported_temploid_friends->put (DECL_UID (decl), owner); /* Regular typedefs will have a NULL TREE_TYPE at this point. */ unsigned tdef_flags = 0; @@ -18976,7 +18982,7 @@ get_originating_module_decl (tree decl) /* An imported temploid friend is attached to the same module the befriending class was. */ if (imported_temploid_friends) - if (tree *slot = imported_temploid_friends->get (decl)) + if (tree *slot = imported_temploid_friends->get (DECL_UID (decl))) decl = *slot; int use; @@ -19306,7 +19312,7 @@ propagate_defining_module (tree decl, tree orig) if (DECL_LANG_SPECIFIC (not_tmpl) && DECL_MODULE_IMPORT_P (not_tmpl)) { - bool exists = imported_temploid_friends->put (decl, orig); + bool exists = imported_temploid_friends->put (DECL_UID (decl), orig); /* We should only be called if lookup for an existing decl failed, in which case there shouldn't already be an entry @@ -20528,8 +20534,7 @@ init_modules (cpp_reader *reader) pending_table = new pending_map_t (EXPERIMENT (1, 400)); entity_map = new entity_map_t (EXPERIMENT (1, 400)); vec_safe_reserve (entity_ary, EXPERIMENT (1, 400)); - imported_temploid_friends - = new hash_map (EXPERIMENT (1, 400)); + imported_temploid_friends = new temploid_map_t (EXPERIMENT (1, 400)); } #if CHECKING_P