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.129.124]) by sourceware.org (Postfix) with ESMTPS id 694CB3858CDB for ; Fri, 12 Apr 2024 18:39:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 694CB3858CDB 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 694CB3858CDB Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712947185; cv=none; b=IUr50ydz6YrM2vy+FcE1T9p5ojOJPSdGiFBZbmr727nW9kdBh5GcqkS50sS6Akc51sD20fgneL1OH8go3hrCPIw9WY0YsPq0tLqCnhELGXlUsuofJm9Tadx1GYKakz9bV2iXgSifmsaIqBrgKH535eN/YS6NlI0vgnzBk9anlo0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712947185; c=relaxed/simple; bh=yEWE1GYLY937u6dPfHwCMtf9JB8tk1947Qw80HKvBAU=; h=DKIM-Signature:From:Date:To:Subject:Message-ID:MIME-Version; b=A0dX2vkRgSK7jWpgxsfyndfwgp7f1TbEmEUBhzAQkbU+9/PlD33mHkI3Iks8wfERIQHun4Hd6fODkG9FYwWzNvuX3SEMKM06DjmW+/GnlCREfrLcP3NHnzUv3irqaVEYXiY74um8GBiXSwX4yLB3l/dOQkjSKNuEEu1fffScSKE= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1712947181; 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=1bssPUKQLdsnlWMsB/kW+P5eZG7bCWie21AIKsBGcDo=; b=AYUnvdjAje2o2NSqxMDYFrQfvrCcFiU0wBwoCh2KTvtVYlIzSJMRByZY2NgSPhMefPMnAQ rjAQsbsZPNyqDo2GNAeyPSKoJroB6XXDzCBYp5syLVcswyrXIVFO+u69F9Lyeb/5NLY40l Q7cGBFuiLxQaeHzwV88HcpL48JyoAEk= Received: from mail-yw1-f197.google.com (mail-yw1-f197.google.com [209.85.128.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-141-c4_5V_0kN9Kg5CFnV_IFHw-1; Fri, 12 Apr 2024 14:39:39 -0400 X-MC-Unique: c4_5V_0kN9Kg5CFnV_IFHw-1 Received: by mail-yw1-f197.google.com with SMTP id 00721157ae682-61510f72bb3so20391117b3.0 for ; Fri, 12 Apr 2024 11:39:39 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712947178; x=1713551978; 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=1bssPUKQLdsnlWMsB/kW+P5eZG7bCWie21AIKsBGcDo=; b=Lpr243KnZvZty7tSP1tqvFj0PBp5LWAgZfIZdXNfV5ULfe1HOShDvm0Bp3bx0/p3hV QI7WjRiE+4+9m/5Grh5yykK9zdgZqbti+m8Y1n698/bL//zG6pVTakTnOgYk9qYYnWC+ oZ9msoQOdYHMejhkozqAH08mqkaeRxAbV3lCzuvqmHUeSJddDHjUkR1cihJbRebVONCT nX9J+hL7KviJMj4aTER8xgMsaJRlmXx0TD6kM17sLRBkciZm8MBx66AbX5jEmGcnp3hr Vs116lpjiISbRI7BVEx2hQWKBHx/hSvpDN1AowiH6MxAFVyl0u6aevzWmLqyqrX5+AHL 9lEw== X-Forwarded-Encrypted: i=1; AJvYcCUwPwie9e8jF2Ntfx1h0SW26DgTR1XVNq+fCl9pRD3XLMo9kWY/HdCWAZHLbF//OxOsZt04Y4WA39JEzavRfVl+JIpZI72jmw== X-Gm-Message-State: AOJu0YxrbX4a1RDuEyj6T/UEyknUmkaXl1e/L+Je7SWQYug+aNT/faid ASzrzDwhk0jb2ZknM2ZNhX8NSwZqnLi2RrgvQMcka4JQbC4W6aK7trCEv5r6i93zi2x9RTTGl3R d/Gjr3Qzbhj/73ruLO6c3yPsJ/kJgSlnPdBrtKHSlFGC1HZBgARi/jWE= X-Received: by 2002:a05:690c:360b:b0:60a:1c9c:e00a with SMTP id ft11-20020a05690c360b00b0060a1c9ce00amr3701222ywb.45.1712947178324; Fri, 12 Apr 2024 11:39:38 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFU5Or7fpdIGV6uVNz8S7fLyuDfvKakQLbsauwj+1nlaBw2xWZtJRgpDp9w9zOcsA/AexE3aw== X-Received: by 2002:a05:690c:360b:b0:60a:1c9c:e00a with SMTP id ft11-20020a05690c360b00b0060a1c9ce00amr3701109ywb.45.1712947176295; Fri, 12 Apr 2024 11:39:36 -0700 (PDT) Received: from [192.168.1.130] (ool-457670bb.dyn.optonline.net. [69.118.112.187]) by smtp.gmail.com with ESMTPSA id i16-20020a0cfcd0000000b006994062299dsm2644797qvq.33.2024.04.12.11.39.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 12 Apr 2024 11:39:35 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Fri, 12 Apr 2024 14:39:34 -0400 (EDT) To: Jason Merrill cc: Patrick Palka , gcc-patches@gcc.gnu.org, nathan@acm.org Subject: Re: [PATCH] c++/modules: local class merging [PR99426] In-Reply-To: <1a24b2c4-5d38-41b2-a290-5dea96ffa789@redhat.com> Message-ID: References: <20240227023734.2742095-1-ppalka@redhat.com> <9d4f1a3e-99ea-5146-639d-22add3e22301@idea> <63e9bd8a-f45c-46e1-a006-110284d7c54d@redhat.com> <1b682650-7ef5-429f-d738-ae189884191b@idea> <45179f66-242f-18f0-74e3-9be4d04d4d55@idea> <945b6320-a469-43c3-a32a-13819f14b971@redhat.com> <1a24b2c4-5d38-41b2-a290-5dea96ffa789@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=-14.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_FILL_THIS_FORM_SHORT,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 Fri, 12 Apr 2024, Jason Merrill wrote: > On 4/12/24 13:48, Patrick Palka wrote: > > On Fri, 12 Apr 2024, Jason Merrill wrote: > > > > > On 4/12/24 10:35, Patrick Palka wrote: > > > > On Wed, 10 Apr 2024, Jason Merrill wrote: > > > > > > > > > On 4/10/24 14:48, Patrick Palka wrote: > > > > > > On Tue, 9 Apr 2024, Jason Merrill wrote: > > > > > > > > > > > > > On 3/5/24 10:31, Patrick Palka wrote: > > > > > > > > On Tue, 27 Feb 2024, Patrick Palka wrote: > > > > > > > > > > > > > > > > Subject: [PATCH] c++/modules: local type merging [PR99426] > > > > > > > > > > > > > > > > One known missing piece in the modules implementation is merging > > > > > > > > of > > > > > > > > a > > > > > > > > streamed-in local type (class or enum) with the corresponding > > > > > > > > in-TU > > > > > > > > version of the local type. 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 types according to their > > > > > > > > position > > > > > > > > within the containing function's definition, analogous 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_type): 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 types.. > > > > > > > > (trees_out::key_local_type): Define. > > > > > > > > (trees_in::key_local_type): Define. > > > > > > > > (trees_out::get_merge_kind) : Return > > > > > > > > MK_local_type for a local type. > > > > > > > > (trees_out::key_mergeable) : Use > > > > > > > > key_local_type. > > > > > > > > (trees_in::key_mergeable) : Likewise. > > > > > > > > (trees_in::is_matching_decl): Be flexible with type mismatches > > > > > > > > for local entities. > > > > > > > > > > > > > > > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > > > > > > > > index 80b63a70a62..d9e34e9a4b9 100644 > > > > > > > > --- a/gcc/cp/module.cc > > > > > > > > +++ b/gcc/cp/module.cc > > > > > > > > @@ -6714,7 +6720,37 @@ 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 type or enumerator, chain the > > > > > > > > + duplicate decl instead of the canonical in-TU decl. > > > > > > > > Seeing > > > > > > > > + a duplicate here means the containing function whose > > > > > > > > body > > > > > > > > + we're streaming in is a duplicate too, so we'll end up > > > > > > > > + discarding this BLOCK (and the rest of the duplicate > > > > > > > > function > > > > > > > > + body) anyway. */ > > > > > > > > + 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)); > > > > > > > > + } > > > > > > > > > > > > > > This seems like a lot of generally-applicable code for finding the > > > > > > > duplicate, > > > > > > > which other calls to maybe_duplicate/odr_duplicate don't use. If > > > > > > > the > > > > > > > template > > > > > > > is a duplicate, why isn't its result? If there's a good reason > > > > > > > for > > > > > > > that, > > > > > > > should this template handling go into maybe_duplicate? > > > > > > > > > > > > Ah yeah, that makes sense. > > > > > > > > > > > > Some context: IIUC modules treats the TEMPLATE_DECL instead of the > > > > > > DECL_TEMPLATE_RESULT as the canonical decl, which in turn means > > > > > > we'll > > > > > > register_duplicate only the TEMPLATE_DECL. But BLOCK_VARS never > > > > > > contains > > > > > > a TEMPLATE_DECL, always the DECL_TEMPLATE_RESULT (i.e. a TYPE_DECL), > > > > > > hence the extra handling. > > > > > > > > > > > > Given that it's relatively more difficult to get at the > > > > > > TEMPLATE_DECL > > > > > > from the DECL_TEMPLATE_RESULT rather than vice versa, maybe we > > > > > > should > > > > > > just register both as duplicates from register_duplicate? That way > > > > > > callers can just simply pass the DECL_TEMPLATE_RESULT to > > > > > > maybe_duplicate > > > > > > and it'll do the right thing. > > > > > > > > > > Sounds good. > > > > > > > > > > > > > @@ -10337,6 +10373,83 @@ trees_in::fn_parms_fini (int tag, tree > > > > > > > > fn, > > > > > > > > tree > > > > > > > > existing, bool is_defn) > > > > > > > > } > > > > > > > > } > > > > > > > > +/* Encode into KEY the position of the local type (class > > > > > > > > or > > > > > > > > enum) > > > > > > > > + 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. */ > > > > > > > > > > > > > > Since we already set DECL_DISCRIMINATOR for mangling, could we use > > > > > > > it+name > > > > > > > for > > > > > > > the key as well? > > > > > > > > > > > > We could (and IIUc that'd be more robust to ODR violations), but > > > > > > wouldn't it mean we'd have to do a linear walk over all BLOCK_VARs > > > > > > of > > > > > > all BLOCKS in order to find the one with the matching > > > > > > name+discriminator? That'd be slower than the current approach > > > > > > which > > > > > > lets us skip to the correct BLOCK and walk only its BLOCK_VARS. > > > > > > > > > > Ah, good point. How about block number + name instead of the index? > > > > > > > > It seems DECL_DISCRIMINATOR is only set at instantiation time and so for > > > > local types from a function template pattern the field is empty, which > > > > means we can't use it as the key in general :/ > > > > > > I meant just block number and name, without DECL_DISCRIMINATOR. Just > > > using > > > the name instead of an index in BLOCK_VARS. > > > > Ah, I think that'd be enough for named local types, but what about > > anonymous local types? IIUC without DECL_DISCRIMINATOR we wouldn't be > > able to reliably distinguisth between multiple anonymous local types > > defined in the same block, since their identifiers aren't stable given > > that they're based off of a global counter (and so sensitive to #include > > order) :( > > Good point. But I'd still think to merge based on name if we have one; as you > said above, to be more robust to ODR violations. Sounds good. > > If the imported fn has a local class that the included header didn't, would we > get the same problem? IIUC yes, all the intra-block indexes would be off by one in that case and deduplicatation would fail or we'd deduplicate distinct types. Here's an incremental diff for the updated patch. The augmented testcase triggered a latent qsort checking failure in depset_cmp that was straightforwardly fixed: diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 707142531dc..ee0e5da1d37 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -2933,7 +2933,7 @@ public: unsigned binfo_mergeable (tree *); private: - tree key_local_type (const merge_key&, tree); + tree key_local_type (const merge_key&, tree, tree); uintptr_t *find_duplicate (tree existing); void register_duplicate (tree decl, tree existing); /* Mark as an already diagnosed bad duplicate. */ @@ -10437,10 +10437,10 @@ trees_out::key_local_type (merge_key& key, tree decl, tree fn) } /* Look up the local type corresponding at the position encoded by - KEY within FN. */ + KEY within FN and named NAME. */ tree -trees_in::key_local_type (const merge_key& key, tree fn) +trees_in::key_local_type (const merge_key& key, tree fn, tree name) { if (!DECL_INITIAL (fn)) return NULL_TREE; @@ -10464,7 +10464,12 @@ trees_in::key_local_type (const merge_key& key, tree fn) { if (TREE_CODE (var) != TYPE_DECL) continue; - if (decl_ix == decl_pos) + /* Prefer using the identifier as the key for more robustness + to ODR violations, except for anonymous types since their + compiler-generated identifiers aren't stable. */ + if (IDENTIFIER_ANON_P (name) + ? decl_ix == decl_pos + : DECL_NAME (var) == name) return var; ++decl_ix; } @@ -11263,7 +11268,7 @@ trees_in::key_mergeable (int tag, merge_kind mk, tree decl, tree inner, case FUNCTION_DECL: gcc_checking_assert (mk == MK_local_type); - existing = key_local_type (key, container); + existing = key_local_type (key, container, name); if (existing && inner != decl) existing = TYPE_TI_TEMPLATE (TREE_TYPE (existing)); break; @@ -13801,9 +13806,9 @@ depset_cmp (const void *a_, const void *b_) { /* Both are bindings. Order by identifier hash. */ gcc_checking_assert (a->get_name () != b->get_name ()); - return (IDENTIFIER_HASH_VALUE (a->get_name ()) - < IDENTIFIER_HASH_VALUE (b->get_name ()) - ? -1 : +1); + hashval_t ah = IDENTIFIER_HASH_VALUE (a->get_name ()); + hashval_t bh = IDENTIFIER_HASH_VALUE (b->get_name ()); + return (ah == bh ? 0 : ah < bh ? -1 : +1); } /* They are the same decl. This can happen with two using decls diff --git a/gcc/testsuite/g++.dg/modules/merge-17.h b/gcc/testsuite/g++.dg/modules/merge-17.h index a5269959702..5ce52cec3dd 100644 --- a/gcc/testsuite/g++.dg/modules/merge-17.h +++ b/gcc/testsuite/g++.dg/modules/merge-17.h @@ -22,7 +22,37 @@ auto gt() { return e; } +inline auto h0() { + struct { int m; } a0; + struct { char n; } a1; + return a0; +} + +inline auto h1() { + struct { int m; } a0; + struct { char n; } a1; + return a1; +} + +template +inline auto h0t() { + struct { int m; } a0; + struct { char n; } a1; + return a0; +} + +template +inline auto h1t() { + struct { int m; } a0; + struct { char n; } a1; + return a1; +} + using ty1 = decltype(f()); using ty2 = decltype(ft()); using ty3 = decltype(g()); using ty4 = decltype(gt()); +using ty5 = decltype(h0()); +using ty6 = decltype(h0t()); +using ty7 = decltype(h1()); +using ty8 = decltype(h1t()); Complete updated patch: -- >8 -- Subject: [PATCH] c++/modules: local type merging [PR99426] PR c++/99426 gcc/cp/ChangeLog: * module.cc (merge_kind::MK_local_type): 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 types.. (trees_out::key_local_type): Define. (trees_in::key_local_type): Define. (trees_out::get_merge_kind) : Return MK_local_type for a local type. (trees_out::key_mergeable) : Use key_local_type. (trees_in::key_mergeable) : Likewise. (trees_in::is_matching_decl): Be flexible with type mismatches for local entities. (trees_in::register_duplicate): Also register the DECL_TEMPLATE_RESULT of a TEMPLATE_DECL as a duplicate. (depset_cmp): Return 0 for equal IDENTIFIER_HASH_VALUE. gcc/testsuite/ChangeLog: * g++.dg/modules/merge-17.h: New test. * g++.dg/modules/merge-17_a.H: New test. * g++.dg/modules/merge-17_b.C: New test. * g++.dg/modules/xtreme-header-7_a.H: New test. * g++.dg/modules/xtreme-header-7_b.C: New test. --- gcc/cp/module.cc | 179 +++++++++++++++--- gcc/testsuite/g++.dg/modules/merge-17.h | 58 ++++++ gcc/testsuite/g++.dg/modules/merge-17_a.H | 3 + gcc/testsuite/g++.dg/modules/merge-17_b.C | 3 + .../g++.dg/modules/xtreme-header-7_a.H | 4 + .../g++.dg/modules/xtreme-header-7_b.C | 5 + 6 files changed, 221 insertions(+), 31 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/merge-17.h create mode 100644 gcc/testsuite/g++.dg/modules/merge-17_a.H create mode 100644 gcc/testsuite/g++.dg/modules/merge-17_b.C 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 ef0280df00a..ee0e5da1d37 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -2772,6 +2772,7 @@ enum merge_kind MK_enum, /* Found by CTX, & 1stMemberNAME. */ MK_keyed, /* Found by key & index. */ + MK_local_type, /* Found by CTX, index. */ MK_friend_spec, /* Like named, but has a tmpl & args too. */ MK_local_friend, /* Found by CTX, index. */ @@ -2798,7 +2799,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 type", "friend spec", "local friend", NULL, /* 8...11 */ NULL, NULL, NULL, NULL, "type spec", "type tmpl spec", /* 16,17 type (template). */ @@ -2932,6 +2933,7 @@ public: unsigned binfo_mergeable (tree *); private: + tree key_local_type (const merge_key&, tree, tree); uintptr_t *find_duplicate (tree existing); void register_duplicate (tree decl, tree existing); /* Mark as an already diagnosed bad duplicate. */ @@ -3092,6 +3094,7 @@ public: void binfo_mergeable (tree binfo); private: + void key_local_type (merge_key&, tree, tree); bool decl_node (tree, walk_kind ref); void type_node (tree); void tree_value (tree); @@ -4959,18 +4962,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); } @@ -6244,7 +6236,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); @@ -6757,7 +6763,29 @@ 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 type or enumerator, chain the + duplicate decl instead of the canonical in-TU decl. Seeing + a duplicate here means the containing function whose body + we're streaming in is a duplicate too, so we'll end up + discarding this BLOCK (and the rest of the duplicate function + body) anyway. */ + decl = maybe_duplicate (decl); + + 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); @@ -10373,6 +10401,88 @@ trees_in::fn_parms_fini (int tag, tree fn, tree existing, bool is_defn) } } +/* Encode into KEY the position of the local type (class or enum) + 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_type (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 type corresponding at the position encoded by + KEY within FN and named NAME. */ + +tree +trees_in::key_local_type (const merge_key& key, tree fn, tree name) +{ + 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; + /* Prefer using the identifier as the key for more robustness + to ODR violations, except for anonymous types since their + compiler-generated identifiers aren't stable. */ + if (IDENTIFIER_ANON_P (name) + ? decl_ix == decl_pos + : DECL_NAME (var) == name) + 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. */ @@ -10492,17 +10602,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_type; break; case RECORD_TYPE: @@ -10804,6 +10907,10 @@ trees_out::key_mergeable (int tag, merge_kind mk, tree decl, tree inner, } break; + case MK_local_type: + key_local_type (key, STRIP_TEMPLATE (decl), container); + break; + case MK_enum: { /* Anonymous enums are located by their first identifier, @@ -11160,11 +11267,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_type); + existing = key_local_type (key, container, name); + if (existing && inner != decl) + existing = TYPE_TI_TEMPLATE (TREE_TYPE (existing)); break; case TYPE_DECL: @@ -11417,6 +11523,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, @@ -11666,6 +11777,12 @@ trees_in::register_duplicate (tree decl, tree existing) uintptr_t &slot = duplicates->get_or_insert (existing, &existed); gcc_checking_assert (!existed); slot = reinterpret_cast (decl); + if (TREE_CODE (decl) == TEMPLATE_DECL) + /* Also register the DECL_TEMPLATE_RESULT as a duplicate so + that passing the _RESULT to maybe_duplicate gives us the + existing _RESULT back. */ + register_duplicate (DECL_TEMPLATE_RESULT (decl), + DECL_TEMPLATE_RESULT (existing)); } /* We've read a definition of MAYBE_EXISTING. If not a duplicate, @@ -13689,9 +13806,9 @@ depset_cmp (const void *a_, const void *b_) { /* Both are bindings. Order by identifier hash. */ gcc_checking_assert (a->get_name () != b->get_name ()); - return (IDENTIFIER_HASH_VALUE (a->get_name ()) - < IDENTIFIER_HASH_VALUE (b->get_name ()) - ? -1 : +1); + hashval_t ah = IDENTIFIER_HASH_VALUE (a->get_name ()); + hashval_t bh = IDENTIFIER_HASH_VALUE (b->get_name ()); + return (ah == bh ? 0 : ah < bh ? -1 : +1); } /* They are the same decl. This can happen with two using decls 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..5ce52cec3dd --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/merge-17.h @@ -0,0 +1,58 @@ +// PR c++/99426 + +inline auto f() { + struct A { int m = 42; }; + return A{}; +} + +template +auto ft() { + decltype(+T()) x; + return [&x] { }; +} + +inline auto g() { + enum E { e }; + return e; +} + +template +auto gt() { + enum E : T { e }; + return e; +} + +inline auto h0() { + struct { int m; } a0; + struct { char n; } a1; + return a0; +} + +inline auto h1() { + struct { int m; } a0; + struct { char n; } a1; + return a1; +} + +template +inline auto h0t() { + struct { int m; } a0; + struct { char n; } a1; + return a0; +} + +template +inline auto h1t() { + struct { int m; } a0; + struct { char n; } a1; + return a1; +} + +using ty1 = decltype(f()); +using ty2 = decltype(ft()); +using ty3 = decltype(g()); +using ty4 = decltype(gt()); +using ty5 = decltype(h0()); +using ty6 = decltype(h0t()); +using ty7 = decltype(h1()); +using ty8 = decltype(h1t()); 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"; 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..3992a24501b --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/xtreme-header-7_b.C @@ -0,0 +1,5 @@ +// A version of xtreme-header_b.C that doesn't use -fno-module-lazy. +// { dg-additional-options -fmodules-ts } + +#include "xtreme-header.h" +import "xtreme-header-7_a.H"; -- 2.44.0.568.g436d4e5b14