Re: [patchv2] compile: Fix crash on cv-qualified self-reference

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Wed, 01 Jul 2015 13:21:32 +0200, Yao Qi wrote:
> Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> > +  quals = 0;
> > +  if (TYPE_CONST (type))
> > +    quals |= GCC_QUALIFIER_CONST;
> > +  if (TYPE_VOLATILE (type))
> > +    quals |= GCC_QUALIFIER_VOLATILE;
> > +  if (TYPE_RESTRICT (type))
> > +    quals |= GCC_QUALIFIER_RESTRICT;
> > +  result = C_CTX (context)->c_ops->build_qualified_type (C_CTX (context),
> > +							 result, quals);
> >    insert_type (context, type, result);
> 
> Can we use convert_qualified instead?  I find code we added here is
> quite similar to convert_qualified.

Not directly to use convert_qualified() but convert_qualified() could be split
in halves and one half could be used here, I agree.


> >    for (i = 0; i < TYPE_NFIELDS (type); ++i)
> > @@ -329,10 +342,13 @@ static gcc_type
> >  convert_type_basic (struct compile_c_instance *context, struct type *type)
> >  {
> >    /* If we are converting a qualified type, first convert the
> > -     unqualified type and then apply the qualifiers.  */
> > +     unqualified type and then apply the qualifiers, except for the
> > +     types handling qualifiers on their own.  */
> >    if ((TYPE_INSTANCE_FLAGS (type) & (TYPE_INSTANCE_FLAG_CONST
> >  				     | TYPE_INSTANCE_FLAG_VOLATILE
> > -				     | TYPE_INSTANCE_FLAG_RESTRICT)) != 0)
> > +				     | TYPE_INSTANCE_FLAG_RESTRICT)) != 0
> > +      && (TYPE_CODE (type) != TYPE_CODE_STRUCT
> > +	  && TYPE_CODE (type) != TYPE_CODE_UNION))
> >      return convert_qualified (context, type);
> 
> It looks right to me, however, isn't cleaner to do in this way?
> 
>    /* Comments on why we do this first */
>    if (TYPE_CODE (type) == TYPE_CODE_STRUCT
>        || TYPE_CODE (type) == TYPE_CODE_UNION)
>        return convert_struct_or_union (context, type);
> 
>   /* If we are converting a qualified type, first convert the
>      unqualified type and then apply the qualifiers.  */
>   if ((TYPE_INSTANCE_FLAGS (type) & (TYPE_INSTANCE_FLAG_CONST
> 				     | TYPE_INSTANCE_FLAG_VOLATILE
> 				     | TYPE_INSTANCE_FLAG_RESTRICT)) != 0)
>     return convert_qualified (context, type);
> 
>   switch (TYPE_CODE (type))
>    {
>       /* Don't need to handle TYPE_CODE_STRUCT and TYPE_CODE_UNION
>          here.  */
>    }

I can change it that way but when you ask "isn't cleaner" then no, I think
your hack is even a bit more ugly than my ugly hack.

There should be two virtual methods, one pure for 'switch (TYPE_CODE (type))'
and the other one checking TYPE_INSTANCE_FLAG* in superclass overriden only by
TYPE_CODE_STRUCT and TYPE_CODE_UNION (there would be no TYPE_CODE_*, though).


> Otherwise, the patch looks good to me.

OK, thanks.  Just it causes a regression with latest GCC now as I have asked
Alexandre Oliva off-list how it really should be fixed.


Jan