c++/modules: Handle chaining already-imported local types [PR114630]
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Build passed
|
Commit Message
Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/14?
-- >8 --
In the linked testcase, an ICE occurs because when reading the
(duplicate) function definition for _M_do_parse from module Y, the local
type definitions have already been streamed from module X and setup as
regular backreferences, rather than being found with find_duplicate,
causing issues with managing DECL_CHAIN.
It is tempting to just skip setting up the DECL_CHAIN for this case.
However, for the future it would be best to ensure that the block vars
for the duplicate definition are accurate, so that we could implement
ODR checking on function definitions at some point.
So to solve this, this patch creates a copy of the streamed-in local
type and chains that; it will be discarded along with the rest of the
duplicate function after we've finished processing.
A couple of suggested implementations from the discussion on the PR that
don't work:
- Replacing the `DECL_CHAIN` assertion with `(*chain && *chain != decl)`
doesn't handle the case where type definitions are followed by regular
local variables, since those won't have been imported as separate
backreferences and so the chains will diverge.
- Correcting the purviewness of GMF template instantiations to force Y
to emit copies of the local types rather than backreferences into X is
insufficient, as it's still possible that the local types got streamed
in a separate cluster to the function definition, and so will be again
referred to via regular backreferences when importing.
- Likewise, preventing the emission of function definitions where an
import has already provided that same definition also is insufficient,
for much the same reason.
PR c++/114630
gcc/cp/ChangeLog:
* module.cc (trees_in::core_vals) <BLOCK>: Chain a new node if
DECL_CHAIN already is set.
gcc/testsuite/ChangeLog:
* g++.dg/modules/pr114630.h: New test.
* g++.dg/modules/pr114630_a.C: New test.
* g++.dg/modules/pr114630_b.C: New test.
* g++.dg/modules/pr114630_c.C: New test.
Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
gcc/cp/module.cc | 14 +++++++++++++-
gcc/testsuite/g++.dg/modules/pr114630.h | 11 +++++++++++
gcc/testsuite/g++.dg/modules/pr114630_a.C | 7 +++++++
gcc/testsuite/g++.dg/modules/pr114630_b.C | 8 ++++++++
gcc/testsuite/g++.dg/modules/pr114630_c.C | 4 ++++
5 files changed, 43 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/modules/pr114630.h
create mode 100644 gcc/testsuite/g++.dg/modules/pr114630_a.C
create mode 100644 gcc/testsuite/g++.dg/modules/pr114630_b.C
create mode 100644 gcc/testsuite/g++.dg/modules/pr114630_c.C
Comments
On 1/9/25 9:31 AM, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/14?
OK.
> -- >8 --
>
> In the linked testcase, an ICE occurs because when reading the
> (duplicate) function definition for _M_do_parse from module Y, the local
> type definitions have already been streamed from module X and setup as
> regular backreferences, rather than being found with find_duplicate,
> causing issues with managing DECL_CHAIN.
>
> It is tempting to just skip setting up the DECL_CHAIN for this case.
> However, for the future it would be best to ensure that the block vars
> for the duplicate definition are accurate, so that we could implement
> ODR checking on function definitions at some point.
>
> So to solve this, this patch creates a copy of the streamed-in local
> type and chains that; it will be discarded along with the rest of the
> duplicate function after we've finished processing.
>
> A couple of suggested implementations from the discussion on the PR that
> don't work:
>
> - Replacing the `DECL_CHAIN` assertion with `(*chain && *chain != decl)`
> doesn't handle the case where type definitions are followed by regular
> local variables, since those won't have been imported as separate
> backreferences and so the chains will diverge.
>
> - Correcting the purviewness of GMF template instantiations to force Y
> to emit copies of the local types rather than backreferences into X is
> insufficient, as it's still possible that the local types got streamed
> in a separate cluster to the function definition, and so will be again
> referred to via regular backreferences when importing.
>
> - Likewise, preventing the emission of function definitions where an
> import has already provided that same definition also is insufficient,
> for much the same reason.
>
> PR c++/114630
>
> gcc/cp/ChangeLog:
>
> * module.cc (trees_in::core_vals) <BLOCK>: Chain a new node if
> DECL_CHAIN already is set.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/modules/pr114630.h: New test.
> * g++.dg/modules/pr114630_a.C: New test.
> * g++.dg/modules/pr114630_b.C: New test.
> * g++.dg/modules/pr114630_c.C: New test.
>
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
> gcc/cp/module.cc | 14 +++++++++++++-
> gcc/testsuite/g++.dg/modules/pr114630.h | 11 +++++++++++
> gcc/testsuite/g++.dg/modules/pr114630_a.C | 7 +++++++
> gcc/testsuite/g++.dg/modules/pr114630_b.C | 8 ++++++++
> gcc/testsuite/g++.dg/modules/pr114630_c.C | 4 ++++
> 5 files changed, 43 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/g++.dg/modules/pr114630.h
> create mode 100644 gcc/testsuite/g++.dg/modules/pr114630_a.C
> create mode 100644 gcc/testsuite/g++.dg/modules/pr114630_b.C
> create mode 100644 gcc/testsuite/g++.dg/modules/pr114630_c.C
>
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 5350e6c4bad..ff2683de73e 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -6928,11 +6928,23 @@ trees_in::core_vals (tree t)
> body) anyway. */
> decl = maybe_duplicate (decl);
>
> - if (!DECL_P (decl) || DECL_CHAIN (decl))
> + if (!DECL_P (decl))
> {
> set_overrun ();
> break;
> }
> +
> + /* If DECL_CHAIN is already set then this was a backreference to a
> + local type or enumerator from a previous read (PR c++/114630).
> + Let's copy the node so we can keep building the chain for ODR
> + checking later. */
> + if (DECL_CHAIN (decl))
> + {
> + gcc_checking_assert (TREE_CODE (decl) == TYPE_DECL
> + && find_duplicate (DECL_CONTEXT (decl)));
> + decl = copy_node (decl);
> + }
> +
> *chain = decl;
> chain = &DECL_CHAIN (decl);
> }
> diff --git a/gcc/testsuite/g++.dg/modules/pr114630.h b/gcc/testsuite/g++.dg/modules/pr114630.h
> new file mode 100644
> index 00000000000..8730007f59f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr114630.h
> @@ -0,0 +1,11 @@
> +template <typename _CharT>
> +void _M_do_parse() {
> + struct A {};
> + struct B {};
> + int x;
> +}
> +
> +template <typename> struct formatter;
> +template <> struct formatter<int> {
> + void parse() { _M_do_parse<int>(); }
> +};
> diff --git a/gcc/testsuite/g++.dg/modules/pr114630_a.C b/gcc/testsuite/g++.dg/modules/pr114630_a.C
> new file mode 100644
> index 00000000000..ecfd7ca0b28
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr114630_a.C
> @@ -0,0 +1,7 @@
> +// { dg-additional-options "-fmodules" }
> +// { dg-module-cmi X }
> +
> +module;
> +#include "pr114630.h"
> +export module X;
> +formatter<int> a;
> diff --git a/gcc/testsuite/g++.dg/modules/pr114630_b.C b/gcc/testsuite/g++.dg/modules/pr114630_b.C
> new file mode 100644
> index 00000000000..52fe04e2ce0
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr114630_b.C
> @@ -0,0 +1,8 @@
> +// { dg-additional-options "-fmodules" }
> +// { dg-module-cmi Y }
> +
> +module;
> +#include "pr114630.h"
> +export module Y;
> +import X;
> +formatter<int> b;
> diff --git a/gcc/testsuite/g++.dg/modules/pr114630_c.C b/gcc/testsuite/g++.dg/modules/pr114630_c.C
> new file mode 100644
> index 00000000000..54a21f08057
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr114630_c.C
> @@ -0,0 +1,4 @@
> +// { dg-additional-options "-fmodules" }
> +
> +#include "pr114630.h"
> +import Y;
On Fri, 10 Jan 2025, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/14?
Nice approach, thanks for fixing this!
>
> -- >8 --
>
> In the linked testcase, an ICE occurs because when reading the
> (duplicate) function definition for _M_do_parse from module Y, the local
> type definitions have already been streamed from module X and setup as
> regular backreferences, rather than being found with find_duplicate,
> causing issues with managing DECL_CHAIN.
>
> It is tempting to just skip setting up the DECL_CHAIN for this case.
> However, for the future it would be best to ensure that the block vars
> for the duplicate definition are accurate, so that we could implement
> ODR checking on function definitions at some point.
>
> So to solve this, this patch creates a copy of the streamed-in local
> type and chains that; it will be discarded along with the rest of the
> duplicate function after we've finished processing.
>
> A couple of suggested implementations from the discussion on the PR that
> don't work:
>
> - Replacing the `DECL_CHAIN` assertion with `(*chain && *chain != decl)`
> doesn't handle the case where type definitions are followed by regular
> local variables, since those won't have been imported as separate
> backreferences and so the chains will diverge.
>
> - Correcting the purviewness of GMF template instantiations to force Y
> to emit copies of the local types rather than backreferences into X is
> insufficient, as it's still possible that the local types got streamed
> in a separate cluster to the function definition, and so will be again
> referred to via regular backreferences when importing.
>
> - Likewise, preventing the emission of function definitions where an
> import has already provided that same definition also is insufficient,
> for much the same reason.
>
> PR c++/114630
>
> gcc/cp/ChangeLog:
>
> * module.cc (trees_in::core_vals) <BLOCK>: Chain a new node if
> DECL_CHAIN already is set.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/modules/pr114630.h: New test.
> * g++.dg/modules/pr114630_a.C: New test.
> * g++.dg/modules/pr114630_b.C: New test.
> * g++.dg/modules/pr114630_c.C: New test.
>
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
> gcc/cp/module.cc | 14 +++++++++++++-
> gcc/testsuite/g++.dg/modules/pr114630.h | 11 +++++++++++
> gcc/testsuite/g++.dg/modules/pr114630_a.C | 7 +++++++
> gcc/testsuite/g++.dg/modules/pr114630_b.C | 8 ++++++++
> gcc/testsuite/g++.dg/modules/pr114630_c.C | 4 ++++
> 5 files changed, 43 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/g++.dg/modules/pr114630.h
> create mode 100644 gcc/testsuite/g++.dg/modules/pr114630_a.C
> create mode 100644 gcc/testsuite/g++.dg/modules/pr114630_b.C
> create mode 100644 gcc/testsuite/g++.dg/modules/pr114630_c.C
>
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 5350e6c4bad..ff2683de73e 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -6928,11 +6928,23 @@ trees_in::core_vals (tree t)
> body) anyway. */
> decl = maybe_duplicate (decl);
>
> - if (!DECL_P (decl) || DECL_CHAIN (decl))
> + if (!DECL_P (decl))
> {
> set_overrun ();
> break;
> }
> +
> + /* If DECL_CHAIN is already set then this was a backreference to a
> + local type or enumerator from a previous read (PR c++/114630).
> + Let's copy the node so we can keep building the chain for ODR
> + checking later. */
> + if (DECL_CHAIN (decl))
> + {
> + gcc_checking_assert (TREE_CODE (decl) == TYPE_DECL
> + && find_duplicate (DECL_CONTEXT (decl)));
> + decl = copy_node (decl);
Shall we use copy_decl here instead so that any DECL_LANG_SPECIFIC node
is copied as well? IIUC we usually don't share DECL_LANG_SPECIFIC
between decls.
> + }
> +
> *chain = decl;
> chain = &DECL_CHAIN (decl);
> }
> diff --git a/gcc/testsuite/g++.dg/modules/pr114630.h b/gcc/testsuite/g++.dg/modules/pr114630.h
> new file mode 100644
> index 00000000000..8730007f59f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr114630.h
> @@ -0,0 +1,11 @@
> +template <typename _CharT>
> +void _M_do_parse() {
> + struct A {};
> + struct B {};
> + int x;
> +}
> +
> +template <typename> struct formatter;
> +template <> struct formatter<int> {
> + void parse() { _M_do_parse<int>(); }
> +};
> diff --git a/gcc/testsuite/g++.dg/modules/pr114630_a.C b/gcc/testsuite/g++.dg/modules/pr114630_a.C
> new file mode 100644
> index 00000000000..ecfd7ca0b28
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr114630_a.C
> @@ -0,0 +1,7 @@
> +// { dg-additional-options "-fmodules" }
> +// { dg-module-cmi X }
> +
> +module;
> +#include "pr114630.h"
> +export module X;
> +formatter<int> a;
> diff --git a/gcc/testsuite/g++.dg/modules/pr114630_b.C b/gcc/testsuite/g++.dg/modules/pr114630_b.C
> new file mode 100644
> index 00000000000..52fe04e2ce0
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr114630_b.C
> @@ -0,0 +1,8 @@
> +// { dg-additional-options "-fmodules" }
> +// { dg-module-cmi Y }
> +
> +module;
> +#include "pr114630.h"
> +export module Y;
> +import X;
> +formatter<int> b;
> diff --git a/gcc/testsuite/g++.dg/modules/pr114630_c.C b/gcc/testsuite/g++.dg/modules/pr114630_c.C
> new file mode 100644
> index 00000000000..54a21f08057
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr114630_c.C
> @@ -0,0 +1,4 @@
> +// { dg-additional-options "-fmodules" }
> +
> +#include "pr114630.h"
> +import Y;
> --
> 2.47.0
>
>
On Thu, Jan 09, 2025 at 05:41:07PM -0500, Patrick Palka wrote:
> On Fri, 10 Jan 2025, Nathaniel Shead wrote:
>
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/14?
>
> Nice approach, thanks for fixing this!
>
> >
> > -- >8 --
> >
> > In the linked testcase, an ICE occurs because when reading the
> > (duplicate) function definition for _M_do_parse from module Y, the local
> > type definitions have already been streamed from module X and setup as
> > regular backreferences, rather than being found with find_duplicate,
> > causing issues with managing DECL_CHAIN.
> >
> > It is tempting to just skip setting up the DECL_CHAIN for this case.
> > However, for the future it would be best to ensure that the block vars
> > for the duplicate definition are accurate, so that we could implement
> > ODR checking on function definitions at some point.
> >
> > So to solve this, this patch creates a copy of the streamed-in local
> > type and chains that; it will be discarded along with the rest of the
> > duplicate function after we've finished processing.
> >
> > A couple of suggested implementations from the discussion on the PR that
> > don't work:
> >
> > - Replacing the `DECL_CHAIN` assertion with `(*chain && *chain != decl)`
> > doesn't handle the case where type definitions are followed by regular
> > local variables, since those won't have been imported as separate
> > backreferences and so the chains will diverge.
> >
> > - Correcting the purviewness of GMF template instantiations to force Y
> > to emit copies of the local types rather than backreferences into X is
> > insufficient, as it's still possible that the local types got streamed
> > in a separate cluster to the function definition, and so will be again
> > referred to via regular backreferences when importing.
> >
> > - Likewise, preventing the emission of function definitions where an
> > import has already provided that same definition also is insufficient,
> > for much the same reason.
> >
> > PR c++/114630
> >
> > gcc/cp/ChangeLog:
> >
> > * module.cc (trees_in::core_vals) <BLOCK>: Chain a new node if
> > DECL_CHAIN already is set.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/modules/pr114630.h: New test.
> > * g++.dg/modules/pr114630_a.C: New test.
> > * g++.dg/modules/pr114630_b.C: New test.
> > * g++.dg/modules/pr114630_c.C: New test.
> >
> > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > ---
> > gcc/cp/module.cc | 14 +++++++++++++-
> > gcc/testsuite/g++.dg/modules/pr114630.h | 11 +++++++++++
> > gcc/testsuite/g++.dg/modules/pr114630_a.C | 7 +++++++
> > gcc/testsuite/g++.dg/modules/pr114630_b.C | 8 ++++++++
> > gcc/testsuite/g++.dg/modules/pr114630_c.C | 4 ++++
> > 5 files changed, 43 insertions(+), 1 deletion(-)
> > create mode 100644 gcc/testsuite/g++.dg/modules/pr114630.h
> > create mode 100644 gcc/testsuite/g++.dg/modules/pr114630_a.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/pr114630_b.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/pr114630_c.C
> >
> > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > index 5350e6c4bad..ff2683de73e 100644
> > --- a/gcc/cp/module.cc
> > +++ b/gcc/cp/module.cc
> > @@ -6928,11 +6928,23 @@ trees_in::core_vals (tree t)
> > body) anyway. */
> > decl = maybe_duplicate (decl);
> >
> > - if (!DECL_P (decl) || DECL_CHAIN (decl))
> > + if (!DECL_P (decl))
> > {
> > set_overrun ();
> > break;
> > }
> > +
> > + /* If DECL_CHAIN is already set then this was a backreference to a
> > + local type or enumerator from a previous read (PR c++/114630).
> > + Let's copy the node so we can keep building the chain for ODR
> > + checking later. */
> > + if (DECL_CHAIN (decl))
> > + {
> > + gcc_checking_assert (TREE_CODE (decl) == TYPE_DECL
> > + && find_duplicate (DECL_CONTEXT (decl)));
> > + decl = copy_node (decl);
>
> Shall we use copy_decl here instead so that any DECL_LANG_SPECIFIC node
> is copied as well? IIUC we usually don't share DECL_LANG_SPECIFIC
> between decls.
>
Happy to use copy_decl instead; I'll retest tonight and push tomorrow if
there's no issues. Thanks!
> > + }
> > +
> > *chain = decl;
> > chain = &DECL_CHAIN (decl);
> > }
> > diff --git a/gcc/testsuite/g++.dg/modules/pr114630.h b/gcc/testsuite/g++.dg/modules/pr114630.h
> > new file mode 100644
> > index 00000000000..8730007f59f
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/pr114630.h
> > @@ -0,0 +1,11 @@
> > +template <typename _CharT>
> > +void _M_do_parse() {
> > + struct A {};
> > + struct B {};
> > + int x;
> > +}
> > +
> > +template <typename> struct formatter;
> > +template <> struct formatter<int> {
> > + void parse() { _M_do_parse<int>(); }
> > +};
> > diff --git a/gcc/testsuite/g++.dg/modules/pr114630_a.C b/gcc/testsuite/g++.dg/modules/pr114630_a.C
> > new file mode 100644
> > index 00000000000..ecfd7ca0b28
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/pr114630_a.C
> > @@ -0,0 +1,7 @@
> > +// { dg-additional-options "-fmodules" }
> > +// { dg-module-cmi X }
> > +
> > +module;
> > +#include "pr114630.h"
> > +export module X;
> > +formatter<int> a;
> > diff --git a/gcc/testsuite/g++.dg/modules/pr114630_b.C b/gcc/testsuite/g++.dg/modules/pr114630_b.C
> > new file mode 100644
> > index 00000000000..52fe04e2ce0
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/pr114630_b.C
> > @@ -0,0 +1,8 @@
> > +// { dg-additional-options "-fmodules" }
> > +// { dg-module-cmi Y }
> > +
> > +module;
> > +#include "pr114630.h"
> > +export module Y;
> > +import X;
> > +formatter<int> b;
> > diff --git a/gcc/testsuite/g++.dg/modules/pr114630_c.C b/gcc/testsuite/g++.dg/modules/pr114630_c.C
> > new file mode 100644
> > index 00000000000..54a21f08057
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/pr114630_c.C
> > @@ -0,0 +1,4 @@
> > +// { dg-additional-options "-fmodules" }
> > +
> > +#include "pr114630.h"
> > +import Y;
> > --
> > 2.47.0
> >
> >
>
@@ -6928,11 +6928,23 @@ trees_in::core_vals (tree t)
body) anyway. */
decl = maybe_duplicate (decl);
- if (!DECL_P (decl) || DECL_CHAIN (decl))
+ if (!DECL_P (decl))
{
set_overrun ();
break;
}
+
+ /* If DECL_CHAIN is already set then this was a backreference to a
+ local type or enumerator from a previous read (PR c++/114630).
+ Let's copy the node so we can keep building the chain for ODR
+ checking later. */
+ if (DECL_CHAIN (decl))
+ {
+ gcc_checking_assert (TREE_CODE (decl) == TYPE_DECL
+ && find_duplicate (DECL_CONTEXT (decl)));
+ decl = copy_node (decl);
+ }
+
*chain = decl;
chain = &DECL_CHAIN (decl);
}
new file mode 100644
@@ -0,0 +1,11 @@
+template <typename _CharT>
+void _M_do_parse() {
+ struct A {};
+ struct B {};
+ int x;
+}
+
+template <typename> struct formatter;
+template <> struct formatter<int> {
+ void parse() { _M_do_parse<int>(); }
+};
new file mode 100644
@@ -0,0 +1,7 @@
+// { dg-additional-options "-fmodules" }
+// { dg-module-cmi X }
+
+module;
+#include "pr114630.h"
+export module X;
+formatter<int> a;
new file mode 100644
@@ -0,0 +1,8 @@
+// { dg-additional-options "-fmodules" }
+// { dg-module-cmi Y }
+
+module;
+#include "pr114630.h"
+export module Y;
+import X;
+formatter<int> b;
new file mode 100644
@@ -0,0 +1,4 @@
+// { dg-additional-options "-fmodules" }
+
+#include "pr114630.h"
+import Y;