RFA: PATCH to gimple-fold.c for c++/80916, bogus "static but not defined" warning

Here we warn because i<l>::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.

Ping

On Fri, Jan 25, 2019 at 10:06 AM Jason Merrill <jason@redhat.com> wrote:
>
> Here we warn because i<l>::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.
>

Ping^2

On Mon, Feb 4, 2019 at 4:09 PM Jason Merrill <jason@redhat.com> wrote:
>
> Ping
>
> On Fri, Jan 25, 2019 at 10:06 AM Jason Merrill <jason@redhat.com> wrote:
> >
> > Here we warn because i<l>::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.
> >

Ping^3?

On Wed, Feb 13, 2019 at 4:28 PM Jason Merrill <jason@redhat.com> wrote:
>
> Ping^2
>
> On Mon, Feb 4, 2019 at 4:09 PM Jason Merrill <jason@redhat.com> wrote:
> >
> > Ping
> >
> > On Fri, Jan 25, 2019 at 10:06 AM Jason Merrill <jason@redhat.com> wrote:
> > >
> > > Here we warn because i<l>::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.
> > >

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 <jason@redhat.com> wrote:
> >
> > Ping^2
> >
> > On Mon, Feb 4, 2019 at 4:09 PM Jason Merrill <jason@redhat.com> wrote:
> > >
> > > Ping
> > >
> > > On Fri, Jan 25, 2019 at 10:06 AM Jason Merrill <jason@redhat.com> wrote:
> > > >
> > > > Here we warn because i<l>::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.
> > > >

On Thu, Feb 28, 2019 at 11:58 AM Jan Hubicka <hubicka@ucw.cz> 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 <jason@redhat.com> wrote:
> > >
> > > Ping^2
> > >
> > > On Mon, Feb 4, 2019 at 4:09 PM Jason Merrill <jason@redhat.com> wrote:
> > > >
> > > > Ping
> > > >
> > > > On Fri, Jan 25, 2019 at 10:06 AM Jason Merrill <jason@redhat.com> wrote:
> > > > >
> > > > > Here we warn because i<l>::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.
> > > > >

On Thu, Feb 28, 2019 at 12:18 PM Jason Merrill <jason@redhat.com> wrote:
> On Thu, Feb 28, 2019 at 11:58 AM Jan Hubicka <hubicka@ucw.cz> 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 <jason@redhat.com>
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 00000000000..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 <typename>
+struct i : j {
+  void dispatch(void *) {} // warning: 'void i< <template-parameter-1-1> >::dispatch(void*) [with <template-parameter-1-1> = {anonymous}::l]' declared 'static' but never defined [-Wunused-function]
+};
+namespace {
+  struct l : i<l> {};
+}
+void f(j *k) {
+  k->dispatch(0);
+}

> On Thu, Feb 28, 2019 at 12:18 PM Jason Merrill <jason@redhat.com> wrote:
> > On Thu, Feb 28, 2019 at 11:58 AM Jan Hubicka <hubicka@ucw.cz> 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 <jason@redhat.com>
> 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 00000000000..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 <typename>
> +struct i : j {
> +  void dispatch(void *) {} // warning: 'void i< <template-parameter-1-1> >::dispatch(void*) [with <template-parameter-1-1> = {anonymous}::l]' declared 'static' but never defined [-Wunused-function]
> +};
> +namespace {
> +  struct l : i<l> {};
> +}
> +void f(j *k) {
> +  k->dispatch(0);
> +}