Re: [patch] Fix PR c++/22604, ICE after invalid covariant return

[Volker Reichelt <reichelt at igpm dot rwth-aachen dot de>]

On  9 Aug, Volker Reichelt wrote:
> On  5 Aug, Mark Mitchell wrote:
>> Volker Reichelt wrote:
>>> Consider the following testcase:
>>> 
>>>   struct A;
>>> 
>>>   struct B
>>>   {
>>>     virtual A* foo();  // { dg-error "overriding" }
>>>   };
>>> 
>>>   struct C : B
>>>   {
>>>     virtual C* foo();  // { dg-error "invalid covariant" }
>>>   };
>>> 
>>> The declaration of foo in struct C is invalid.
>>> When updating the vtable for C the compiler fails to check for invalid
>>> overriders in update_vtable_entry_for_fn, thus causing an ICE.
>> 
>> What about making foo non-virtual when the error occurs?  If that's 
>> hard, your patch is OK.
> 
> look_for_overrides_r calls check_final_overrider to test whether
> the overrider is valid. In the testcase above the overrider is invalid,
> therefore check_final_overrider emits an error message and returns zero.
> 
> I tried the following patch to mark the function as non-virtual in
> this case:
> 
> ===================================================================
> --- gcc/gcc/cp/search.c	2 Jul 2005 10:54:57 -0000	1.351
> +++ gcc/gcc/cp/search.c	8 Aug 2005 18:14:07 -0000
> @@ -1971,7 +1971,8 @@ look_for_overrides_r (tree type, tree fn
>  	{
>  	  /* It's definitely virtual, even if not explicitly set.  */
>  	  DECL_VIRTUAL_P (fndecl) = 1;
> -	  check_final_overrider (fndecl, fn);
> +	  if (!check_final_overrider (fndecl, fn))
> +	    DECL_VIRTUAL_P (fndecl) = 0;
>  	}
>        return 1;
>      }
> ===================================================================
> 
> This indeed fixes the testcase above, but I still get an ICE
> deep in the bowels of update_vtable_entry_for_fn for the testcase below:
> 
> ===========================
> struct A;
> 
> struct B
> {
>     virtual B* foo();
> };
> 
> struct C : B
> {
>     virtual A* foo();
> };
> 
> struct D : C
> {
>     virtual D* foo();
> };
> ===========================
> 
> Maybe some other bits have to be changed as well for marking the function
> as non-virtual. What am I missing here?
> 
> Or should I stick with the original patch that fixes all the testcases?

I just found the following testcase (I'll refer to it as testcase #3,
and to the one above as testcase #2):

=========================
struct A;

struct B
{
    virtual A* foo();
};

struct C : B
{
    virtual C* foo();
};

struct D : C
{
    virtual D* foo();
};
========================

This one crashes the version with the original patch, but works fine
with the patch above. It seems that either way, an additional patch
for update_vtable_entry_for_fn is needed, since base_binfo can be 0.

===================================================================
--- class.c     1 Aug 2005 04:02:23 -0000       1.730
+++ class.c     9 Aug 2005 10:42:22 -0000
@@ -2020,19 +2022,18 @@ update_vtable_entry_for_fn (tree t, tree
             order.  Of course it is lame that we have to repeat the
             search here anyway -- we should really be caching pieces
             of the vtable and avoiding this repeated work.  */
-         tree thunk_binfo, base_binfo;
+         tree thunk_binfo = TYPE_BINFO (over_return);
+	  tree base_binfo = TYPE_BINFO (base_return);
 
          /* Find the base binfo within the overriding function's
             return type.  We will always find a thunk_binfo, except
             when the covariancy is invalid (which we will have
             already diagnosed).  */
-         for (base_binfo = TYPE_BINFO (base_return),
-              thunk_binfo = TYPE_BINFO (over_return);
-              thunk_binfo;
-              thunk_binfo = TREE_CHAIN (thunk_binfo))
-           if (SAME_BINFO_TYPE_P (BINFO_TYPE (thunk_binfo),
-                                  BINFO_TYPE (base_binfo)))
-             break;
+         if (base_binfo)
+           for ( ; thunk_binfo; thunk_binfo = TREE_CHAIN (thunk_binfo))
+             if (SAME_BINFO_TYPE_P (BINFO_TYPE (thunk_binfo),
+                                    BINFO_TYPE (base_binfo)))
+               break;
 
          /* See if virtual inheritance is involved.  */
          for (virtual_offset = thunk_binfo;
===================================================================

With this additional patch and either my original patch or the one with
DECL_VIRTUAL_P the compiler works fine with all three testcases.

I haven't bootstrapped and regtested this, though.

Which way is the preferrable one?

Regards,
Volker