Re: RFA: PATCH to gimple-fold.c for c++/80916, bogus "static but not defined" warning
> On Thu, Feb 28, 2019 at 12:18 PM Jason Merrill wrote:
> > On Thu, Feb 28, 2019 at 11:58 AM Jan Hubicka wrote:
> > > sorry for late reply - I did not identify it as a patch to symbol table.
> > > Indeed we want can_refer_decl_in_current_unit_p is a good place to test
> > > this. Is there a reason to resrict this to functions with no body?
> >
> > If the function has a definition, then of course we can refer to it in
> > its own unit. Am I missing something?
>
> Ah, yes, I was. You mean, why do we care about DECL_INITIAL if
> DECL_EXTERNAL is set? I think I added that check out of caution.
>
> This would be a more straightforward change:
> commit 6af927c40585a4ff75a83b7cdabe8f9074a8d391
> Author: Jason Merrill
> Date: Fri Jan 25 09:09:17 2019 -0500
>
> PR c++/80916 - spurious "static but not defined" warning.
>
> Nothing can refer to an internal decl with no definition, so we shouldn't
> treat such a decl as a possible devirtualization target.
>
> * gimple-fold.c (can_refer_decl_in_current_unit_p): Return false
> for an internal function with no definition.
Yes, this looks fine to me. Thanks a lot!
I hope frontends are not setting this combination of flags in scenarios
this transformation would be valid, but I can't think of a reason they
would be.
Patch is OK.
Honza
>
> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
> index 7ef5004f5f9..62d2e0abc26 100644
> --- a/gcc/gimple-fold.c
> +++ b/gcc/gimple-fold.c
> @@ -121,9 +121,12 @@ can_refer_decl_in_current_unit_p (tree decl, tree
> from_decl)
>|| !VAR_OR_FUNCTION_DECL_P (decl))
> return true;
>
> - /* Static objects can be referred only if they was not optimized out yet.
> */
> - if (!TREE_PUBLIC (decl) && !DECL_EXTERNAL (decl))
> + /* Static objects can be referred only if they are defined and not
> optimized
> + out yet. */
> + if (!TREE_PUBLIC (decl))
> {
> + if (DECL_EXTERNAL (decl))
> + return false;
>/* Before we start optimizing unreachable code we can be sure all
>static objects are defined. */
>if (symtab->function_flags_ready)
> diff --git a/gcc/testsuite/g++.dg/warn/unused-fn1.C
> b/gcc/testsuite/g++.dg/warn/unused-fn1.C
> new file mode 100644
> index 000..aabc01b3f44
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/unused-fn1.C
> @@ -0,0 +1,16 @@
> +// PR c++/80916
> +// { dg-options "-Os -Wunused" }
> +
> +struct j {
> + virtual void dispatch(void *) {}
> +};
> +template
> +struct i : j {
> + void dispatch(void *) {} // warning: 'void i<
> >::dispatch(void*) [with = {anonymous}::l]' declared
> 'static' but never defined [-Wunused-function]
> +};
> +namespace {
> + struct l : i {};
> +}
> +void f(j *k) {
> + k->dispatch(0);
> +}
Re: RFA: PATCH to gimple-fold.c for c++/80916, bogus "static but not defined" warning
On Thu, Feb 28, 2019 at 12:18 PM Jason Merrill wrote:
> On Thu, Feb 28, 2019 at 11:58 AM Jan Hubicka wrote:
> > sorry for late reply - I did not identify it as a patch to symbol table.
> > Indeed we want can_refer_decl_in_current_unit_p is a good place to test
> > this. Is there a reason to resrict this to functions with no body?
>
> If the function has a definition, then of course we can refer to it in
> its own unit. Am I missing something?
Ah, yes, I was. You mean, why do we care about DECL_INITIAL if
DECL_EXTERNAL is set? I think I added that check out of caution.
This would be a more straightforward change:
commit 6af927c40585a4ff75a83b7cdabe8f9074a8d391
Author: Jason Merrill
Date: Fri Jan 25 09:09:17 2019 -0500
PR c++/80916 - spurious "static but not defined" warning.
Nothing can refer to an internal decl with no definition, so we shouldn't
treat such a decl as a possible devirtualization target.
* gimple-fold.c (can_refer_decl_in_current_unit_p): Return false
for an internal function with no definition.
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index 7ef5004f5f9..62d2e0abc26 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -121,9 +121,12 @@ can_refer_decl_in_current_unit_p (tree decl, tree from_decl)
|| !VAR_OR_FUNCTION_DECL_P (decl))
return true;
- /* Static objects can be referred only if they was not optimized out yet. */
- if (!TREE_PUBLIC (decl) && !DECL_EXTERNAL (decl))
+ /* Static objects can be referred only if they are defined and not optimized
+ out yet. */
+ if (!TREE_PUBLIC (decl))
{
+ if (DECL_EXTERNAL (decl))
+ return false;
/* Before we start optimizing unreachable code we can be sure all
static objects are defined. */
if (symtab->function_flags_ready)
diff --git a/gcc/testsuite/g++.dg/warn/unused-fn1.C b/gcc/testsuite/g++.dg/warn/unused-fn1.C
new file mode 100644
index 000..aabc01b3f44
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/unused-fn1.C
@@ -0,0 +1,16 @@
+// PR c++/80916
+// { dg-options "-Os -Wunused" }
+
+struct j {
+ virtual void dispatch(void *) {}
+};
+template
+struct i : j {
+ void dispatch(void *) {} // warning: 'void i< >::dispatch(void*) [with = {anonymous}::l]' declared 'static' but never defined [-Wunused-function]
+};
+namespace {
+ struct l : i {};
+}
+void f(j *k) {
+ k->dispatch(0);
+}
Re: RFA: PATCH to gimple-fold.c for c++/80916, bogus "static but not defined" warning
On Thu, Feb 28, 2019 at 11:58 AM Jan Hubicka wrote: > > Hi, > sorry for late reply - I did not identify it as a patch to symbol table. > Indeed we want can_refer_decl_in_current_unit_p is a good place to test > this. Is there a reason to resrict this to functions with no body? If the function has a definition, then of course we can refer to it in its own unit. Am I missing something? > I see that we may be able to inline the function, but all the > devirtualization code works by first turning call to a direct call and > inlining later. We would need to teach the code that it can't > devirtualize without inlining (which can be done by the speculative call > macinery) and probably we will need to check that the function body does > not contain some calls to similar symbols. > > We don't have support for this (do we want to do that in future)? > For GCC 9 it would thus seem more consistent to simply return false on > all symbols with this combination of flags. We probably also can teach > symtab verifiers that we do not contain any references to such symbols. > > Thanks, > Honza > > Ping^3? > > > > On Wed, Feb 13, 2019 at 4:28 PM Jason Merrill wrote: > > > > > > Ping^2 > > > > > > On Mon, Feb 4, 2019 at 4:09 PM Jason Merrill wrote: > > > > > > > > Ping > > > > > > > > On Fri, Jan 25, 2019 at 10:06 AM Jason Merrill wrote: > > > > > > > > > > Here we warn because i::dispatch has internal linkage (because l > > > > > does) and is never instantiated (because the vtable is never emitted). > > > > > The regression happened because devirtualization started adding it to > > > > > cgraph as a possible target. I think the way to fix this is to avoid > > > > > adding an undefined internal function to cgraph as a possible target, > > > > > since it is not, in fact, possible for it to be the actual target. > > > > > > > > > > I think that the best place to fix this would be in > > > > > can_refer_decl_in_current_unit_p, since the same reasoning applies to > > > > > other possible references. But we could fix it only in > > > > > gimple_get_virt_method_for_vtable. > > > > > > > > > > First patch tested x86_64-pc-linux-gnu. > > > > >
Re: RFA: PATCH to gimple-fold.c for c++/80916, bogus "static but not defined" warning
Hi, sorry for late reply - I did not identify it as a patch to symbol table. Indeed we want can_refer_decl_in_current_unit_p is a good place to test this. Is there a reason to resrict this to functions with no body? I see that we may be able to inline the function, but all the devirtualization code works by first turning call to a direct call and inlining later. We would need to teach the code that it can't devirtualize without inlining (which can be done by the speculative call macinery) and probably we will need to check that the function body does not contain some calls to similar symbols. We don't have support for this (do we want to do that in future)? For GCC 9 it would thus seem more consistent to simply return false on all symbols with this combination of flags. We probably also can teach symtab verifiers that we do not contain any references to such symbols. Thanks, Honza > Ping^3? > > On Wed, Feb 13, 2019 at 4:28 PM Jason Merrill wrote: > > > > Ping^2 > > > > On Mon, Feb 4, 2019 at 4:09 PM Jason Merrill wrote: > > > > > > Ping > > > > > > On Fri, Jan 25, 2019 at 10:06 AM Jason Merrill wrote: > > > > > > > > Here we warn because i::dispatch has internal linkage (because l > > > > does) and is never instantiated (because the vtable is never emitted). > > > > The regression happened because devirtualization started adding it to > > > > cgraph as a possible target. I think the way to fix this is to avoid > > > > adding an undefined internal function to cgraph as a possible target, > > > > since it is not, in fact, possible for it to be the actual target. > > > > > > > > I think that the best place to fix this would be in > > > > can_refer_decl_in_current_unit_p, since the same reasoning applies to > > > > other possible references. But we could fix it only in > > > > gimple_get_virt_method_for_vtable. > > > > > > > > First patch tested x86_64-pc-linux-gnu. > > > >
Re: RFA: PATCH to gimple-fold.c for c++/80916, bogus "static but not defined" warning
Ping^3? On Wed, Feb 13, 2019 at 4:28 PM Jason Merrill wrote: > > Ping^2 > > On Mon, Feb 4, 2019 at 4:09 PM Jason Merrill wrote: > > > > Ping > > > > On Fri, Jan 25, 2019 at 10:06 AM Jason Merrill wrote: > > > > > > Here we warn because i::dispatch has internal linkage (because l > > > does) and is never instantiated (because the vtable is never emitted). > > > The regression happened because devirtualization started adding it to > > > cgraph as a possible target. I think the way to fix this is to avoid > > > adding an undefined internal function to cgraph as a possible target, > > > since it is not, in fact, possible for it to be the actual target. > > > > > > I think that the best place to fix this would be in > > > can_refer_decl_in_current_unit_p, since the same reasoning applies to > > > other possible references. But we could fix it only in > > > gimple_get_virt_method_for_vtable. > > > > > > First patch tested x86_64-pc-linux-gnu. > > >
Re: RFA: PATCH to gimple-fold.c for c++/80916, bogus "static but not defined" warning
Ping^2 On Mon, Feb 4, 2019 at 4:09 PM Jason Merrill wrote: > > Ping > > On Fri, Jan 25, 2019 at 10:06 AM Jason Merrill wrote: > > > > Here we warn because i::dispatch has internal linkage (because l > > does) and is never instantiated (because the vtable is never emitted). > > The regression happened because devirtualization started adding it to > > cgraph as a possible target. I think the way to fix this is to avoid > > adding an undefined internal function to cgraph as a possible target, > > since it is not, in fact, possible for it to be the actual target. > > > > I think that the best place to fix this would be in > > can_refer_decl_in_current_unit_p, since the same reasoning applies to > > other possible references. But we could fix it only in > > gimple_get_virt_method_for_vtable. > > > > First patch tested x86_64-pc-linux-gnu. > >
Re: RFA: PATCH to gimple-fold.c for c++/80916, bogus "static but not defined" warning
Ping On Fri, Jan 25, 2019 at 10:06 AM Jason Merrill wrote: > > Here we warn because i::dispatch has internal linkage (because l > does) and is never instantiated (because the vtable is never emitted). > The regression happened because devirtualization started adding it to > cgraph as a possible target. I think the way to fix this is to avoid > adding an undefined internal function to cgraph as a possible target, > since it is not, in fact, possible for it to be the actual target. > > I think that the best place to fix this would be in > can_refer_decl_in_current_unit_p, since the same reasoning applies to > other possible references. But we could fix it only in > gimple_get_virt_method_for_vtable. > > First patch tested x86_64-pc-linux-gnu. >
RFA: PATCH to gimple-fold.c for c++/80916, bogus "static but not defined" warning
Here we warn because i::dispatch has internal linkage (because l
does) and is never instantiated (because the vtable is never emitted).
The regression happened because devirtualization started adding it to
cgraph as a possible target. I think the way to fix this is to avoid
adding an undefined internal function to cgraph as a possible target,
since it is not, in fact, possible for it to be the actual target.
I think that the best place to fix this would be in
can_refer_decl_in_current_unit_p, since the same reasoning applies to
other possible references. But we could fix it only in
gimple_get_virt_method_for_vtable.
First patch tested x86_64-pc-linux-gnu.
commit 3a02b58301c2c11620c2adc1aee4db1b7e8e36f2
Author: Jason Merrill
Date: Fri Jan 25 09:09:17 2019 -0500
PR c++/80916 - spurious "static but not defined" warning.
* gimple-fold.c (can_refer_decl_in_current_unit_p): Return false
for an internal function with no definition.
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index 92d3fb4a9e0..20564e26de1 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -135,6 +135,12 @@ can_refer_decl_in_current_unit_p (tree decl, tree from_decl)
return !node || !node->global.inlined_to;
}
+ /* This function is internal and not defined, so nothing can refer to it. */
+ if (!TREE_PUBLIC (decl) && DECL_EXTERNAL (decl)
+ && TREE_CODE (decl) == FUNCTION_DECL
+ && DECL_INITIAL (decl) == NULL_TREE)
+return false;
+
/* We will later output the initializer, so we can refer to it.
So we are concerned only when DECL comes from initializer of
external var or var that has been optimized out. */
diff --git a/gcc/testsuite/g++.dg/warn/unused-fn1.C b/gcc/testsuite/g++.dg/warn/unused-fn1.C
new file mode 100644
index 000..aabc01b3f44
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/unused-fn1.C
@@ -0,0 +1,16 @@
+// PR c++/80916
+// { dg-options "-Os -Wunused" }
+
+struct j {
+ virtual void dispatch(void *) {}
+};
+template
+struct i : j {
+ void dispatch(void *) {} // warning: 'void i< >::dispatch(void*) [with = {anonymous}::l]' declared 'static' but never defined [-Wunused-function]
+};
+namespace {
+ struct l : i {};
+}
+void f(j *k) {
+ k->dispatch(0);
+}
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index 92d3fb4a9e0..8d63d815a5e 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -7146,7 +7146,10 @@ gimple_get_virt_method_for_vtable (HOST_WIDE_INT token,
devirtualize. This can happen in WHOPR when the actual method
ends up in other partition, because we found devirtualization
possibility too late. */
- if (!can_refer_decl_in_current_unit_p (fn, vtable))
+ if (!can_refer_decl_in_current_unit_p (fn, vtable)
+ || (!TREE_PUBLIC (decl) && DECL_EXTERNAL (decl)
+ && TREE_CODE (decl) == FUNCTION_DECL
+ && DECL_INITIAL (decl) == NULL_TREE))
{
if (can_refer)
{
