| Message ID | 665332b6.170a0220.0d48.3d88@mx.google.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++/modules: Prevent revealing a using-decl affecting cached overloads [PR114867] | expand |
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
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); +}
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