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 E23543858D37 for ; Tue, 27 Feb 2024 18:10:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E23543858D37 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org E23543858D37 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709057432; cv=none; b=ZO9sxjGdnAsUEelearQkPb10qJgLmwhqkrN5qH0/33nEG5P61jp7KaJ4Q5pi1yCUbj+DnhYyfxnKYwinQmRb+4dS2LrQ7pYYLVzoWKiYyTCsPiOsQWivQaXVagQCuuSXYIp5jxn5f80Oz9OE5xuWdgQLBGcqRiYIqsYSSpLgkMs= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709057432; c=relaxed/simple; bh=AIlRVtyaNtaTagV5zj/CbJwxdePEj171q86h2FfGAvo=; h=DKIM-Signature:From:Date:To:Subject:Message-ID:MIME-Version; b=wpypc3oHxNcCXK4lv7R9dwq2VNdN08K9PApCDMARQB++8YOZDidSzw7Y7yoGibF6Wj5AQQXHkIWiF196TYyqoc6B4psN1kkON9kc2SBDoUkfzI9pCi3R7PAzabkJjnitgScYA8aCsyp9fzXBdyWEQjtGLeJC6aatnz8CI9mNXbw= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1709057429; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=y5xKRNDO6mWdDPa9u65+tsiU5mrIuky4TjiD06HN8nk=; b=YBm8dvWuiXBg4F1Ue92Vyb1X2Fyqhy6ReVUbjN3luf/FsrWs+wsnWCKAblqZ+ZpGHipQr+ naeOfTnfOxUIFcRCubpJrbzth25WmonZB+Hdb/7IIE3iPN4sBuKZonMcmG+YO6MkFnq8Mi jDnS3wb7pvxsocmOV9kJqqQJCsKUFVA= Received: from mail-qk1-f197.google.com (mail-qk1-f197.google.com [209.85.222.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-344-NqQfhKVpN-OwiSFbr25klQ-1; Tue, 27 Feb 2024 13:10:27 -0500 X-MC-Unique: NqQfhKVpN-OwiSFbr25klQ-1 Received: by mail-qk1-f197.google.com with SMTP id af79cd13be357-787bd91c5feso336055385a.0 for ; Tue, 27 Feb 2024 10:10:27 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709057427; x=1709662227; h=mime-version:references:message-id:in-reply-to:subject:cc:to:date :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=y5xKRNDO6mWdDPa9u65+tsiU5mrIuky4TjiD06HN8nk=; b=Bjm2I1/Lc6i9eI8T/f8Rs/H3C4c/lP41zO7IlH7KaqtJedFYghx1Pa7mbxqHmErDQd D+M1ZRrJyqVohAm0nAaFZV16JeZwQI4hYYEQEE6MHfD56VEc0F28QLEcZakekFev1RKq IOSykRwchobwRMfNQoXbNQnyOaUQ+13GsggX6qyvXLGOaD60DlcARiWF778mUtVeprCD Vyj+Je3+4r10vgZ4mhzJe7oN5ReJLQTkl1zhIMamu6646xG6lHBj2iWNduJteMo1DdFi oqeg2+8tw5OnEMVsMZ8t5Jn1BEiQ8vZUmoKs8YRloiaDi/bprmAH2mOZZtGAYBxCKl8D +wkg== X-Gm-Message-State: AOJu0YyiDrSFhq6kOBs5ELMYvoOG5A/I5U7LCE68oE4E+S0n0IBjzsNn 7t45iF4uJ8jxEAgtGafPQnCWeSQhH88haJUud5ixwKiHGf+vlACCYtKcMdqLwLdA5TlyCBAgL37 aZnLvN/lYfpgOaB3HYExgl4nhBv6C5MAdJ7qBuJV2YrHM+Hc2fAy215c= X-Received: by 2002:a05:6214:c68:b0:68f:4372:2b56 with SMTP id t8-20020a0562140c6800b0068f43722b56mr2815541qvj.22.1709057426937; Tue, 27 Feb 2024 10:10:26 -0800 (PST) X-Google-Smtp-Source: AGHT+IE3xXskngxG1iAzeHFXQdgsatoHjs8XFqS+RWF7MWuH62CX9jc0+WadePIjDUZAxCmjGwQDLA== X-Received: by 2002:a05:6214:c68:b0:68f:4372:2b56 with SMTP id t8-20020a0562140c6800b0068f43722b56mr2815522qvj.22.1709057426559; Tue, 27 Feb 2024 10:10:26 -0800 (PST) Received: from [192.168.1.130] (ool-457670bb.dyn.optonline.net. [69.118.112.187]) by smtp.gmail.com with ESMTPSA id nh18-20020a056214391200b0068feed84646sm4167400qvb.26.2024.02.27.10.10.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 27 Feb 2024 10:10:26 -0800 (PST) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Tue, 27 Feb 2024 13:10:25 -0500 (EST) To: Patrick Palka cc: gcc-patches@gcc.gnu.org, jason@redhat.com, nathan@acm.org Subject: Re: [PATCH] c++/modules: local class merging [PR99426] In-Reply-To: <20240227023734.2742095-1-ppalka@redhat.com> Message-ID: <9d4f1a3e-99ea-5146-639d-22add3e22301@idea> References: <20240227023734.2742095-1-ppalka@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-13.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE,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: On Mon, 26 Feb 2024, Patrick Palka wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this approach > look reasonable? > > -- >8 -- > > One known missing piece in the modules implementation is merging of a > streamed-in local class with the corresponding in-TU version of the > local class. This missing piece turns out to cause a hard-to-reduce > use-after-free GC issue due to the entity_ary not being marked as a GC > root (deliberately), and manifests as a serialization error on stream-in > as in PR99426 (see comment #6 for a reduction). It's also reproducible > on trunk when running the xtreme-header tests without -fno-module-lazy. > > This patch makes us merge such local classes according to their position > within the containing function's definition, similar to how we merge > FIELD_DECLs of a class according to their index in the TYPE_FIELDS > list. > > PR c++/99426 > > gcc/cp/ChangeLog: > > * module.cc (merge_kind::MK_local_class): New enumerator. > (merge_kind_name): Update. > (trees_out::chained_decls): Move BLOCK-specific handling > of DECL_LOCAL_DECL_P decls to ... > (trees_out::core_vals) : ... here. Stream > BLOCK_VARS manually. > (trees_in::core_vals) : Stream BLOCK_VARS > manually. Handle deduplicated local classes. > (trees_out::key_local_class): Define. > (trees_in::key_local_class): Define. > (trees_out::get_merge_kind) : Return > MK_local_class for a local class. > (trees_out::key_mergeable) : Use > key_local_class. > (trees_in::key_mergeable) : Likewise. > (trees_in::is_matching_decl): Be flexible with type mismatches > for local entities. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/xtreme-header-7_a.H: New test. > * g++.dg/modules/xtreme-header-7_b.C: New test. > --- > gcc/cp/module.cc | 167 +++++++++++++++--- > .../g++.dg/modules/xtreme-header-7_a.H | 4 + > .../g++.dg/modules/xtreme-header-7_b.C | 6 + > 3 files changed, 149 insertions(+), 28 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/xtreme-header-7_a.H > create mode 100644 gcc/testsuite/g++.dg/modules/xtreme-header-7_b.C > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index fa91c6ff9cb..f77f73a59ed 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -2771,6 +2771,7 @@ enum merge_kind > > MK_enum, /* Found by CTX, & 1stMemberNAME. */ > MK_keyed, /* Found by key & index. */ > + MK_local_class, /* Found by CTX, index. */ > > MK_friend_spec, /* Like named, but has a tmpl & args too. */ > MK_local_friend, /* Found by CTX, index. */ > @@ -2799,7 +2800,7 @@ static char const *const merge_kind_name[MK_hwm] = > "unique", "named", "field", "vtable", /* 0...3 */ > "asbase", "partial", "enum", "attached", /* 4...7 */ > > - "friend spec", "local friend", NULL, NULL, /* 8...11 */ > + "local class", "friend spec", "local friend", NULL, /* 8...11 */ > NULL, NULL, NULL, NULL, > > "type spec", "type tmpl spec", /* 16,17 type (template). */ > @@ -2928,6 +2929,7 @@ public: > unsigned binfo_mergeable (tree *); > > private: > + tree key_local_class (const merge_key&, tree); > uintptr_t *find_duplicate (tree existing); > void register_duplicate (tree decl, tree existing); > /* Mark as an already diagnosed bad duplicate. */ > @@ -3086,6 +3088,7 @@ public: > void binfo_mergeable (tree binfo); > > private: > + void key_local_class (merge_key&, tree, tree); > bool decl_node (tree, walk_kind ref); > void type_node (tree); > void tree_value (tree); > @@ -4952,18 +4955,7 @@ void > trees_out::chained_decls (tree decls) > { > for (; decls; decls = DECL_CHAIN (decls)) > - { > - if (VAR_OR_FUNCTION_DECL_P (decls) > - && DECL_LOCAL_DECL_P (decls)) > - { > - /* Make sure this is the first encounter, and mark for > - walk-by-value. */ > - gcc_checking_assert (!TREE_VISITED (decls) > - && !DECL_TEMPLATE_INFO (decls)); > - mark_by_value (decls); > - } > - tree_node (decls); > - } > + tree_node (decls); > tree_node (NULL_TREE); > } > > @@ -6204,7 +6196,21 @@ trees_out::core_vals (tree t) > > /* DECL_LOCAL_DECL_P decls are first encountered here and > streamed by value. */ > - chained_decls (t->block.vars); > + for (tree decls = t->block.vars; decls; decls = DECL_CHAIN (decls)) > + { > + if (VAR_OR_FUNCTION_DECL_P (decls) > + && DECL_LOCAL_DECL_P (decls)) > + { > + /* Make sure this is the first encounter, and mark for > + walk-by-value. */ > + gcc_checking_assert (!TREE_VISITED (decls) > + && !DECL_TEMPLATE_INFO (decls)); > + mark_by_value (decls); > + } > + tree_node (decls); > + } > + tree_node (NULL_TREE); > + > /* nonlocalized_vars is a middle-end thing. */ > WT (t->block.subblocks); > WT (t->block.supercontext); > @@ -6717,7 +6723,34 @@ trees_in::core_vals (tree t) > case BLOCK: > t->block.locus = state->read_location (*this); > t->block.end_locus = state->read_location (*this); > - t->block.vars = chained_decls (); > + > + for (tree *chain = &t->block.vars;;) > + if (tree decl = tree_node ()) > + { > + /* For a deduplicated local class, chain the to-be-discarded > + decl not the in-TU decl (which is already chained to in-TU > + entities). */ > + if (is_duplicate (decl)) > + decl = maybe_duplicate (decl); > + else if (DECL_IMPLICIT_TYPEDEF_P (decl) > + && TYPE_TEMPLATE_INFO (TREE_TYPE (decl))) > + { > + tree tmpl = TYPE_TI_TEMPLATE (TREE_TYPE (decl)); > + if (DECL_TEMPLATE_RESULT (tmpl) == decl && is_duplicate (tmpl)) > + decl = DECL_TEMPLATE_RESULT (maybe_duplicate (tmpl)); > + } > + > + if (!DECL_P (decl) || DECL_CHAIN (decl)) > + { > + set_overrun (); > + break; > + } > + *chain = decl; > + chain = &DECL_CHAIN (decl); > + } > + else > + break; > + > /* nonlocalized_vars is middle-end. */ > RT (t->block.subblocks); > RT (t->block.supercontext); > @@ -10335,6 +10368,83 @@ trees_in::fn_parms_fini (int tag, tree fn, tree existing, bool is_defn) > } > } > > +/* Encode into KEY the position of the local class declaration DECL > + within FN. The position is encoded as the index of the innermost > + BLOCK (numbered in BFS order) along with the index within its > + BLOCK_VARS list. */ > + > +void > +trees_out::key_local_class (merge_key& key, tree decl, tree fn) > +{ > + auto_vec blocks; > + blocks.quick_push (DECL_INITIAL (fn)); > + unsigned block_ix = 0; > + while (block_ix != blocks.length ()) > + { > + tree block = blocks[block_ix]; > + unsigned decl_ix = 0; > + for (tree var = BLOCK_VARS (block); var; var = DECL_CHAIN (var)) > + { > + if (TREE_CODE (var) != TYPE_DECL) > + continue; > + if (var == decl) > + { > + key.index = (block_ix << 10) | decl_ix; > + return; > + } > + ++decl_ix; > + } > + for (tree sub = BLOCK_SUBBLOCKS (block); sub; sub = BLOCK_CHAIN (sub)) > + blocks.safe_push (sub); > + ++block_ix; > + } > + > + /* Not-found value. */ > + key.index = 1023; > +} > + > +/* Look up the local class corresponding at the position encoded by > + KEY within FN. */ > + > +tree > +trees_in::key_local_class (const merge_key& key, tree fn) > +{ > + if (!DECL_INITIAL (fn)) > + return NULL_TREE; > + > + const unsigned block_pos = key.index >> 10; > + const unsigned decl_pos = key.index & 1023; > + > + if (decl_pos == 1023) > + return NULL_TREE; > + > + auto_vec blocks; > + blocks.quick_push (DECL_INITIAL (fn)); > + unsigned block_ix = 0; > + while (block_ix != blocks.length ()) > + { > + tree block = blocks[block_ix]; > + if (block_ix == block_pos) > + { > + unsigned decl_ix = 0; > + for (tree var = BLOCK_VARS (block); var; var = DECL_CHAIN (var)) > + { > + if (TREE_CODE (var) != TYPE_DECL) > + continue; > + if (decl_ix == decl_pos) > + return var; > + ++decl_ix; > + } > + return NULL_TREE; > + } > + for (tree sub = BLOCK_SUBBLOCKS (block); sub; sub = BLOCK_CHAIN (sub)) > + blocks.safe_push (sub); > + ++block_ix; > + } > + > + return NULL_TREE; > +} > + > /* DEP is the depset of some decl we're streaming by value. Determine > the merging behaviour. */ > > @@ -10454,17 +10564,10 @@ trees_out::get_merge_kind (tree decl, depset *dep) > gcc_unreachable (); > > case FUNCTION_DECL: > - // FIXME: This can occur for (a) voldemorty TYPE_DECLS > - // (which are returned from a function), or (b) > - // block-scope class definitions in template functions. > - // These are as unique as the containing function. While > - // on read-back we can discover if the CTX was a > - // duplicate, we don't have a mechanism to get from the > - // existing CTX to the existing version of this decl. > gcc_checking_assert > (DECL_IMPLICIT_TYPEDEF_P (STRIP_TEMPLATE (decl))); > > - mk = MK_unique; > + mk = MK_local_class; > break; > > case RECORD_TYPE: > @@ -10768,6 +10871,10 @@ trees_out::key_mergeable (int tag, merge_kind mk, tree decl, tree inner, > } > break; > > + case MK_local_class: > + key_local_class (key, STRIP_TEMPLATE (decl), container); > + break; > + > case MK_enum: > { > /* Anonymous enums are located by their first identifier, > @@ -11117,11 +11224,10 @@ trees_in::key_mergeable (int tag, merge_kind mk, tree decl, tree inner, > break; > > case FUNCTION_DECL: > - // FIXME: What about a voldemort? how do we find what it > - // duplicates? Do we have to number vmorts relative to > - // their containing function? But how would that work > - // when matching an in-TU declaration? > - kind = "unique"; > + gcc_checking_assert (mk == MK_local_class); > + existing = key_local_class (key, container); > + if (existing && inner != decl) > + existing = TYPE_TI_TEMPLATE (TREE_TYPE (existing)); > break; > > case TYPE_DECL: > @@ -11374,6 +11480,11 @@ trees_in::is_matching_decl (tree existing, tree decl, bool is_typedef) > /* Just like duplicate_decls, presum the user knows what > they're doing in overriding a builtin. */ > TREE_TYPE (existing) = TREE_TYPE (decl); > + else if (decl_function_context (decl)) > + /* The type of a mergeable local entity (such as a function scope > + capturing lambda's closure type fields) can depend on an > + unmergeable local entity (such as a local variable), so type > + equality isn't feasible in general for local entities. */; > else > { > // FIXME:QOI Might be template specialization from a module, > diff --git a/gcc/testsuite/g++.dg/modules/xtreme-header-7_a.H b/gcc/testsuite/g++.dg/modules/xtreme-header-7_a.H > new file mode 100644 > index 00000000000..bf7859fba99 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/xtreme-header-7_a.H > @@ -0,0 +1,4 @@ > +// { dg-additional-options -fmodule-header } > + > +// { dg-module-cmi {} } > +#include "xtreme-header.h" > diff --git a/gcc/testsuite/g++.dg/modules/xtreme-header-7_b.C b/gcc/testsuite/g++.dg/modules/xtreme-header-7_b.C > new file mode 100644 > index 00000000000..03f3dc1bae6 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/xtreme-header-7_b.C > @@ -0,0 +1,6 @@ > +// A version of xtreme-header_{a.H,b.C} that doesn't pass > +// -fno-module-lazy. > +// { dg-additional-options -fmodules-ts } > + > +#include "xtreme-header.h" > +import "xtreme-header-7_a.H"; > -- Consider the following minimal testcase added that verifies we now properly merge local classes (and local enums): gcc/testsuite/g++.dg/modules/merge-17.h | 28 ++++++++++++++++++++++++++++ gcc/testsuite/g++.dg/modules/merge-17_a.H | 3 +++ gcc/testsuite/g++.dg/modules/merge-17_b.C | 3 +++ 3 files changed, 34 insertions(+) diff --git a/gcc/testsuite/g++.dg/modules/merge-17.h b/gcc/testsuite/g++.dg/modules/merge-17.h new file mode 100644 index 00000000000..246ccd8011d --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/merge-17.h @@ -0,0 +1,28 @@ +// PR c++/99426 + +inline auto f() { + struct A { int m = 42; }; + return A{}; +} + +template +inline auto ft() { + decltype(+T()) x; + return [&x] { }; +} + +inline auto g() { + enum E { e }; + return e; +} + +template +inline auto gt() { + enum E : T { e }; + return e; +} + +using ty1 = decltype(f()); +using ty2 = decltype(ft()); +using ty3 = decltype(g()); +using ty4 = decltype(gt()); diff --git a/gcc/testsuite/g++.dg/modules/merge-17_a.H b/gcc/testsuite/g++.dg/modules/merge-17_a.H new file mode 100644 index 00000000000..0440cd765e9 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/merge-17_a.H @@ -0,0 +1,3 @@ +// { dg-additional-options "-fmodule-header" } +// { dg-module-cmi {} } +#include "merge-17.h" diff --git a/gcc/testsuite/g++.dg/modules/merge-17_b.C b/gcc/testsuite/g++.dg/modules/merge-17_b.C new file mode 100644 index 00000000000..4315b99f172 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/merge-17_b.C @@ -0,0 +1,3 @@ +// { dg-additional-options "-fmodules-ts -fno-module-lazy" } +#include "merge-17.h" +import "merge-17_a.H";