* [PATCH] c++/modules: Prevent revealing a using-decl affecting cached overloads [PR114867]
@ 2024-05-26 13:01 Nathaniel Shead
2024-05-28 18:57 ` Jason Merrill
0 siblings, 1 reply; 4+ messages in thread
From: Nathaniel Shead @ 2024-05-26 13:01 UTC (permalink / raw)
To: gcc-patches; +Cc: Jason Merrill, Nathan Sidwell
Is this approach OK? Alternatively I suppose we could do a deep copy of
the overload list when this occurs to ensure we don't affect existing
referents, would that be preferable?
Bootstrapped and regtested (so far just modules.exp) on
x86_64-pc-linux-gnu, OK for trunk if full regtest succeeds?
-- >8 --
Doing 'remove_node' here is not safe, because it not only mutates the
OVERLOAD we're walking over but potentially any other references to this
OVERLOAD that are cached from phase-1 template lookup. This causes the
attached testcase to fail because the overload set in X::test no longer
contains the 'ns::foo' template once instantiated at the end of the
file.
This patch works around this by simply not removing the old declaration.
This does make the overload list potentially longer than it otherwise
would have been, but only when re-exporting the same set of functions in
a using-decl. Additionally, because 'ovl_insert' always prepends these
newly inserted overloads, repeated exported using-decls won't continue
to add declarations, as the first exported using-decl will be found
before the original (unexported) declaration.
PR c++/114867
gcc/cp/ChangeLog:
* name-lookup.cc (do_nonmember_using_decl): Don't remove the
existing overload.
gcc/testsuite/ChangeLog:
* g++.dg/modules/using-17_a.C: New test.
* g++.dg/modules/using-17_b.C: New test.
Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
gcc/cp/name-lookup.cc | 24 +++++++-----------
gcc/testsuite/g++.dg/modules/using-17_a.C | 31 +++++++++++++++++++++++
gcc/testsuite/g++.dg/modules/using-17_b.C | 13 ++++++++++
3 files changed, 53 insertions(+), 15 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/modules/using-17_a.C
create mode 100644 gcc/testsuite/g++.dg/modules/using-17_b.C
diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
index f1f8c19feb1..130a0e6b5db 100644
--- a/gcc/cp/name-lookup.cc
+++ b/gcc/cp/name-lookup.cc
@@ -5231,25 +5231,19 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
if (new_fn == old_fn)
{
- /* The function already exists in the current
- namespace. We will still want to insert it if
- it is revealing a not-revealed thing. */
+ /* The function already exists in the current namespace. */
found = true;
- if (!revealing_p)
- ;
- else if (old.using_p ())
+ if (exporting)
{
- if (exporting)
+ if (old.using_p ())
/* Update in place. 'tis ok. */
OVL_EXPORT_P (old.get_using ()) = true;
- ;
- }
- else if (DECL_MODULE_EXPORT_P (new_fn))
- ;
- else
- {
- value = old.remove_node (value);
- found = false;
+ else if (!DECL_MODULE_EXPORT_P (new_fn))
+ /* We need to re-insert this function as an exported
+ declaration. We can't remove the existing decl
+ because that will change any overloads cached in
+ template functions. */
+ found = false;
}
break;
}
diff --git a/gcc/testsuite/g++.dg/modules/using-17_a.C b/gcc/testsuite/g++.dg/modules/using-17_a.C
new file mode 100644
index 00000000000..de601ea2be0
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/using-17_a.C
@@ -0,0 +1,31 @@
+// PR c++/114867
+// { dg-additional-options "-fmodules-ts -Wno-global-module" }
+// { dg-module-cmi M }
+
+module;
+
+namespace ns {
+ template <typename T> void f(T);
+
+ namespace inner {
+ class E {};
+ int f(E);
+ }
+ using inner::f;
+}
+
+export module M;
+
+template <typename T>
+struct X {
+ void test() { ns::f(T{}); }
+};
+template struct X<int>;
+
+export namespace ns {
+ using ns::f;
+}
+
+export auto get_e() {
+ return ns::inner::E{};
+}
diff --git a/gcc/testsuite/g++.dg/modules/using-17_b.C b/gcc/testsuite/g++.dg/modules/using-17_b.C
new file mode 100644
index 00000000000..e31582110e5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/using-17_b.C
@@ -0,0 +1,13 @@
+// PR c++/114867
+// { dg-additional-options "-fmodules-ts" }
+
+import M;
+
+int main() {
+ int a = 0;
+ ns::f(a);
+
+ // Should also still find the inner::f overload
+ auto e = get_e();
+ int r = ns::f(e);
+}
--
2.43.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] c++/modules: Prevent revealing a using-decl affecting cached overloads [PR114867]
2024-05-26 13:01 [PATCH] c++/modules: Prevent revealing a using-decl affecting cached overloads [PR114867] Nathaniel Shead
@ 2024-05-28 18:57 ` Jason Merrill
2024-05-31 15:57 ` [PATCH v2] c++/modules: Fix revealing with using-decls [PR114867] Nathaniel Shead
0 siblings, 1 reply; 4+ messages in thread
From: Jason Merrill @ 2024-05-28 18:57 UTC (permalink / raw)
To: Nathaniel Shead, gcc-patches; +Cc: Nathan Sidwell
On 5/26/24 09:01, Nathaniel Shead wrote:
> Is this approach OK? Alternatively I suppose we could do a deep copy of
> the overload list when this occurs to ensure we don't affect existing
> referents, would that be preferable?
This strategy makes sense, but I have other concerns:
> Bootstrapped and regtested (so far just modules.exp) on
> x86_64-pc-linux-gnu, OK for trunk if full regtest succeeds?
>
> -- >8 --
>
> Doing 'remove_node' here is not safe, because it not only mutates the
> OVERLOAD we're walking over but potentially any other references to this
> OVERLOAD that are cached from phase-1 template lookup. This causes the
> attached testcase to fail because the overload set in X::test no longer
> contains the 'ns::foo' template once instantiated at the end of the
It looks like ns::foo has been renamed to just f in the testcase.
> file.
>
> This patch works around this by simply not removing the old declaration.
> This does make the overload list potentially longer than it otherwise
> would have been, but only when re-exporting the same set of functions in
> a using-decl. Additionally, because 'ovl_insert' always prepends these
> newly inserted overloads, repeated exported using-decls won't continue
> to add declarations, as the first exported using-decl will be found
> before the original (unexported) declaration.
>
> PR c++/114867
>
> gcc/cp/ChangeLog:
>
> * name-lookup.cc (do_nonmember_using_decl): Don't remove the
> existing overload.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/modules/using-17_a.C: New test.
> * g++.dg/modules/using-17_b.C: New test.
>
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
> gcc/cp/name-lookup.cc | 24 +++++++-----------
> gcc/testsuite/g++.dg/modules/using-17_a.C | 31 +++++++++++++++++++++++
> gcc/testsuite/g++.dg/modules/using-17_b.C | 13 ++++++++++
> 3 files changed, 53 insertions(+), 15 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/modules/using-17_a.C
> create mode 100644 gcc/testsuite/g++.dg/modules/using-17_b.C
>
> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> index f1f8c19feb1..130a0e6b5db 100644
> --- a/gcc/cp/name-lookup.cc
> +++ b/gcc/cp/name-lookup.cc
> @@ -5231,25 +5231,19 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
>
> if (new_fn == old_fn)
> {
> - /* The function already exists in the current
> - namespace. We will still want to insert it if
> - it is revealing a not-revealed thing. */
> + /* The function already exists in the current namespace. */
> found = true;
> - if (!revealing_p)
> - ;
> - else if (old.using_p ())
> + if (exporting)
> {
> - if (exporting)
> + if (old.using_p ())
> /* Update in place. 'tis ok. */
> OVL_EXPORT_P (old.get_using ()) = true;
> - ;
> - }
> - else if (DECL_MODULE_EXPORT_P (new_fn))
> - ;
> - else
> - {
> - value = old.remove_node (value);
> - found = false;
> + else if (!DECL_MODULE_EXPORT_P (new_fn))
> + /* We need to re-insert this function as an exported
> + declaration. We can't remove the existing decl
> + because that will change any overloads cached in
> + template functions. */
> + found = false;
What if we're revealing without exporting? That is, a using-declaration
in module purview that isn't exported? Such a declaration should still
prevent discarding, which is my understanding of the use of "revealing"
here.
It seems like the current code already gets that wrong for e.g.
M_1.C:
module;
struct A {};
inline int f() { return 42; }
export module M;
using ::A;
using ::f;
M_2.C:
import M;
inline int f();
struct A a; // { dg-bogus "incomplete" }
int main() {
return f(); // { dg-bogus "undefined" }
}
It looks like part of the problem is that add_binding_entity is only
interested in exported usings, but I think it should also handle
revealing ones.
Jason
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] c++/modules: Fix revealing with using-decls [PR114867]
2024-05-28 18:57 ` Jason Merrill
@ 2024-05-31 15:57 ` Nathaniel Shead
2024-05-31 20:23 ` Jason Merrill
0 siblings, 1 reply; 4+ messages in thread
From: Nathaniel Shead @ 2024-05-31 15:57 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches, Nathan Sidwell
On Tue, May 28, 2024 at 02:57:09PM -0400, Jason Merrill wrote:
> On 5/26/24 09:01, Nathaniel Shead wrote:
> > Is this approach OK? Alternatively I suppose we could do a deep copy of
> > the overload list when this occurs to ensure we don't affect existing
> > referents, would that be preferable?
>
> This strategy makes sense, but I have other concerns:
>
> > Bootstrapped and regtested (so far just modules.exp) on
> > x86_64-pc-linux-gnu, OK for trunk if full regtest succeeds?
> >
> > -- >8 --
> >
> > Doing 'remove_node' here is not safe, because it not only mutates the
> > OVERLOAD we're walking over but potentially any other references to this
> > OVERLOAD that are cached from phase-1 template lookup. This causes the
> > attached testcase to fail because the overload set in X::test no longer
> > contains the 'ns::foo' template once instantiated at the end of the
>
> It looks like ns::foo has been renamed to just f in the testcase.
>
Whoops, fixed.
> > file.
> >
> > This patch works around this by simply not removing the old declaration.
> > This does make the overload list potentially longer than it otherwise
> > would have been, but only when re-exporting the same set of functions in
> > a using-decl. Additionally, because 'ovl_insert' always prepends these
> > newly inserted overloads, repeated exported using-decls won't continue
> > to add declarations, as the first exported using-decl will be found
> > before the original (unexported) declaration.
> >
> > PR c++/114867
> >
> > gcc/cp/ChangeLog:
> >
> > * name-lookup.cc (do_nonmember_using_decl): Don't remove the
> > existing overload.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/modules/using-17_a.C: New test.
> > * g++.dg/modules/using-17_b.C: New test.
> >
> > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > ---
> > gcc/cp/name-lookup.cc | 24 +++++++-----------
> > gcc/testsuite/g++.dg/modules/using-17_a.C | 31 +++++++++++++++++++++++
> > gcc/testsuite/g++.dg/modules/using-17_b.C | 13 ++++++++++
> > 3 files changed, 53 insertions(+), 15 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/modules/using-17_a.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/using-17_b.C
> >
> > diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> > index f1f8c19feb1..130a0e6b5db 100644
> > --- a/gcc/cp/name-lookup.cc
> > +++ b/gcc/cp/name-lookup.cc
> > @@ -5231,25 +5231,19 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
> > if (new_fn == old_fn)
> > {
> > - /* The function already exists in the current
> > - namespace. We will still want to insert it if
> > - it is revealing a not-revealed thing. */
> > + /* The function already exists in the current namespace. */
> > found = true;
> > - if (!revealing_p)
> > - ;
> > - else if (old.using_p ())
> > + if (exporting)
> > {
> > - if (exporting)
> > + if (old.using_p ())
> > /* Update in place. 'tis ok. */
> > OVL_EXPORT_P (old.get_using ()) = true;
> > - ;
> > - }
> > - else if (DECL_MODULE_EXPORT_P (new_fn))
> > - ;
> > - else
> > - {
> > - value = old.remove_node (value);
> > - found = false;
> > + else if (!DECL_MODULE_EXPORT_P (new_fn))
> > + /* We need to re-insert this function as an exported
> > + declaration. We can't remove the existing decl
> > + because that will change any overloads cached in
> > + template functions. */
> > + found = false;
>
> What if we're revealing without exporting? That is, a using-declaration in
> module purview that isn't exported? Such a declaration should still prevent
> discarding, which is my understanding of the use of "revealing" here.
>
> It seems like the current code already gets that wrong for e.g.
>
> M_1.C:
> module;
> struct A {};
> inline int f() { return 42; }
> export module M;
> using ::A;
> using ::f;
>
> M_2.C:
> import M;
> inline int f();
> struct A a; // { dg-bogus "incomplete" }
> int main() {
> return f(); // { dg-bogus "undefined" }
> }
>
> It looks like part of the problem is that add_binding_entity is only
> interested in exported usings, but I think it should also handle revealing
> ones.
>
> Jason
>
Right; I hadn't thought about that. The cleanest way to solve this I
think is to add a new flag to OVERLOAD to indicate their purviewness,
which we can then use in 'add_binding_entity' instead of the current
reliance on exported usings; this is what I've done in the below patch.
(There aren't any more TREE_LANG_FLAG_?s left so I just picked another
unused flag lying around; alternatively I could create _7, there does
seem to be spare bits in tree_base.)
Another option would be to do like what I've done in my workaround for
non-functions and just mark the original decl as being in PURVIEW_P; I'm
not a huge fan of this though.
Bootstrapped and regtested (so far just dg.exp and modules.exp) on
x86_64-pc-linux-gnu, OK for trunk if full regtest succeeds?
-- >8 --
This patch fixes a couple issues with the current handling of revealing
declarations with using-decls.
Firstly, doing 'remove_node' when handling function overload sets is not
safe, because it not only mutates the OVERLOAD we're walking over but
potentially any other references to this OVERLOAD that are cached from
phase-1 template lookup. This causes the attached using-17 testcase to
fail because the overload set in 'X::test()' no longer contains the
'ns::f(T)' template once instantiated at the end of the file.
This patch works around this by simply not removing the old declaration.
This does make the overload list potentially longer than it otherwise
would have been, but only when re-exporting the same set of functions in
a using-decl. Additionally, because 'ovl_insert' always prepends these
newly inserted overloads, repeated exported using-decls won't continue
to add declarations, as the first exported using-decl will be found
before the original (unexported) declaration.
Another, related, issue is that using-decls of GMF entities currently
doesn't mark them as reachable unless they are also exported, and thus
they may not be available in e.g. module implementation units. We solve
this with a new flag on OVERLOADs set when they are declared within the
module purview. This starts to run into the more general issue of
handling using-decls of non-functions (see e.g. PR114863) but by just
marking such GMF entities as purview we can work around this for now.
This also allows us to get rid of the special-casing of exported
using-decls in 'add_binding_entity', which was incorrect anyway: a
non-exported using-decl still needs to be emitted anyway if it lives in
the module purview, even if referring to a non-purview item.
PR c++/114867
gcc/cp/ChangeLog:
* cp-tree.h (OVL_PURVIEW_P): New.
(ovl_iterator::purview_p): New.
* module.cc (depset::hash::add_binding_entity): Only ignore
entities not within module purview. Set OVL_PURVIEW_P on new
OVERLOADs for emitted declarations.
(module_state::read_cluster): Imported using-decls are always
in purview, mark as OVL_PURVIEW_P.
* name-lookup.h (enum WMB_Flags): New WMB_Purview flag.
* name-lookup.cc (walk_module_binding): Set WMB_Purview as
needed.
(do_nonmember_using_decl): Don't remove from existing OVERLOADs.
Also reveal non-exported decls. Also reveal 'extern "C"' decls.
Add workaround to reveal non-function decls.
* tree.cc (ovl_insert): Adjust to also set OVL_PURVIEW_P when
needed.
gcc/testsuite/ChangeLog:
* g++.dg/modules/using-17_a.C: New test.
* g++.dg/modules/using-17_b.C: New test.
* g++.dg/modules/using-18_a.C: New test.
* g++.dg/modules/using-18_b.C: New test.
Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
gcc/cp/cp-tree.h | 7 ++
gcc/cp/module.cc | 8 +-
gcc/cp/name-lookup.cc | 106 ++++++++++++----------
gcc/cp/name-lookup.h | 1 +
gcc/cp/tree.cc | 12 ++-
gcc/testsuite/g++.dg/modules/using-17_a.C | 31 +++++++
gcc/testsuite/g++.dg/modules/using-17_b.C | 13 +++
gcc/testsuite/g++.dg/modules/using-18_a.C | 29 ++++++
gcc/testsuite/g++.dg/modules/using-18_b.C | 11 +++
9 files changed, 161 insertions(+), 57 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/modules/using-17_a.C
create mode 100644 gcc/testsuite/g++.dg/modules/using-17_b.C
create mode 100644 gcc/testsuite/g++.dg/modules/using-18_a.C
create mode 100644 gcc/testsuite/g++.dg/modules/using-18_b.C
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 655850a9ab6..bf3dd454b0f 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -813,6 +813,8 @@ typedef struct ptrmem_cst * ptrmem_cst_t;
#define OVL_LOOKUP_P(NODE) TREE_LANG_FLAG_4 (OVERLOAD_CHECK (NODE))
/* If set, this OVL_USING_P overload is exported. */
#define OVL_EXPORT_P(NODE) TREE_LANG_FLAG_5 (OVERLOAD_CHECK (NODE))
+/* If set, this OVL_USING_P overload is in the module purview. */
+#define OVL_PURVIEW_P(NODE) (OVERLOAD_CHECK (NODE)->base.public_flag)
/* If set, this overload includes name-independent declarations. */
#define OVL_NAME_INDEPENDENT_DECL_P(NODE) \
TREE_LANG_FLAG_6 (OVERLOAD_CHECK (NODE))
@@ -887,6 +889,11 @@ class ovl_iterator {
return (TREE_CODE (ovl) == USING_DECL
|| (TREE_CODE (ovl) == OVERLOAD && OVL_USING_P (ovl)));
}
+ /* Whether this using is in the module purview. */
+ bool purview_p () const
+ {
+ return OVL_PURVIEW_P (get_using ());
+ }
/* Whether this using is being exported. */
bool exporting_p () const
{
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 3f8f84bb9fd..ed24814b601 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -13137,9 +13137,8 @@ depset::hash::add_binding_entity (tree decl, WMB_Flags flags, void *data_)
inner = DECL_TEMPLATE_RESULT (inner);
if ((!DECL_LANG_SPECIFIC (inner) || !DECL_MODULE_PURVIEW_P (inner))
- && !((flags & WMB_Using) && (flags & WMB_Export)))
- /* Ignore global module fragment entities unless explicitly
- exported with a using declaration. */
+ && !((flags & WMB_Using) && (flags & WMB_Purview)))
+ /* Ignore entities not within the module purview. */
return false;
if (VAR_OR_FUNCTION_DECL_P (inner)
@@ -13189,6 +13188,7 @@ depset::hash::add_binding_entity (tree decl, WMB_Flags flags, void *data_)
{
if (!(flags & WMB_Hidden))
d->clear_hidden_binding ();
+ OVL_PURVIEW_P (d->get_entity ()) = true;
if (flags & WMB_Export)
OVL_EXPORT_P (d->get_entity ()) = true;
return bool (flags & WMB_Export);
@@ -13230,6 +13230,7 @@ depset::hash::add_binding_entity (tree decl, WMB_Flags flags, void *data_)
decl = ovl_make (decl, NULL_TREE);
if (!unscoped_enum_const_p)
OVL_USING_P (decl) = true;
+ OVL_PURVIEW_P (decl) = true;
if (flags & WMB_Export)
OVL_EXPORT_P (decl) = true;
}
@@ -15311,6 +15312,7 @@ module_state::read_cluster (unsigned snum)
{
dedup = true;
OVL_USING_P (decls) = true;
+ OVL_PURVIEW_P (decls) = true;
if (flags & cbf_export)
OVL_EXPORT_P (decls) = true;
}
diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
index f1f8c19feb1..b7adf58eacd 100644
--- a/gcc/cp/name-lookup.cc
+++ b/gcc/cp/name-lookup.cc
@@ -4234,6 +4234,8 @@ walk_module_binding (tree binding, bitmap partitions,
if (iter.using_p ())
{
flags = WMB_Flags (flags | WMB_Using);
+ if (iter.purview_p ())
+ flags = WMB_Flags (flags | WMB_Purview);
if (iter.exporting_p ())
flags = WMB_Flags (flags | WMB_Export);
}
@@ -4307,6 +4309,8 @@ walk_module_binding (tree binding, bitmap partitions,
if (iter.using_p ())
{
flags = WMB_Flags (flags | WMB_Using);
+ if (iter.purview_p ())
+ flags = WMB_Flags (flags | WMB_Purview);
if (iter.exporting_p ())
flags = WMB_Flags (flags | WMB_Export);
}
@@ -5210,9 +5214,10 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
for (lkp_iterator usings (lookup.value); usings; ++usings)
{
tree new_fn = *usings;
- bool exporting = revealing_p && module_exporting_p ();
- if (exporting)
- exporting = check_can_export_using_decl (new_fn);
+ tree inner = STRIP_TEMPLATE (new_fn);
+ bool exporting_p = revealing_p && module_exporting_p ();
+ if (exporting_p)
+ exporting_p = check_can_export_using_decl (new_fn);
/* [namespace.udecl]
@@ -5239,18 +5244,18 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
;
else if (old.using_p ())
{
- if (exporting)
- /* Update in place. 'tis ok. */
+ /* Update in place. 'tis ok. */
+ OVL_PURVIEW_P (old.get_using ()) = true;
+ if (exporting_p)
OVL_EXPORT_P (old.get_using ()) = true;
- ;
- }
- else if (DECL_MODULE_EXPORT_P (new_fn))
- ;
- else
- {
- value = old.remove_node (value);
- found = false;
}
+ else if (!DECL_LANG_SPECIFIC (inner)
+ || !DECL_MODULE_PURVIEW_P (inner))
+ /* We need to re-insert this function as a revealed
+ (possibly exported) declaration. We can't remove
+ the existing decl because that will change any
+ overloads cached in template functions. */
+ found = false;
break;
}
else if (old.using_p ())
@@ -5261,8 +5266,14 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
continue; /* Parameters do not match. */
else if (decls_match (new_fn, old_fn))
{
- /* Extern "C" in different namespaces. */
+ /* Extern "C" in different namespaces. But similarly
+ to above, if revealing a not-revealed thing we may
+ need to reinsert. */
found = true;
+ if (revealing_p
+ && (!DECL_LANG_SPECIFIC (inner)
+ || !DECL_MODULE_PURVIEW_P (inner)))
+ found = false;
break;
}
else
@@ -5278,7 +5289,7 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
/* Unlike the decl-pushing case we don't drop anticipated
builtins here. They don't cause a problem, and we'd
like to match them with a future declaration. */
- value = ovl_insert (new_fn, value, 1 + exporting);
+ value = ovl_insert (new_fn, value, 1 + revealing_p + exporting_p);
}
}
else if (value
@@ -5291,32 +5302,32 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
}
else if (insert_p)
{
- if (revealing_p
- && module_exporting_p ()
- && check_can_export_using_decl (lookup.value)
- && lookup.value == value
- && !DECL_MODULE_EXPORT_P (value))
+ /* FIXME: Handle exporting declarations from a different scope
+ without also marking those declarations as exported.
+ This will require not just binding directly to the underlying
+ value; see c++/114863 and c++/114865. We allow this for purview
+ declarations for now as this doesn't (currently) cause ICEs
+ later down the line, but should also be revisited then. */
+ if (revealing_p)
{
- /* We're redeclaring the same value, but this time as
- newly exported: make sure to mark it as such. */
- if (TREE_CODE (value) == TEMPLATE_DECL)
- {
- DECL_MODULE_EXPORT_P (value) = true;
-
- tree result = DECL_TEMPLATE_RESULT (value);
- retrofit_lang_decl (result);
- DECL_MODULE_PURVIEW_P (result) = true;
- DECL_MODULE_EXPORT_P (result) = true;
- }
- else
+ if (module_exporting_p ()
+ && check_can_export_using_decl (lookup.value)
+ && lookup.value == value
+ && !DECL_MODULE_EXPORT_P (lookup.value))
{
- retrofit_lang_decl (value);
- DECL_MODULE_PURVIEW_P (value) = true;
- DECL_MODULE_EXPORT_P (value) = true;
+ /* We're redeclaring the same value, but this time as
+ newly exported: make sure to mark it as such. */
+ if (TREE_CODE (lookup.value) == TEMPLATE_DECL)
+ DECL_MODULE_EXPORT_P (DECL_TEMPLATE_RESULT (lookup.value)) = true;
+ DECL_MODULE_EXPORT_P (lookup.value) = true;
}
+ /* We're also revealing this declaration with a using-decl,
+ so mark it as purview to ensure it's emitted. */
+ tree inner = STRIP_TEMPLATE (lookup.value);
+ retrofit_lang_decl (inner);
+ DECL_MODULE_PURVIEW_P (inner) = true;
}
- else
- value = lookup.value;
+ value = lookup.value;
}
/* Now the type binding. */
@@ -5329,20 +5340,17 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
}
else if (insert_p)
{
- if (revealing_p
- && module_exporting_p ()
- && check_can_export_using_decl (lookup.type)
- && lookup.type == type
- && !DECL_MODULE_EXPORT_P (type))
+ if (revealing_p)
{
- /* We're redeclaring the same type, but this time as
- newly exported: make sure to mark it as such. */
- retrofit_lang_decl (type);
- DECL_MODULE_PURVIEW_P (type) = true;
- DECL_MODULE_EXPORT_P (type) = true;
+ /* As with revealing value bindings. */
+ if (module_exporting_p ()
+ && check_can_export_using_decl (lookup.type)
+ && lookup.type == type)
+ DECL_MODULE_EXPORT_P (lookup.type) = true;
+ retrofit_lang_decl (lookup.type);
+ DECL_MODULE_PURVIEW_P (lookup.type) = true;
}
- else
- type = lookup.type;
+ type = lookup.type;
}
}
diff --git a/gcc/cp/name-lookup.h b/gcc/cp/name-lookup.h
index d2371323337..5cf6ae6374a 100644
--- a/gcc/cp/name-lookup.h
+++ b/gcc/cp/name-lookup.h
@@ -495,6 +495,7 @@ enum WMB_Flags
WMB_Export = 1 << 1,
WMB_Using = 1 << 2,
WMB_Hidden = 1 << 3,
+ WMB_Purview = 1 << 4,
};
extern unsigned walk_module_binding (tree binding, bitmap partitions,
diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc
index fe3f034d000..fcd226ba028 100644
--- a/gcc/cp/tree.cc
+++ b/gcc/cp/tree.cc
@@ -2353,11 +2353,11 @@ ovl_make (tree fn, tree next)
return result;
}
-/* Add FN to the (potentially NULL) overload set OVL. USING_OR_HIDDEN is >
- zero if this is a using-decl. It is > 1 if we're exporting the
- using decl. USING_OR_HIDDEN is < 0, if FN is hidden. (A decl
- cannot be both using and hidden.) We keep the hidden decls first,
- but remaining ones are unordered. */
+/* Add FN to the (potentially NULL) overload set OVL. USING_OR_HIDDEN is > 0
+ if this is a using-decl. It is > 1 if we're revealing the using decl.
+ It is > 2 if we're also exporting it. USING_OR_HIDDEN is < 0, if FN is
+ hidden. (A decl cannot be both using and hidden.) We keep the hidden
+ decls first, but remaining ones are unordered. */
tree
ovl_insert (tree fn, tree maybe_ovl, int using_or_hidden)
@@ -2384,6 +2384,8 @@ ovl_insert (tree fn, tree maybe_ovl, int using_or_hidden)
{
OVL_DEDUP_P (maybe_ovl) = OVL_USING_P (maybe_ovl) = true;
if (using_or_hidden > 1)
+ OVL_PURVIEW_P (maybe_ovl) = true;
+ if (using_or_hidden > 2)
OVL_EXPORT_P (maybe_ovl) = true;
}
}
diff --git a/gcc/testsuite/g++.dg/modules/using-17_a.C b/gcc/testsuite/g++.dg/modules/using-17_a.C
new file mode 100644
index 00000000000..de601ea2be0
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/using-17_a.C
@@ -0,0 +1,31 @@
+// PR c++/114867
+// { dg-additional-options "-fmodules-ts -Wno-global-module" }
+// { dg-module-cmi M }
+
+module;
+
+namespace ns {
+ template <typename T> void f(T);
+
+ namespace inner {
+ class E {};
+ int f(E);
+ }
+ using inner::f;
+}
+
+export module M;
+
+template <typename T>
+struct X {
+ void test() { ns::f(T{}); }
+};
+template struct X<int>;
+
+export namespace ns {
+ using ns::f;
+}
+
+export auto get_e() {
+ return ns::inner::E{};
+}
diff --git a/gcc/testsuite/g++.dg/modules/using-17_b.C b/gcc/testsuite/g++.dg/modules/using-17_b.C
new file mode 100644
index 00000000000..e31582110e5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/using-17_b.C
@@ -0,0 +1,13 @@
+// PR c++/114867
+// { dg-additional-options "-fmodules-ts" }
+
+import M;
+
+int main() {
+ int a = 0;
+ ns::f(a);
+
+ // Should also still find the inner::f overload
+ auto e = get_e();
+ int r = ns::f(e);
+}
diff --git a/gcc/testsuite/g++.dg/modules/using-18_a.C b/gcc/testsuite/g++.dg/modules/using-18_a.C
new file mode 100644
index 00000000000..72c2bf8116d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/using-18_a.C
@@ -0,0 +1,29 @@
+// { dg-additional-options "-fmodules-ts -Wno-global-module" }
+// { dg-module-cmi M }
+// Test revealing non-exported declarations still prevents
+// needed GMF declarations from being discarded
+
+module;
+
+struct A {};
+int f();
+
+namespace ns {
+ struct B {};
+ int g();
+}
+
+extern "C" int h();
+namespace ns {
+ extern "C" int h();
+}
+
+export module M;
+
+using ::A;
+using ::f;
+
+using ns::B;
+using ns::g;
+
+using ns::h;
diff --git a/gcc/testsuite/g++.dg/modules/using-18_b.C b/gcc/testsuite/g++.dg/modules/using-18_b.C
new file mode 100644
index 00000000000..f5f608da964
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/using-18_b.C
@@ -0,0 +1,11 @@
+// { dg-additional-options "-fmodules-ts" }
+
+module M;
+
+void test() {
+ A a;
+ B b;
+ int x = f();
+ int y = g();
+ int z = h();
+}
--
2.43.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] c++/modules: Fix revealing with using-decls [PR114867]
2024-05-31 15:57 ` [PATCH v2] c++/modules: Fix revealing with using-decls [PR114867] Nathaniel Shead
@ 2024-05-31 20:23 ` Jason Merrill
0 siblings, 0 replies; 4+ messages in thread
From: Jason Merrill @ 2024-05-31 20:23 UTC (permalink / raw)
To: Nathaniel Shead; +Cc: gcc-patches, Nathan Sidwell
On 5/31/24 11:57, Nathaniel Shead wrote:
> On Tue, May 28, 2024 at 02:57:09PM -0400, Jason Merrill wrote:
>> What if we're revealing without exporting? That is, a using-declaration in
>> module purview that isn't exported? Such a declaration should still prevent
>> discarding, which is my understanding of the use of "revealing" here.
>>
>> It seems like the current code already gets that wrong for e.g.
>>
>> M_1.C:
>> module;
>> struct A {};
>> inline int f() { return 42; }
>> export module M;
>> using ::A;
>> using ::f;
>>
>> M_2.C:
>> import M;
>> inline int f();
>> struct A a; // { dg-bogus "incomplete" }
>> int main() {
>> return f(); // { dg-bogus "undefined" }
>> }
>>
>> It looks like part of the problem is that add_binding_entity is only
>> interested in exported usings, but I think it should also handle revealing
>> ones.
>
> Right; I hadn't thought about that. The cleanest way to solve this I
> think is to add a new flag to OVERLOAD to indicate their purviewness,
> which we can then use in 'add_binding_entity' instead of the current
> reliance on exported usings; this is what I've done in the below patch.
> (There aren't any more TREE_LANG_FLAG_?s left so I just picked another
> unused flag lying around; alternatively I could create _7, there does
> seem to be spare bits in tree_base.)
>
> Another option would be to do like what I've done in my workaround for
> non-functions and just mark the original decl as being in PURVIEW_P; I'm
> not a huge fan of this though.
Agreed.
> + /* FIXME: Handle exporting declarations from a different scope
> + without also marking those declarations as exported.
> + This will require not just binding directly to the underlying
> + value; see c++/114863 and c++/114865. We allow this for purview
> + declarations for now as this doesn't (currently) cause ICEs
> + later down the line, but should also be revisited then. */
I'm not sure what "then" you're referring to?
OK either clarifying, or striking "also" and "then".
Jason
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-05-31 20:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-26 13:01 [PATCH] c++/modules: Prevent revealing a using-decl affecting cached overloads [PR114867] Nathaniel Shead
2024-05-28 18:57 ` Jason Merrill
2024-05-31 15:57 ` [PATCH v2] c++/modules: Fix revealing with using-decls [PR114867] Nathaniel Shead
2024-05-31 20:23 ` Jason Merrill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).