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 5A5E63858D20 for ; Tue, 27 Feb 2024 02:37:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5A5E63858D20 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 5A5E63858D20 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=1709001465; cv=none; b=Do7FJRCdCfQbjxBDFclixLjxoJY21LETKeElvjcgWcQha7ZrKiGNOGMQC0zDc+OIR2KfHa3tdMMaDN3KVTvOUMFzXMpBjVf9xIupp6xAKW9TmZ/I4qwxK9DdyeVD1JQBdi4X+9G/WXVsA84DX3sqmCnl3HSF0pMSVBaYoYL57uo= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709001465; c=relaxed/simple; bh=FoUm+NuUndQ3YgWG3V1ZhQa92gM/S5AP/6mDh65yqJs=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=r2QjXhwlOXGFsah6P3+sNYTcNakh5mBp1N132B3fi7DlylCqXIWuFKQFqk0sAILSSeWDCzDmf+vFlYRPtYINIQgBwUnLri0Nm3C0LaPwICA9bfgIR1760y67MFheLQeFuNUjwSyeKV3YuvPraXmIZEg3M81/T6VIxGDY03vROqE= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1709001463; 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: content-transfer-encoding:content-transfer-encoding; bh=OkofoIgcEQFMca/e4pfl/lJis4+OSsepqPJUmwMGXCo=; b=bCRfE+1td6jtPGo01+RkRymtVXjNHAm5qWOHZfWXQiFl3VQq0nKg/nk1X8gTmSqi/X4mfV THHED06k+TAPM4rNvn3fTlybp0cywC+WP3fzPJ5mpWaB4T/OaLxLGy8d65ikfPRmVdjWoO ol9NLCLlvGqtsTOpouhbLASPfZdVVZI= Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-516-rougVEPYMZCNhWt8mWudfQ-1; Mon, 26 Feb 2024 21:37:41 -0500 X-MC-Unique: rougVEPYMZCNhWt8mWudfQ-1 Received: by mail-qk1-f199.google.com with SMTP id af79cd13be357-787e0e69f82so38993885a.2 for ; Mon, 26 Feb 2024 18:37:41 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709001460; x=1709606260; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=OkofoIgcEQFMca/e4pfl/lJis4+OSsepqPJUmwMGXCo=; b=kYVGHeTjmjSJvHdN46+Z2y50qulKuZY9MYmTHrDWnvT1o4bF3UF9T1FJHxNq7VO72G yHzQ3k12Lu1iBeVOZ8rn8+l8FOH5WPHx6r8aeTw50kgX/6P3aGytHtxLxn9BVWRrGkaq JPPNBQxlh8BU/k5INKdNeLHW9LcRPQvOQGWUtCHSxaLQ60o+P/90UmKfOmEdHba3U8WL RkqoU0aC1jSNjn7XmAJ2phkGZvrjPpBjiEkm6X26KuWg4a+UKUVbSrFM/COKkk0BkHUo eBnomD4OlRp8zRxhweeZORrBf5o+RMilCH81PltXaz+Ca1ug1A/+lsNkHeacPWav4XOU n2gw== X-Gm-Message-State: AOJu0YwezY96NywNOfWFSK+HcxE5iiWQTt4mq7d033H/P3S0KR4rElCL ZU45BOLtxLPZf5IQjGJm/f5buhWWE2yivwNqbJc0AfPk3BiP6Axc52RBREe1cuGHdFql8qugaRO 1jA+GU4uWaZnL0NJN8b10gFp3DZeMaN7YBOMQN+5q3HXhUNR6OsoUa+LERnmJEfcnN0GbyxdLF5 RU/FtPF9orljdHiemQHuXKrku+JCErNI+dU5S0 X-Received: by 2002:a05:620a:46ab:b0:787:3503:6c71 with SMTP id bq43-20020a05620a46ab00b0078735036c71mr1303935qkb.5.1709001459927; Mon, 26 Feb 2024 18:37:39 -0800 (PST) X-Google-Smtp-Source: AGHT+IEI8wKZdmRCj0slpR6UkHMtzITKK6rikyxF1/MnXA2iWesBs8tUUIh4iSsV4om/D5eUEQ6xVQ== X-Received: by 2002:a05:620a:46ab:b0:787:3503:6c71 with SMTP id bq43-20020a05620a46ab00b0078735036c71mr1303913qkb.5.1709001459292; Mon, 26 Feb 2024 18:37:39 -0800 (PST) Received: from localhost.localdomain (ool-457670bb.dyn.optonline.net. [69.118.112.187]) by smtp.gmail.com with ESMTPSA id j26-20020a05620a001a00b00787b93d8df1sm3162937qki.99.2024.02.26.18.37.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 26 Feb 2024 18:37:38 -0800 (PST) From: Patrick Palka To: gcc-patches@gcc.gnu.org Cc: jason@redhat.com, nathan@acm.org, Patrick Palka Subject: [PATCH] c++/modules: local class merging [PR99426] Date: Mon, 26 Feb 2024 21:37:34 -0500 Message-ID: <20240227023734.2742095-1-ppalka@redhat.com> X-Mailer: git-send-email 2.44.0.rc1.15.g4fc51f00ef MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true X-Spam-Status: No, score=-12.1 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,RCVD_IN_SORBS_WEB,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: 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"; -- 2.44.0.rc1.15.g4fc51f00ef