On 02/28/2014 03:56 PM, Jan Hubicka wrote:
I think we can safely test here DECL_ARTIFICIAL && (DECL_EXTERNAL ||
DECL_COMDAT). If the dtor is going to be output anyway, we are safe to use it.
We already skipped DECL_EXTERNAL decls, and artificial members are
always DECL_COMDAT, but I'll add the COMDAT check.
Are those programs valid by C++ standard? (I believe it is not valid to include
stuff whose implementation you do not link with.).
Symbol visibility is outside the scope of the standard.
If we just want to avoid
breaking python and libreoffice (I fixed libreoffice part however), we may just
go with the ipa-devirt change as you propose (with external&comdat check).
If this is an correctness issue, I think we want to be safe that other
optimizations
won't do the same. In that case your check seems misplaced.
If DECL_ARTIFICIAL destructors are not safe to inline, I would add it into
function_attribute_inlinable_p. If the dtor is not safe to refer, then I would
add it into can_refer_decl_in_current_unit_p
Both such changes would however inhibit quite some optimization, since
artificial destructors are quite common case, right? Or is there some reason why
only speculative devirtualization count possibly work out reference to these?
Normally, it's fine to inline destructors, and refer to them. The
problem comes when we turn what had been a virtual call (which goes
through the vtable that is hidden in the DSO) into a direct call to a
hidden function. We don't do that for user-defined virtual functions
because the user controls whether or not they are defined in the header,
and we don't devirtualize if no definition is available, but
implicitly-declared functions are different because the user has no way
to prevent the definition from being available.
This also isn't a problem for cprop devirtualization, because in that
situation we must have already referred to the vtable.
Jason
commit 2a05a09c268ce3abb373aa86cf731d20aac8dd7a
Author: Jason Merrill <ja...@redhat.com>
Date: Fri Feb 28 14:03:19 2014 -0500
PR c++/58678
* ipa-devirt.c (ipa_devirt): Don't choose an implicitly-declared
function.
diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
index 21649cb..2f84f17 100644
--- a/gcc/ipa-devirt.c
+++ b/gcc/ipa-devirt.c
@@ -1710,7 +1710,7 @@ ipa_devirt (void)
int npolymorphic = 0, nspeculated = 0, nconverted = 0, ncold = 0;
int nmultiple = 0, noverwritable = 0, ndevirtualized = 0, nnotdefined = 0;
- int nwrong = 0, nok = 0, nexternal = 0;;
+ int nwrong = 0, nok = 0, nexternal = 0, nartificial = 0;
FOR_EACH_DEFINED_FUNCTION (n)
{
@@ -1820,6 +1820,17 @@ ipa_devirt (void)
nexternal++;
continue;
}
+ /* Don't use an implicitly-declared destructor (c++/58678). */
+ struct cgraph_node *non_thunk_target
+ = cgraph_function_node (likely_target);
+ if (DECL_ARTIFICIAL (non_thunk_target->decl)
+ && DECL_COMDAT (non_thunk_target->decl))
+ {
+ if (dump_file)
+ fprintf (dump_file, "Target is artificial\n\n");
+ nartificial++;
+ continue;
+ }
if (cgraph_function_body_availability (likely_target)
<= AVAIL_OVERWRITABLE
&& symtab_can_be_discarded (likely_target))
@@ -1862,10 +1873,10 @@ ipa_devirt (void)
" %i speculatively devirtualized, %i cold\n"
"%i have multiple targets, %i overwritable,"
" %i already speculated (%i agree, %i disagree),"
- " %i external, %i not defined\n",
+ " %i external, %i not defined, %i artificial\n",
npolymorphic, ndevirtualized, nconverted, ncold,
nmultiple, noverwritable, nspeculated, nok, nwrong,
- nexternal, nnotdefined);
+ nexternal, nnotdefined, nartificial);
return ndevirtualized ? TODO_remove_functions : 0;
}
diff --git a/gcc/testsuite/g++.dg/ipa/devirt-28.C b/gcc/testsuite/g++.dg/ipa/devirt-28.C
new file mode 100644
index 0000000..e18b818
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/devirt-28.C
@@ -0,0 +1,17 @@
+// PR c++/58678
+// { dg-options "-O3 -fdump-ipa-devirt" }
+
+struct A {
+ virtual ~A();
+};
+struct B : A {
+ virtual int m_fn1();
+};
+void fn1(B* b) {
+ delete b;
+}
+
+// { dg-final { scan-assembler-not "_ZN1AD2Ev" } }
+// { dg-final { scan-assembler-not "_ZN1BD0Ev" } }
+// { dg-final { scan-ipa-dump "Target is artificial" "devirt" } }
+// { dg-final { cleanup-ipa-dump "devirt" } }
