Re: [PATCH] c++/modules: fix virtual destructors [PR103499]
On 11/9/23 18:29, Nathaniel Shead wrote:
On Thu, Nov 09, 2023 at 05:57:39PM -0500, Nathan Sidwell wrote:
On 11/9/23 04:55, Nathaniel Shead wrote:
I'm not sure if this is just papering over a general issue of clones not being
exported/imported, or if this is just an exception to the general case of
clones being able to be freely regenerated with no other issues.
Alternatively, would it be better to override the DECL_VINDEX of the original
declaration after filling it in for the clones as well? I wasn't able to see
anything depending on the current behaviour (though I didn't look very hard).
I think your patch is a fine approach. IIRC just streaming out the clones
directly ran into a bunch of issues, hence the current implementation.
ok
nathan
Sorry, I don't have write access, would you be able to push? Thanks.
(And for my other patch.)
ok, no worries
Bootstrapped and regtexted on x86_64-pc-linux-gnu.
-- >8 --
Currently, cloned functions are not included in the CMI. However, for
virtual destructors the clones must have a different DECL_VINDEX from
their base declaration: the former have an INTEGER_CST indicating the
index into the vtable, while the latter indicate the FUNCTION_DECL that
they're overriding.
As such, this patch ensures that DECL_VINDEX is properly passed on for
cloned functions as well to prevent this from causing issues.
PR c++/103499
gcc/cp/ChangeLog:
* module.cc (trees_out::decl_node): Write DECL_VINDEX for
virtual clones.
(trees_in::tree_node): Read DECL_VINDEX for virtual clones.
gcc/testsuite/ChangeLog:
* g++.dg/modules/pr103499_a.C: New test.
* g++.dg/modules/pr103499_b.C: New test.
Signed-off-by: Nathaniel Shead
---
gcc/cp/module.cc | 6 ++
gcc/testsuite/g++.dg/modules/pr103499_a.C | 12
gcc/testsuite/g++.dg/modules/pr103499_b.C | 8
3 files changed, 26 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/modules/pr103499_a.C
create mode 100644 gcc/testsuite/g++.dg/modules/pr103499_b.C
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index c1c8c226bc1..416a7c414cc 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -8648,6 +8648,8 @@ trees_out::decl_node (tree decl, walk_kind ref)
tree_node (target);
tree_node (DECL_NAME (decl));
+ if (TREE_CODE (decl) == FUNCTION_DECL && DECL_VIRTUAL_P (decl))
+ tree_node (DECL_VINDEX (decl));
int tag = insert (decl);
if (streaming_p ())
dump (dumper::TREE)
@@ -9869,6 +9871,10 @@ trees_in::tree_node (bool is_use)
}
}
+ /* A clone might have a different vtable entry. */
+ if (res && TREE_CODE (res) == FUNCTION_DECL && DECL_VIRTUAL_P (res))
+ DECL_VINDEX (res) = tree_node ();
+
if (!res)
set_overrun ();
int tag = insert (res);
diff --git a/gcc/testsuite/g++.dg/modules/pr103499_a.C
b/gcc/testsuite/g++.dg/modules/pr103499_a.C
new file mode 100644
index 000..0497c2c5504
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr103499_a.C
@@ -0,0 +1,12 @@
+// PR c++/103499
+// { dg-module-do compile }
+// { dg-additional-options "-fmodules-ts" }
+// { dg-module-cmi pr103499 }
+
+export module pr103499;
+
+export struct base {
+ virtual ~base() = default;
+};
+
+export struct derived : base {};
diff --git a/gcc/testsuite/g++.dg/modules/pr103499_b.C
b/gcc/testsuite/g++.dg/modules/pr103499_b.C
new file mode 100644
index 000..b7468562ba9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr103499_b.C
@@ -0,0 +1,8 @@
+// PR c++/103499
+// { dg-additional-options "-fmodules-ts" }
+
+import pr103499;
+
+void test(derived* p) {
+ delete p;
+}
--
Nathan Sidwell
--
Nathan Sidwell
Re: [PATCH] c++/modules: fix virtual destructors [PR103499]
On Thu, Nov 09, 2023 at 05:57:39PM -0500, Nathan Sidwell wrote:
> On 11/9/23 04:55, Nathaniel Shead wrote:
> > I'm not sure if this is just papering over a general issue of clones not
> > being
> > exported/imported, or if this is just an exception to the general case of
> > clones being able to be freely regenerated with no other issues.
> >
> > Alternatively, would it be better to override the DECL_VINDEX of the
> > original
> > declaration after filling it in for the clones as well? I wasn't able to see
> > anything depending on the current behaviour (though I didn't look very
> > hard).
>
> I think your patch is a fine approach. IIRC just streaming out the clones
> directly ran into a bunch of issues, hence the current implementation.
>
>
> ok
>
> nathan
Sorry, I don't have write access, would you be able to push? Thanks.
(And for my other patch.)
> >
> > Bootstrapped and regtexted on x86_64-pc-linux-gnu.
> >
> > -- >8 --
> >
> > Currently, cloned functions are not included in the CMI. However, for
> > virtual destructors the clones must have a different DECL_VINDEX from
> > their base declaration: the former have an INTEGER_CST indicating the
> > index into the vtable, while the latter indicate the FUNCTION_DECL that
> > they're overriding.
> >
> > As such, this patch ensures that DECL_VINDEX is properly passed on for
> > cloned functions as well to prevent this from causing issues.
> >
> > PR c++/103499
> >
> > gcc/cp/ChangeLog:
> >
> > * module.cc (trees_out::decl_node): Write DECL_VINDEX for
> > virtual clones.
> > (trees_in::tree_node): Read DECL_VINDEX for virtual clones.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/modules/pr103499_a.C: New test.
> > * g++.dg/modules/pr103499_b.C: New test.
> >
> > Signed-off-by: Nathaniel Shead
> > ---
> > gcc/cp/module.cc | 6 ++
> > gcc/testsuite/g++.dg/modules/pr103499_a.C | 12
> > gcc/testsuite/g++.dg/modules/pr103499_b.C | 8
> > 3 files changed, 26 insertions(+)
> > create mode 100644 gcc/testsuite/g++.dg/modules/pr103499_a.C
> > create mode 100644 gcc/testsuite/g++.dg/modules/pr103499_b.C
> >
> > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > index c1c8c226bc1..416a7c414cc 100644
> > --- a/gcc/cp/module.cc
> > +++ b/gcc/cp/module.cc
> > @@ -8648,6 +8648,8 @@ trees_out::decl_node (tree decl, walk_kind ref)
> > tree_node (target);
> > tree_node (DECL_NAME (decl));
> > + if (TREE_CODE (decl) == FUNCTION_DECL && DECL_VIRTUAL_P (decl))
> > + tree_node (DECL_VINDEX (decl));
> > int tag = insert (decl);
> > if (streaming_p ())
> > dump (dumper::TREE)
> > @@ -9869,6 +9871,10 @@ trees_in::tree_node (bool is_use)
> > }
> > }
> > + /* A clone might have a different vtable entry. */
> > + if (res && TREE_CODE (res) == FUNCTION_DECL && DECL_VIRTUAL_P (res))
> > + DECL_VINDEX (res) = tree_node ();
> > +
> > if (!res)
> > set_overrun ();
> > int tag = insert (res);
> > diff --git a/gcc/testsuite/g++.dg/modules/pr103499_a.C
> > b/gcc/testsuite/g++.dg/modules/pr103499_a.C
> > new file mode 100644
> > index 000..0497c2c5504
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/pr103499_a.C
> > @@ -0,0 +1,12 @@
> > +// PR c++/103499
> > +// { dg-module-do compile }
> > +// { dg-additional-options "-fmodules-ts" }
> > +// { dg-module-cmi pr103499 }
> > +
> > +export module pr103499;
> > +
> > +export struct base {
> > + virtual ~base() = default;
> > +};
> > +
> > +export struct derived : base {};
> > diff --git a/gcc/testsuite/g++.dg/modules/pr103499_b.C
> > b/gcc/testsuite/g++.dg/modules/pr103499_b.C
> > new file mode 100644
> > index 000..b7468562ba9
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/pr103499_b.C
> > @@ -0,0 +1,8 @@
> > +// PR c++/103499
> > +// { dg-additional-options "-fmodules-ts" }
> > +
> > +import pr103499;
> > +
> > +void test(derived* p) {
> > + delete p;
> > +}
>
> --
> Nathan Sidwell
>
Re: [PATCH] c++/modules: fix virtual destructors [PR103499]
On 11/9/23 04:55, Nathaniel Shead wrote:
I'm not sure if this is just papering over a general issue of clones not being
exported/imported, or if this is just an exception to the general case of
clones being able to be freely regenerated with no other issues.
Alternatively, would it be better to override the DECL_VINDEX of the original
declaration after filling it in for the clones as well? I wasn't able to see
anything depending on the current behaviour (though I didn't look very hard).
I think your patch is a fine approach. IIRC just streaming out the clones
directly ran into a bunch of issues, hence the current implementation.
ok
nathan
Bootstrapped and regtexted on x86_64-pc-linux-gnu.
-- >8 --
Currently, cloned functions are not included in the CMI. However, for
virtual destructors the clones must have a different DECL_VINDEX from
their base declaration: the former have an INTEGER_CST indicating the
index into the vtable, while the latter indicate the FUNCTION_DECL that
they're overriding.
As such, this patch ensures that DECL_VINDEX is properly passed on for
cloned functions as well to prevent this from causing issues.
PR c++/103499
gcc/cp/ChangeLog:
* module.cc (trees_out::decl_node): Write DECL_VINDEX for
virtual clones.
(trees_in::tree_node): Read DECL_VINDEX for virtual clones.
gcc/testsuite/ChangeLog:
* g++.dg/modules/pr103499_a.C: New test.
* g++.dg/modules/pr103499_b.C: New test.
Signed-off-by: Nathaniel Shead
---
gcc/cp/module.cc | 6 ++
gcc/testsuite/g++.dg/modules/pr103499_a.C | 12
gcc/testsuite/g++.dg/modules/pr103499_b.C | 8
3 files changed, 26 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/modules/pr103499_a.C
create mode 100644 gcc/testsuite/g++.dg/modules/pr103499_b.C
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index c1c8c226bc1..416a7c414cc 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -8648,6 +8648,8 @@ trees_out::decl_node (tree decl, walk_kind ref)
tree_node (target);
tree_node (DECL_NAME (decl));
+ if (TREE_CODE (decl) == FUNCTION_DECL && DECL_VIRTUAL_P (decl))
+ tree_node (DECL_VINDEX (decl));
int tag = insert (decl);
if (streaming_p ())
dump (dumper::TREE)
@@ -9869,6 +9871,10 @@ trees_in::tree_node (bool is_use)
}
}
+ /* A clone might have a different vtable entry. */
+ if (res && TREE_CODE (res) == FUNCTION_DECL && DECL_VIRTUAL_P (res))
+ DECL_VINDEX (res) = tree_node ();
+
if (!res)
set_overrun ();
int tag = insert (res);
diff --git a/gcc/testsuite/g++.dg/modules/pr103499_a.C
b/gcc/testsuite/g++.dg/modules/pr103499_a.C
new file mode 100644
index 000..0497c2c5504
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr103499_a.C
@@ -0,0 +1,12 @@
+// PR c++/103499
+// { dg-module-do compile }
+// { dg-additional-options "-fmodules-ts" }
+// { dg-module-cmi pr103499 }
+
+export module pr103499;
+
+export struct base {
+ virtual ~base() = default;
+};
+
+export struct derived : base {};
diff --git a/gcc/testsuite/g++.dg/modules/pr103499_b.C
b/gcc/testsuite/g++.dg/modules/pr103499_b.C
new file mode 100644
index 000..b7468562ba9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr103499_b.C
@@ -0,0 +1,8 @@
+// PR c++/103499
+// { dg-additional-options "-fmodules-ts" }
+
+import pr103499;
+
+void test(derived* p) {
+ delete p;
+}
--
Nathan Sidwell
[PATCH] c++/modules: fix virtual destructors [PR103499]
I'm not sure if this is just papering over a general issue of clones not being
exported/imported, or if this is just an exception to the general case of
clones being able to be freely regenerated with no other issues.
Alternatively, would it be better to override the DECL_VINDEX of the original
declaration after filling it in for the clones as well? I wasn't able to see
anything depending on the current behaviour (though I didn't look very hard).
Bootstrapped and regtexted on x86_64-pc-linux-gnu.
-- >8 --
Currently, cloned functions are not included in the CMI. However, for
virtual destructors the clones must have a different DECL_VINDEX from
their base declaration: the former have an INTEGER_CST indicating the
index into the vtable, while the latter indicate the FUNCTION_DECL that
they're overriding.
As such, this patch ensures that DECL_VINDEX is properly passed on for
cloned functions as well to prevent this from causing issues.
PR c++/103499
gcc/cp/ChangeLog:
* module.cc (trees_out::decl_node): Write DECL_VINDEX for
virtual clones.
(trees_in::tree_node): Read DECL_VINDEX for virtual clones.
gcc/testsuite/ChangeLog:
* g++.dg/modules/pr103499_a.C: New test.
* g++.dg/modules/pr103499_b.C: New test.
Signed-off-by: Nathaniel Shead
---
gcc/cp/module.cc | 6 ++
gcc/testsuite/g++.dg/modules/pr103499_a.C | 12
gcc/testsuite/g++.dg/modules/pr103499_b.C | 8
3 files changed, 26 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/modules/pr103499_a.C
create mode 100644 gcc/testsuite/g++.dg/modules/pr103499_b.C
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index c1c8c226bc1..416a7c414cc 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -8648,6 +8648,8 @@ trees_out::decl_node (tree decl, walk_kind ref)
tree_node (target);
tree_node (DECL_NAME (decl));
+ if (TREE_CODE (decl) == FUNCTION_DECL && DECL_VIRTUAL_P (decl))
+ tree_node (DECL_VINDEX (decl));
int tag = insert (decl);
if (streaming_p ())
dump (dumper::TREE)
@@ -9869,6 +9871,10 @@ trees_in::tree_node (bool is_use)
}
}
+ /* A clone might have a different vtable entry. */
+ if (res && TREE_CODE (res) == FUNCTION_DECL && DECL_VIRTUAL_P (res))
+ DECL_VINDEX (res) = tree_node ();
+
if (!res)
set_overrun ();
int tag = insert (res);
diff --git a/gcc/testsuite/g++.dg/modules/pr103499_a.C
b/gcc/testsuite/g++.dg/modules/pr103499_a.C
new file mode 100644
index 000..0497c2c5504
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr103499_a.C
@@ -0,0 +1,12 @@
+// PR c++/103499
+// { dg-module-do compile }
+// { dg-additional-options "-fmodules-ts" }
+// { dg-module-cmi pr103499 }
+
+export module pr103499;
+
+export struct base {
+ virtual ~base() = default;
+};
+
+export struct derived : base {};
diff --git a/gcc/testsuite/g++.dg/modules/pr103499_b.C
b/gcc/testsuite/g++.dg/modules/pr103499_b.C
new file mode 100644
index 000..b7468562ba9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr103499_b.C
@@ -0,0 +1,8 @@
+// PR c++/103499
+// { dg-additional-options "-fmodules-ts" }
+
+import pr103499;
+
+void test(derived* p) {
+ delete p;
+}
--
2.42.0
