From patchwork Thu Apr 4 12:27:36 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nathaniel Shead X-Patchwork-Id: 1919787 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=h3hH/t3y; 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 4V9LW54dd7z1yYf for ; Thu, 4 Apr 2024 23:28:17 +1100 (AEDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 9C9113858424 for ; Thu, 4 Apr 2024 12:28:15 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-pf1-x42f.google.com (mail-pf1-x42f.google.com [IPv6:2607:f8b0:4864:20::42f]) by sourceware.org (Postfix) with ESMTPS id 49F7F3858C98 for ; Thu, 4 Apr 2024 12:27:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 49F7F3858C98 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 49F7F3858C98 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::42f ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712233669; cv=none; b=iPmHCMrk+Gl73IjrfXQt55VX7h/FQV24oCUNIBa3TD5PQqVGV8BSDcIOaddGSRhKkzYHwh5IlF2Ik2QH21GJNlXRY1qahVBPSiHFbp2jkEW8pNf3V6hZIFF3N7JKvFmSjrxRW+jaHc0kJgCnEFVJ3n3zVRjPjXxqHg925YAmTm8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712233669; c=relaxed/simple; bh=j305jnoXCpZY/BLIWHdlVMg9zy3n44IwB0qavmiFEXo=; h=DKIM-Signature:Message-ID:Date:From:To:Subject:MIME-Version; b=TIV8HTarzn/bsAYhCwb+LZUyoYWHxtjRsFXqMqPVhamikvhrBa+nWGywD05ygY0X+9mgsqxl+VB03cilC3O7pWcyY464iB6JyjD/NWMdU1rPKveR/y5KSveyvzNJQwI82NGDaQBV2g2PdxnX2w1VfUVNeUIogSksWRWi0aCcoEY= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pf1-x42f.google.com with SMTP id d2e1a72fcca58-6eced6fd98aso492182b3a.0 for ; Thu, 04 Apr 2024 05:27:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1712233662; x=1712838462; 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=qliGPdHiUzWeDl5HlPjAbX6Evi3mhXvgvbhJaAG4IJc=; b=h3hH/t3yctCIHvBnsu4uPU2x0WjBCX6c+2ICzzAtozCHi7F8Lm5keOpY2QorQHUdxR +JrQigSpAnuHVgGo5XoFsKELRCkAmRGed+Ue3A57nRZcmFUfcPfDLqsEEOR0FepSFAn+ WN+EyXt0SN42nqebSREl1Ms4Mdu7i6uY8v/4QON6aY4eBThajMARA+XYXipgAux0HFg6 gVlVZfCf4RmAOiXv8eYF8mZnOT4QjgAq0lE0mO+pW9zyozVfepZevV7yaL/8OamOqBOr KK9PaO1cIvgSoaYwc/yjFWijI9ID3kjBRFKnHaOlCwRHDoHiba29hSVkVGn8uy+gjkl1 jBAA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712233662; x=1712838462; 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=qliGPdHiUzWeDl5HlPjAbX6Evi3mhXvgvbhJaAG4IJc=; b=Wmc6DnFGqcvu53OlLtdu99HEOjswBRFy6LOZAGe5u7zX5zKA9L1iBYc+2N400YHGVs 2bk1AkbX7f7268tFix7xP88qOUPJtn1r6xI1LPKEHF+mVSIhNzB5ZFuxmv5AygyYjyLp 1x53Vt+xmLCZEd8n/abR9ny0AmW1vohFmd3HxnreQWLLGqNPaajqN9hl/RQoTuoR0qj1 F/m6JTbZcFiwSGWojBh973P8ZN0UnFoNVsbI3W8XBM6/1aR9ZcrIDCaN05yXOGILeO0q m4ExJ0B/dQAp/VC+5ienBwCVzklRTChGjrjBFS5rU1kFG5JyT81tJuvMPc1RcW3SEH0Y SLHQ== X-Gm-Message-State: AOJu0YyFFFZ1WFA0Fdrt0VXzClGfZHOkT/e6OWt9LkTyIYLXVUL5cVPO OUbEL52ejGmpkK7RbEls1WeFSv9UBHps3k5gi8KqgERP/Kdkq+TJ X-Google-Smtp-Source: AGHT+IG1WZh2J8h06CtafB7LmmvyxXKfwjtfmwZ4ihEjThkJ/j3aufH9tCslfFm4txz3Sr6UfTawfQ== X-Received: by 2002:a05:6a21:1509:b0:1a6:f93b:e53b with SMTP id nq9-20020a056a21150900b001a6f93be53bmr2978562pzb.49.1712233662173; Thu, 04 Apr 2024 05:27:42 -0700 (PDT) Received: from Thaum. (193-119-89-230.tpgi.com.au. [193.119.89.230]) by smtp.gmail.com with ESMTPSA id s64-20020a625e43000000b006ea8cc9250bsm14032135pfb.44.2024.04.04.05.27.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 04 Apr 2024 05:27:41 -0700 (PDT) Message-ID: <660e9cbd.620a0220.e9163.84e7@mx.google.com> X-Google-Original-Message-ID: Date: Thu, 4 Apr 2024 23:27:36 +1100 From: Nathaniel Shead To: Jason Merrill Cc: gcc-patches@gcc.gnu.org, Nathan Sidwell , Patrick Palka Subject: [PATCH v2] c++/modules: Track declarations imported from partitions [PR99377] References: <66056128.050a0220.c7f0.2cc8@mx.google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-11.3 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 Wed, Apr 03, 2024 at 02:16:25PM -0400, Jason Merrill wrote: > On 3/28/24 08:22, Nathaniel Shead wrote: > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > > > -- >8 -- > > > > The testcase in comment 15 of the linked PR is caused because the > > following assumption in depset::hash::make_dependency doesn't hold: > > > > if (DECL_LANG_SPECIFIC (not_tmpl) > > && DECL_MODULE_IMPORT_P (not_tmpl)) > > { > > /* Store the module number and index in cluster/section, > > so we don't have to look them up again. */ > > unsigned index = import_entity_index (decl); > > module_state *from = import_entity_module (index); > > /* Remap will be zero for imports from partitions, which > > we want to treat as-if declared in this TU. */ > > if (from->remap) > > { > > dep->cluster = index - from->entity_lwm; > > dep->section = from->remap; > > dep->set_flag_bit (); > > } > > } > > > > This is because at least for template specialisations, we first see the > > declaration in the header unit imported from the partition, and then the > > instantiation provided by the partition itself. This means that the > > 'import_entity_index' lookup doesn't report that the specialisation was > > declared in the partition and thus should be considered as-if it was > > part of the TU, and get exported. > > I think "exported" is the wrong term here; IIUC template specializations are > not themselves exported, just the template itself. Yes, sorry; I meant "emitted" here, in terms of whether the definition is in the CMI (regardless of whether or not that means that importers can name it). > But if the declaration or point of instantiation of the specialization is > within a module instantiation unit, it is reachable to any importers, > including the primary module interface unit importing the partition > interface unit. > > Does this work differently if "check" is a separate module rather than a > partition? Yes. When in a non-partition module (say, Bar), then the instantiation is emitted into Bar's CMI. When a caller imports Foo, it transitively streams in Bar as well when looking for the entity and imports its definition from there. However, partitions work differently. In the testcase the instantiation is emitted into Foo:check's CMI, but partition CMIs are only used within that module: importers don't know that partitions exist, and only read Foo's CMI. And because make_dependency doesn't realise that the instantiation came from a partition it hasn't emitted it into Foo's CMI which means that importers don't see a definition for it at all. > > To fix this, this patch allows, as a special case for installing an > > entity from a partition, to overwrite the entity_map entry with the > > (later) index into the partition so that this assumption holds again. > > Rather than special-casing partitions, would it make sense to override a > declaration with a definition? > > Jason > And so in this case I think that special-casing partitions is exactly what needs to happen, because the special case is that it came from a partition (rather than just it was a definition). That said, on further reflection I don't think I like the way I did this, since it means that for this instantiation, errors will refer to it as belonging to Foo:check instead of pr99377-3_a.H, which seems wrong (or at least inconsistent with existing behaviour). So maybe something like this would make more sense? (Or using a new hash_set rather than a new flag on decls, perhaps?) Bootstrapped and regtested on x86_64-pc-linux-gnu (so far just modules.exp), OK for trunk if full regtest passes? -- >8 -- The testcase in comment 15 of the linked PR is caused because the following assumption in depset::hash::make_dependency doesn't hold: if (DECL_LANG_SPECIFIC (not_tmpl) && DECL_MODULE_IMPORT_P (not_tmpl)) { /* Store the module number and index in cluster/section, so we don't have to look them up again. */ unsigned index = import_entity_index (decl); module_state *from = import_entity_module (index); /* Remap will be zero for imports from partitions, which we want to treat as-if declared in this TU. */ if (from->remap) { dep->cluster = index - from->entity_lwm; dep->section = from->remap; dep->set_flag_bit (); } } This is because at least for template specialisations, we first see the declaration in the header unit imported from the partition, and then the instantiation provided by the partition itself. This means that the 'import_entity_index' lookup doesn't report that the specialisation was declared in the partition and thus should be considered as-if it was part of the TU, and get emitted into the CMI. We always need to emit definitions from module partitions into the primary module interface's CMI, as unlike with other kinds of transitive imports the built CMIs for module partitions are not visible to importers. To fix this, rather than relying on the owning module for a declaration being the partition it was defined in, this patch adds a new flag to determine whether the declaration we read came from a partition or not, and then use that flag to determine whether we should treat the declaration as-if it was provided in this TU. PR c++/99377 gcc/cp/ChangeLog: * cp-tree.h (DECL_MODULE_PARTITION_P): New. (struct lang_decl_base): Add module_partition_p. * module.cc (trees_in::install_entity): Set DECL_MODULE_PARTITION_P when installing from partition. (depset::hash::make_dependency): Use DECL_MODULE_PARTITION_P to determine if we treat a decl as-if it came from this TU. gcc/testsuite/ChangeLog: * g++.dg/modules/pr99377-3_a.H: New test. * g++.dg/modules/pr99377-3_b.C: New test. * g++.dg/modules/pr99377-3_c.C: New test. * g++.dg/modules/pr99377-3_d.C: New test. Signed-off-by: Nathaniel Shead --- gcc/cp/cp-tree.h | 10 ++++++++-- gcc/cp/module.cc | 23 ++++++++++++++-------- gcc/testsuite/g++.dg/modules/pr99377-3_a.H | 17 ++++++++++++++++ gcc/testsuite/g++.dg/modules/pr99377-3_b.C | 10 ++++++++++ gcc/testsuite/g++.dg/modules/pr99377-3_c.C | 5 +++++ gcc/testsuite/g++.dg/modules/pr99377-3_d.C | 8 ++++++++ 6 files changed, 63 insertions(+), 10 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/pr99377-3_a.H create mode 100644 gcc/testsuite/g++.dg/modules/pr99377-3_b.C create mode 100644 gcc/testsuite/g++.dg/modules/pr99377-3_c.C create mode 100644 gcc/testsuite/g++.dg/modules/pr99377-3_d.C diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 52d53589e51..802ae002ee2 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -1772,6 +1772,11 @@ check_constraint_info (tree t) #define DECL_MODULE_ENTITY_P(NODE) \ (DECL_LANG_SPECIFIC (DECL_MODULE_CHECK (NODE))->u.base.module_entity_p) +/* True if this decl was provided from a module partition, and should be + treated as-if it was declared in this TU. */ +#define DECL_MODULE_PARTITION_P(NODE) \ + (DECL_LANG_SPECIFIC (DECL_MODULE_CHECK (NODE))->u.base.module_partition_p) + /* DECL that has attached decls for ODR-relatedness. */ #define DECL_MODULE_KEYED_DECLS_P(NODE) \ (DECL_LANG_SPECIFIC (DECL_MODULE_CHECK (NODE))->u.base.module_keyed_decls_p) @@ -2890,16 +2895,17 @@ struct GTY(()) lang_decl_base { unsigned var_declared_inline_p : 1; /* var */ unsigned dependent_init_p : 1; /* var */ - /* The following four apply to VAR, FUNCTION, TYPE, CONCEPT, & NAMESPACE + /* The following five apply to VAR, FUNCTION, TYPE, CONCEPT, & NAMESPACE decls. */ unsigned module_purview_p : 1; /* in named-module purview */ unsigned module_attach_p : 1; /* attached to named module */ unsigned module_import_p : 1; /* from an import */ unsigned module_entity_p : 1; /* is in the entitity ary & hash */ + unsigned module_partition_p : 1; /* imported from a partition */ unsigned module_keyed_decls_p : 1; /* has keys, applies to all decls */ - /* 11 spare bits. */ + /* 10 spare bits. */ }; /* True for DECL codes which have template info and access. */ diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 8aab9ea0bae..29e2930485a 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -7650,6 +7650,11 @@ trees_in::install_entity (tree decl) slot = ident; } + if (state->is_partition ()) + /* Remember that this came from a partition, even if we've seen it from a + non-partition before (e.g. for template instantiations). */ + DECL_MODULE_PARTITION_P (not_tmpl) = true; + return true; } @@ -12717,20 +12722,22 @@ depset::hash::make_dependency (tree decl, entity_kind ek) tree not_tmpl = STRIP_TEMPLATE (decl); if (DECL_LANG_SPECIFIC (not_tmpl) - && DECL_MODULE_IMPORT_P (not_tmpl)) + && DECL_MODULE_IMPORT_P (not_tmpl) + /* Treat entities imported from partitions as-if declared in + this TU, unless we ourselves are also a partition. */ + && (!DECL_MODULE_PARTITION_P (not_tmpl) + || module_partition_p ())) { /* Store the module number and index in cluster/section, so we don't have to look them up again. */ unsigned index = import_entity_index (decl); module_state *from = import_entity_module (index); /* Remap will be zero for imports from partitions, which - we want to treat as-if declared in this TU. */ - if (from->remap) - { - dep->cluster = index - from->entity_lwm; - dep->section = from->remap; - dep->set_flag_bit (); - } + should have been covered by the check above. */ + gcc_assert (from->remap); + dep->cluster = index - from->entity_lwm; + dep->section = from->remap; + dep->set_flag_bit (); } if (ek == EK_DECL diff --git a/gcc/testsuite/g++.dg/modules/pr99377-3_a.H b/gcc/testsuite/g++.dg/modules/pr99377-3_a.H new file mode 100644 index 00000000000..580a7631ae1 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr99377-3_a.H @@ -0,0 +1,17 @@ +// { dg-additional-options "-fmodule-header" } +// { dg-module-cmi {} } + +template +struct Widget +{ + Widget (int) { } + + bool First() const { return true; } + + bool Second () const { return true;} +}; + +inline void Frob (const Widget& w) noexcept +{ + w.First (); +} diff --git a/gcc/testsuite/g++.dg/modules/pr99377-3_b.C b/gcc/testsuite/g++.dg/modules/pr99377-3_b.C new file mode 100644 index 00000000000..5cbce7b3544 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr99377-3_b.C @@ -0,0 +1,10 @@ +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi Foo:check } + +export module Foo:check; +import "pr99377-3_a.H"; + +export inline bool Check (const Widget& w) +{ + return w.Second (); +} diff --git a/gcc/testsuite/g++.dg/modules/pr99377-3_c.C b/gcc/testsuite/g++.dg/modules/pr99377-3_c.C new file mode 100644 index 00000000000..fa7c24203bd --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr99377-3_c.C @@ -0,0 +1,5 @@ +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi Foo } + +export module Foo; +export import :check; diff --git a/gcc/testsuite/g++.dg/modules/pr99377-3_d.C b/gcc/testsuite/g++.dg/modules/pr99377-3_d.C new file mode 100644 index 00000000000..cb1f28321b1 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr99377-3_d.C @@ -0,0 +1,8 @@ +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts" } + +import Foo; + +int main() { + return Check(0) ? 0 : 1; +}