Re: [C++ Patch] PR 3761 (ICE in check_template_shadow)

[Scott Brumbaugh <scottb dot lists at verizon dot net>]

On Sun, 24 Aug 2003, Kriang Lerdsuwanakij wrote:

> I have looked at this bug before.  The real problem is
> actually up in the parser.  The declaration
> 
>           friend void Foo::func(void);
> 
> somehow causes the names inside Foo to be merged to the current
> scope.  This triggers all sort of ambiguity.
> 
> --Kriang
> 


Hi Kriang,

Thanks for replying to this patch proposal.  I would like to
understand where the problem is occurring in the parser.  I am still
learning how the C++ front-end works and trying to fix some bugs really
seems like a good way to do it.  So help me to increase my
understanding.

You say that the problem is in the parser.  At what point in the parse
do you see this problem developing?  From what I see, it seems like
the parser behavior is pretty consistent but the ICE is caused by an
infrequently encountered state.  That being the scope Foo<T>, a
template is made current for name lookup inside a nested class before
the definition of Foo<T> is complete.  I will try and explain what I
mean here in more detail in the following.  Please correct any
misunderstanding I may have.  For reference I will repeat the code
snippet under question.

struct A {};
struct B {};

template <class T>
struct Foo : A, B
{
    void func(void);
    struct Nested
    {
        friend void Foo::func(void);    // <-- analysis starts here
					// We have just parsed the
					// declarator-id Foo::func
    };
};

In my explanation I will assume that the parse has gone correctly
until the friend declaration is encountered.  I realize that this
assumption may be totally wrong!

We start from cp_parser_direct_declarator after the qualified-id
Foo::func has been parsed by cp_parser_declarator_id.
cp_parser_declarator_id returns a SCOPE_REF, the first argument is the
scope Foo<T>, the second is an identifier node func, an
unqualified-id.  The parser calls push_scope( 'Foo<T>' ) so name
lookups for the rest of this declaration will be done in the scope of
Foo<T>.

Ok, push_scope eventually calls push_class_decls which caches every
name in the Foo<T> inheritance graph.  Maybe this is the spot where
you state that the names are all merged into to current scope,
resulting in ambiguities.  Is this correct, or are you referring to a
different point in the parse?

I think that it is clear that in this case the ambiguities come from
the multiple base classes A and B of Foo<T>.  The fatal ambiguity in
this case results from an implicitly defined member function,
operator= (one of several) which exists in the complete base classes A
and B, but not the incomplete Foo<T>.

Continuing on, push_class_decls walks the Foo<T> inheritance graph
calling dfs_push_decls on each base.  For base A, setup_class_bindings
is called and passed the identifier node for A's implicit operator=.
It is in setup_class_bindings where things then start to get more
complicated.  This is because setup_class_bindings calls
lookup_method('Foo<T>', 'operator=' ).  Foo<T> is current_class_type
because we have done push_scope('Foo<T>').  But, at this point, Foo<T>
is not completely defined.  So Foo<T>'s own implicit operator= has not
yet been added by the parser.  The lookup_member call finds an
operator= in both base class A and base class B. So, lookup_member
'does the right thing' and returns a TREE_LIST with
TREE_TYPE=error_mark_node and TREE_VALUE set to the chain of ambiguous
operator= FUNCTION_DECL nodes.

It seems like this behavior is anticipated because there is a special
case coded just for this ambiguous lookup in setup_class_bindings.  In
this fragment from search.c:2119, value_binding holds the TREE_LIST
returned by lookup_member:

      if (TREE_CODE (value_binding) == TREE_LIST
          && TREE_TYPE (value_binding) == error_mark_node)
        /* NAME is ambiguous.  */
        push_class_level_binding (name, value_binding);


Now, the above shows that push_class_level_binding is explicitly
called and passed the result of the ambiguous lookup.  Further on,
inside push_class_level_binding, check_template_shadow is called and
passed the TREE_LIST containing the chain of ambiguous operator=
FUNCTION_DECLS.  The reason that this ICE doesn't show up more often
is that this call to push_class_level_binding with the ambiguous
TREE_LIST argument is happening inside of a template.  In many other
cases of multiple inheritance where this ambiguity happens and a
TREE_LIST is passed, the function check_template_shadow will
short-circuit and return early because current_template_parms will be
0.  The ICE occurs at pt.c:1935 in check_template_shadow where the
TREE_LIST is evaluated by the DECL_NAME macro, hence the patch:

     /* Figure out what we're shadowing.  */
     if (TREE_CODE (decl) == OVERLOAD)
       decl = OVL_CURRENT (decl);
 +   /* lookup_member will return a TREE_LIST if a name is ambiguous, the
 +      TREE_TYPE will be error_mark_node, TREE_VALUE will hold all the
 +      ambiguous candidates. */
 +   else if (TREE_CODE (decl) == TREE_LIST
 +        && TREE_TYPE (decl) == error_mark_node)
 +     decl = TREE_VALUE (decl);
 +
     olddecl = IDENTIFIER_VALUE (DECL_NAME (decl));


The patch treats the ambiguous decl chain the same way chain of
OVERLOAD decls would be treated when checking for the reuse of a
template parameter.

Well, that was pretty long winded...  Maybe I am overlooking something
completely obvious, but it has been fun trying to figure this out.
All comments will be appreciated.


Thanks,


Scott Brumbaugh