[C++] PR 90909 ("[10 Regression] call devirtualized to pure virtual")

Hi,

this bug notices that the more aggressive de-virtualization check that 
we have now in place (fixed c++/67184) doesn't work correctly for the 
below reproducer, which involves a pure virtual: we de-virtualize and 
the build fails at link-time. To cure this I believe we simply want an 
additional DECL_PURE_VIRTUAL_P in the condition. I also checked that the 
other compilers I have at hand appear to do the same, that is, they 
compile the reproducer both as-is and without the final specifier to the 
same assembly.

Note, in principle we have the option of not doing the additional 
DECL_PURE_VIRTUAL_P check when the final overrider comes from the class 
itself, not from a base, that is in the cases that we were already 
de-virtualizing pre-67184. That is, for something like:

struct A final
{
   virtual void foo () = 0;
};

void fun(A* a, B* b)
{
   a->foo();
}

devirtualize anyway (which then doesn't link). We could add back an '|| 
CLASSTYPE_FINAL (TYPE_METHOD_BASETYPE (TREE_TYPE (fn)))' for that. ICC 
appears to behave this way.

Tested x86_64-linux.

Thanks, Paolo.

////////////////////////
/cp
2019-06-20  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/90909
	* call.c (build_over_call): Do not try to devirtualize when
	then function is pure virtual.

/testsuite
2019-06-20  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/90909
	* g++.dg/other/final6.C: New.

On 6/20/19 12:24 AM, Paolo Carlini wrote:
> Hi,
> 
> this bug notices that the more aggressive de-virtualization check that 
> we have now in place (fixed c++/67184) doesn't work correctly for the 
> below reproducer, which involves a pure virtual: we de-virtualize and 
> the build fails at link-time. To cure this I believe we simply want an 
> additional DECL_PURE_VIRTUAL_P in the condition.

I don't see why this bug would be specific to pure virtual functions, so 
the fix also should not be.  If S1::f is not pure virtual, I'd expect 
that we wrongly devirtualize to it in the same way.

Devirtualizing the call in S4 is a good optimization, we're just 
selecting the wrong function.

It seems like the issue here is that the using-declaration hides the 
final overrider from lookup.  So we need to work harder to find the 
actual final overrider, perhaps by looking into the argtype vtable.

Jason

Hi,

On 21/06/19 20:50, Jason Merrill wrote:
> On 6/20/19 12:24 AM, Paolo Carlini wrote:
>> Hi,
>>
>> this bug notices that the more aggressive de-virtualization check 
>> that we have now in place (fixed c++/67184) doesn't work correctly 
>> for the below reproducer, which involves a pure virtual: we 
>> de-virtualize and the build fails at link-time. To cure this I 
>> believe we simply want an additional DECL_PURE_VIRTUAL_P in the 
>> condition.
>
> I don't see why this bug would be specific to pure virtual functions, 
> so the fix also should not be.  If S1::f is not pure virtual, I'd 
> expect that we wrongly devirtualize to it in the same way.
>
> Devirtualizing the call in S4 is a good optimization, we're just 
> selecting the wrong function.
>
> It seems like the issue here is that the using-declaration hides the 
> final overrider from lookup.  So we need to work harder to find the 
> actual final overrider, perhaps by looking into the argtype vtable.

I see, thanks for the suggestion.

The issue seems rather tricky, then. For the time being I'm going to 
revert the recent improvements. Probably I'm also going to unassign 
myself, because I don't want to prevent somebody else more knowledgeable 
than me in the area from contributing a good solution.

Thanks, Paolo.

Hi,

On 21/06/19 21:27, Paolo Carlini wrote:
> Hi,
>
> On 21/06/19 20:50, Jason Merrill wrote:
>> On 6/20/19 12:24 AM, Paolo Carlini wrote:
>>> Hi,
>>>
>>> this bug notices that the more aggressive de-virtualization check 
>>> that we have now in place (fixed c++/67184) doesn't work correctly 
>>> for the below reproducer, which involves a pure virtual: we 
>>> de-virtualize and the build fails at link-time. To cure this I 
>>> believe we simply want an additional DECL_PURE_VIRTUAL_P in the 
>>> condition.
>>
>> I don't see why this bug would be specific to pure virtual functions, 
>> so the fix also should not be.  If S1::f is not pure virtual, I'd 
>> expect that we wrongly devirtualize to it in the same way.

By the way, if S1:.f is not pure virtual, just a virtual declaration - 
all the rest being the same - the code doesn't link: undefined 
references to vtable and typeinfo for S1. The same happens with current 
clang and icc. I don't know if this detail helps us in the short term....

Paolo.

and...
> By the way, if S1:.f is not pure virtual, just a virtual declaration - 
> all the rest being the same - the code doesn't link: undefined 
> references to vtable and typeinfo for S1. The same happens with 
> current clang and icc. I don't know if this detail helps us in the 
> short term....

... the same happens without final too.

Paolo.

... so, now that I see the issue more clearly, I'm adding to the 
testsuite the below too.

Thanks, Paolo.

////////////////////////
Index: final7.C
===================================================================
--- final7.C	(nonexistent)
+++ final7.C	(working copy)
@@ -0,0 +1,11 @@
+// PR c++/90909
+// { dg-do run { target c++11 } }
+
+#include <cassert>
+
+struct S1 { virtual bool f() { return false; } };
+struct S2: S1 { virtual bool f() { return true; } };
+struct S3: S2 { using S1::f; };
+struct S4 final: S3 { void g(); };
+void S4::g() { assert (f() == true); }
+int main() { S4().g(); }

... hi again ;)

The other day I was having a look at using declarations for this issue 
and noticed that only a few lines below the de-virtualization check we 
have to handle functions found by a using declaration, for various 
reasons. In particular, we know whether we found a function fn where has 
been declared or in a derived class. Thus the idea: for the purpose of 
making some progress, in particular all the cases in c++/67184 & co, 
would it make sense for the time being to simply add a check to the 
de-virtualization condition restricting it to non-using declarations? 
See the below (it also moves the conditional a few lines below only for 
clarity and consistency with the code handling using declarations, no 
functional impact) What do you think?

Thanks, Paolo.

///////////////////
Index: cp/call.c
===================================================================
--- cp/call.c	(revision 272583)
+++ cp/call.c	(working copy)
@@ -8241,12 +8241,6 @@ build_over_call (struct z_candidate *cand, int fla
 	    return error_mark_node;
 	}
 
-      /* See if the function member or the whole class type is declared
-	 final and the call can be devirtualized.  */
-      if (DECL_FINAL_P (fn)
-	  || CLASSTYPE_FINAL (TYPE_METHOD_BASETYPE (TREE_TYPE (fn))))
-	flags |= LOOKUP_NONVIRTUAL;
-
       /* [class.mfct.nonstatic]: If a nonstatic member function of a class
 	 X is called for an object that is not of type X, or of a type
 	 derived from X, the behavior is undefined.
@@ -8271,6 +8265,17 @@ build_over_call (struct z_candidate *cand, int fla
 	  else
 	    return error_mark_node;
 	}
+
+      /* See if the function member or the whole class type is declared
+	 final and the call can be devirtualized.  */
+      if (DECL_FINAL_P (fn)
+	  || (CLASSTYPE_FINAL (TREE_TYPE (argtype))
+	      /* Give up for now if fn was found by a using declaration,
+		 the complex case, see c++/90909.  */
+	      && (TREE_TYPE (TREE_TYPE (converted_arg))
+		  == TREE_TYPE (parmtype))))
+	flags |= LOOKUP_NONVIRTUAL;
+
       /* If fn was found by a using declaration, the conversion path
 	 will be to the derived class, not the base declaring fn. We
 	 must convert from derived to base.  */
Index: testsuite/g++.dg/other/final3.C
===================================================================
--- testsuite/g++.dg/other/final3.C	(nonexistent)
+++ testsuite/g++.dg/other/final3.C	(working copy)
@@ -0,0 +1,28 @@
+// PR c++/67184
+// { dg-do compile { target c++11 } }
+// { dg-options "-fdump-tree-original"  }
+
+struct V {
+ virtual void foo(); 
+};
+
+struct wV final : V {
+};
+
+struct oV final : V {
+  void foo();
+};
+
+void call(wV& x)
+{
+  x.foo();
+  x.V::foo();
+}
+
+void call(oV& x)
+{
+  x.foo();
+  x.V::foo();
+}
+
+// { dg-final { scan-tree-dump-times "OBJ_TYPE_REF" 0 "original" } }
Index: testsuite/g++.dg/other/final4.C
===================================================================
--- testsuite/g++.dg/other/final4.C	(nonexistent)
+++ testsuite/g++.dg/other/final4.C	(working copy)
@@ -0,0 +1,16 @@
+// PR c++/67184
+// { dg-do compile { target c++11 } }
+// { dg-options "-fdump-tree-original"  }
+
+struct B
+{
+  virtual void operator()();
+  virtual operator int();
+  virtual int operator++();
+};
+
+struct D final : B { };
+
+void foo(D& d) { d(); int t = d; ++d; }
+
+// { dg-final { scan-tree-dump-times "OBJ_TYPE_REF" 0 "original" } }
Index: testsuite/g++.dg/other/final5.C
===================================================================
--- testsuite/g++.dg/other/final5.C	(nonexistent)
+++ testsuite/g++.dg/other/final5.C	(working copy)
@@ -0,0 +1,19 @@
+// PR c++/69445
+// { dg-do compile { target c++11 } }
+// { dg-options "-fdump-tree-original"  }
+
+struct Base {
+  virtual void foo() const = 0;
+  virtual void bar() const {}
+};
+
+struct C final : Base {
+  void foo() const { }
+};
+
+void func(const C & c) {
+  c.bar();
+  c.foo();
+}
+
+// { dg-final { scan-tree-dump-times "OBJ_TYPE_REF" 0 "original" } }

On 6/23/19 7:53 AM, Paolo Carlini wrote:
> ... hi again ;)
> 
> The other day I was having a look at using declarations for this issue 
> and noticed that only a few lines below the de-virtualization check we 
> have to handle functions found by a using declaration, for various 
> reasons. In particular, we know whether we found a function fn where has 
> been declared or in a derived class. Thus the idea: for the purpose of 
> making some progress, in particular all the cases in c++/67184 & co, 
> would it make sense for the time being to simply add a check to the 
> de-virtualization condition restricting it to non-using declarations? 
> See the below (it also moves the conditional a few lines below only for 
> clarity and consistency with the code handling using declarations, no 
> functional impact) What do you think?

Hmm, perhaps we should check CLASSTYPE_FINAL in resolves_to_fixed_type_p 
rather than in build_over_call at all; then the code in 
build_new_method_call ought to set LOOKUP_NONVIRTUAL when appropriate.

Jason

Hi,

On 23/06/19 19:45, Jason Merrill wrote:
> On 6/23/19 7:53 AM, Paolo Carlini wrote:
>> ... hi again ;)
>>
>> The other day I was having a look at using declarations for this 
>> issue and noticed that only a few lines below the de-virtualization 
>> check we have to handle functions found by a using declaration, for 
>> various reasons. In particular, we know whether we found a function 
>> fn where has been declared or in a derived class. Thus the idea: for 
>> the purpose of making some progress, in particular all the cases in 
>> c++/67184 & co, would it make sense for the time being to simply add 
>> a check to the de-virtualization condition restricting it to 
>> non-using declarations? See the below (it also moves the conditional 
>> a few lines below only for clarity and consistency with the code 
>> handling using declarations, no functional impact) What do you think?
>
> Hmm, perhaps we should check CLASSTYPE_FINAL in 
> resolves_to_fixed_type_p rather than in build_over_call at all; then 
> the code in build_new_method_call ought to set LOOKUP_NONVIRTUAL when 
> appropriate.

I think your suggestion has to do with the initial implementation of 
this optimization, as contributed by my friend Roberto Agostino: we had 
the issue that it didn't handle at all user-defined operators and 
Vincenzo filed c++/53186. Thus, upon your suggestion, we moved the code 
to build_over_call, the current place:

     https://gcc.gnu.org/ml/gcc-patches/2012-05/msg00246.html

where it catched both member functions and operators. Now - before we 
get to the details - if I move the CLASSTYPE_FINAL check to 
resolves_to_fixed_type_p we exactly regress on c++/53186, that is 
other/final2.C, because resolves_to_fixed_type_p is *never* called. The 
pending final4.C, also involving operators (I constructed it exactly 
because I knew operators could be tricky) is also not fixed, but in that 
case at least resolves_to_fixed_type_p *is* called, only, too late (I 
think, I more details later, if you like).

All the other existing and pending testcases - involving member 
functions - appear to be Ok, even with a draft implementation of your 
suggestion (I slapped a 'if (CLASS_TYPE_P (t) && CLASSTYPE_FINAL (t)) 
return true;' in the middle of resolves_to_fixed_type_p.

Thanks, Paolo.

On 6/24/19 4:52 AM, Paolo Carlini wrote:
> Hi,
> 
> On 23/06/19 19:45, Jason Merrill wrote:
>> On 6/23/19 7:53 AM, Paolo Carlini wrote:
>>> ... hi again ;)
>>>
>>> The other day I was having a look at using declarations for this 
>>> issue and noticed that only a few lines below the de-virtualization 
>>> check we have to handle functions found by a using declaration, for 
>>> various reasons. In particular, we know whether we found a function 
>>> fn where has been declared or in a derived class. Thus the idea: for 
>>> the purpose of making some progress, in particular all the cases in 
>>> c++/67184 & co, would it make sense for the time being to simply add 
>>> a check to the de-virtualization condition restricting it to 
>>> non-using declarations? See the below (it also moves the conditional 
>>> a few lines below only for clarity and consistency with the code 
>>> handling using declarations, no functional impact) What do you think?
>>
>> Hmm, perhaps we should check CLASSTYPE_FINAL in 
>> resolves_to_fixed_type_p rather than in build_over_call at all; then 
>> the code in build_new_method_call ought to set LOOKUP_NONVIRTUAL when 
>> appropriate.
> 
> I think your suggestion has to do with the initial implementation of 
> this optimization, as contributed by my friend Roberto Agostino: we had 
> the issue that it didn't handle at all user-defined operators and 
> Vincenzo filed c++/53186. Thus, upon your suggestion, we moved the code 
> to build_over_call, the current place:
> 
>      https://gcc.gnu.org/ml/gcc-patches/2012-05/msg00246.html
> 
> where it catched both member functions and operators. Now - before we 
> get to the details - if I move the CLASSTYPE_FINAL check to 
> resolves_to_fixed_type_p we exactly regress on c++/53186, that is 
> other/final2.C, because resolves_to_fixed_type_p is *never* called. The 
> pending final4.C, also involving operators (I constructed it exactly 
> because I knew operators could be tricky) is also not fixed, but in that 
> case at least resolves_to_fixed_type_p *is* called, only, too late (I 
> think, I more details later, if you like).

Ah, thanks.  Then perhaps we want to change the CLASSTYPE_FINAL in 
build_over_call to resolves_to_fixed_type_p (arg), to also handle the 
other reasons we might know the dynamic type of the argument, and remove 
the related code from build_new_method_call_1?

You could avoid needing to move the conditional lower by comparing 
DECL_CONTEXT (fn) and BINFO_TYPE (cand->conversion_path) rather than 
parmtype and TREE_TYPE (converted_arg).

Jason

Hi,

On 27/06/19 23:19, Jason Merrill wrote:
> Ah, thanks.  Then perhaps we want to change the CLASSTYPE_FINAL in 
> build_over_call to resolves_to_fixed_type_p (arg), to also handle the 
> other reasons we might know the dynamic type of the argument, and 
> remove the related code from build_new_method_call_1?
>
> You could avoid needing to move the conditional lower by comparing 
> DECL_CONTEXT (fn) and BINFO_TYPE (cand->conversion_path) rather than 
> parmtype and TREE_TYPE (converted_arg).

Sorry for late replying, a few days off.

Anyway, great, it looks like we are reaching a nice synthesis. I must 
admit that until yesterday I hadn't noticed that Fabien dealt precisely 
with using declarations in order to fix c++/11750, thus the existing 
check in build_new_method_call_1 is exactly what we need. The below does 
that and passes testing, in it I didn't keep the checks of DECL_VINDEX 
(fn) && ! (flags & LOOKUP_NONVIRTUAL) which don't seem necessary, might 
avoid function calls, though. Let me know...

Thanks, Paolo.

////////////////////
Index: cp/call.c
===================================================================
--- cp/call.c	(revision 273076)
+++ cp/call.c	(working copy)
@@ -8241,10 +8241,17 @@ build_over_call (struct z_candidate *cand, int fla
 	    return error_mark_node;
 	}
 
-      /* See if the function member or the whole class type is declared
-	 final and the call can be devirtualized.  */
+      /* Optimize away vtable lookup if we know that this
+	 function can't be overridden.  We need to check if
+	 the context and the type where we found fn are the same,
+	 actually FN might be defined in a different class
+	 type because of a using-declaration. In this case, we
+	 do not want to perform a non-virtual call.  Note that
+	 resolves_to_fixed_type_p checks CLASSTYPE_FINAL too.  */
       if (DECL_FINAL_P (fn)
-	  || CLASSTYPE_FINAL (TYPE_METHOD_BASETYPE (TREE_TYPE (fn))))
+	  || (resolves_to_fixed_type_p (arg, 0)
+	      && same_type_ignoring_top_level_qualifiers_p
+	      (DECL_CONTEXT (fn), BINFO_TYPE (cand->conversion_path)))) 
 	flags |= LOOKUP_NONVIRTUAL;
 
       /* [class.mfct.nonstatic]: If a nonstatic member function of a class
@@ -9845,17 +9852,6 @@ build_new_method_call_1 (tree instance, tree fns,
 
 	  if (call != error_mark_node)
 	    {
-	      /* Optimize away vtable lookup if we know that this
-		 function can't be overridden.  We need to check if
-		 the context and the type where we found fn are the same,
-		 actually FN might be defined in a different class
-		 type because of a using-declaration. In this case, we
-		 do not want to perform a non-virtual call.  */
-	      if (DECL_VINDEX (fn) && ! (flags & LOOKUP_NONVIRTUAL)
-		  && same_type_ignoring_top_level_qualifiers_p
-		  (DECL_CONTEXT (fn), BINFO_TYPE (binfo))
-		  && resolves_to_fixed_type_p (instance, 0))
-		flags |= LOOKUP_NONVIRTUAL;
               if (explicit_targs)
                 flags |= LOOKUP_EXPLICIT_TMPL_ARGS;
 	      /* Now we know what function is being called.  */
Index: testsuite/g++.dg/other/final4.C
===================================================================
--- testsuite/g++.dg/other/final4.C	(nonexistent)
+++ testsuite/g++.dg/other/final4.C	(working copy)
@@ -0,0 +1,16 @@
+// PR c++/67184
+// { dg-do compile { target c++11 } }
+// { dg-options "-fdump-tree-original"  }
+
+struct B
+{
+  virtual void operator()();
+  virtual operator int();
+  virtual int operator++();
+};
+
+struct D final : B { };
+
+void foo(D& d) { d(); int t = d; ++d; }
+
+// { dg-final { scan-tree-dump-times "OBJ_TYPE_REF" 0 "original" } }

On 7/4/19 8:56 AM, Paolo Carlini wrote:
> Hi,
> 
> On 27/06/19 23:19, Jason Merrill wrote:
>> Ah, thanks.  Then perhaps we want to change the CLASSTYPE_FINAL in 
>> build_over_call to resolves_to_fixed_type_p (arg), to also handle the 
>> other reasons we might know the dynamic type of the argument, and 
>> remove the related code from build_new_method_call_1?
>>
>> You could avoid needing to move the conditional lower by comparing 
>> DECL_CONTEXT (fn) and BINFO_TYPE (cand->conversion_path) rather than 
>> parmtype and TREE_TYPE (converted_arg).
> 
> Sorry for late replying, a few days off.
> 
> Anyway, great, it looks like we are reaching a nice synthesis. I must 
> admit that until yesterday I hadn't noticed that Fabien dealt precisely 
> with using declarations in order to fix c++/11750, thus the existing 
> check in build_new_method_call_1 is exactly what we need. The below does 
> that and passes testing, in it I didn't keep the checks of DECL_VINDEX 
> (fn) && ! (flags & LOOKUP_NONVIRTUAL) which don't seem necessary, might 
> avoid function calls, though. Let me know...

Yeah, they're an optimization to avoid the calls when they aren't 
necessary but I wouldn't expect that the difference is measurable.  The 
patch is OK, thanks.

Jason

On Thu, Jul 04, 2019 at 02:56:47PM +0200, Paolo Carlini wrote:
> --- cp/call.c	(revision 273076)
> +++ cp/call.c	(working copy)
> @@ -9845,17 +9852,6 @@ build_new_method_call_1 (tree instance, tree fns,
>  
>  	  if (call != error_mark_node)
>  	    {
> -	      /* Optimize away vtable lookup if we know that this
> -		 function can't be overridden.  We need to check if
> -		 the context and the type where we found fn are the same,
> -		 actually FN might be defined in a different class
> -		 type because of a using-declaration. In this case, we
> -		 do not want to perform a non-virtual call.  */
> -	      if (DECL_VINDEX (fn) && ! (flags & LOOKUP_NONVIRTUAL)
> -		  && same_type_ignoring_top_level_qualifiers_p
> -		  (DECL_CONTEXT (fn), BINFO_TYPE (binfo))
> -		  && resolves_to_fixed_type_p (instance, 0))
> -		flags |= LOOKUP_NONVIRTUAL;
>                if (explicit_targs)
>                  flags |= LOOKUP_EXPLICIT_TMPL_ARGS;
>  	      /* Now we know what function is being called.  */

This change broke bootstrap, as it removes the last use of binfo
variable besides the setter of that variable.

I'll commit following as obvious if I get successfully past that point in
bootstrap:

2019-07-05  Jakub Jelinek  <jakub@redhat.com>

	PR c++/67184
	PR c++/69445
	* call.c (build_new_method_call_1): Remove set but not used variable
	binfo.

--- gcc/call.c.jj	2019-07-05 22:09:49.694367815 +0200
+++ gcc/call.c	2019-07-05 22:25:58.476016114 +0200
@@ -9564,7 +9564,7 @@ build_new_method_call_1 (tree instance,
   struct z_candidate *candidates = 0, *cand;
   tree explicit_targs = NULL_TREE;
   tree basetype = NULL_TREE;
-  tree access_binfo, binfo;
+  tree access_binfo;
   tree optype;
   tree first_mem_arg = NULL_TREE;
   tree name;
@@ -9603,7 +9603,6 @@ build_new_method_call_1 (tree instance,
   if (!conversion_path)
     conversion_path = BASELINK_BINFO (fns);
   access_binfo = BASELINK_ACCESS_BINFO (fns);
-  binfo = BASELINK_BINFO (fns);
   optype = BASELINK_OPTYPE (fns);
   fns = BASELINK_FUNCTIONS (fns);
   if (TREE_CODE (fns) == TEMPLATE_ID_EXPR)


	Jakub