Gengtype : strange code in output_type_enum

Gengtype : strange code in output_type_enum

[jeremie.salvucci]

Hello all,

While hacking on gengtype with Basile, we noticed a strange piece of code at line 2539 in gcc/gengtype.c r162692

static void
output_type_enum (outf_p of, type_p s)
{
  if (s->kind == TYPE_PARAM_STRUCT && s->u.s.line.file != NULL) /* Strange code @@*/
    {
      oprintf (of, ", gt_e_");
      output_mangled_typename (of, s);
    }
  else if (UNION_OR_STRUCT_P (s) && s->u.s.line.file != NULL)
    {
      oprintf (of, ", gt_ggc_e_");
      output_mangled_typename (of, s);
    }
  else
    oprintf (of, ", gt_types_enum_last");
}

We think that the enum type_kind discriminates fields union in struct type. So for TYPE_PARAM_STRUCT we believe that 
the param_struct field of union u inside struct type is used. If this is true, the test s->u.s.line.file != NULL is meaningless when s->kind == TYPE_PARAM_STRUCT, it should be s->u.param_struct.line.file != NULL instead in our opinion.

However, the existing code appears to work but we don't understand why.

Or can a type have a kind TYPE_PARAM_STRUCT and only have s->u.s valid? It might be related to the code in new_structure near line  638 of gengtype.c which sets ls->kind = TYPE_LANG_STRUCT.

Perhaps TYPE_PARAM_STRUCT has two different roles. If that is indeed the case, we have to distinguish them when serializing gengtype's state.

Cheers.

-- 

Jeremie Salvucci & Basile Starynkevitch

Re: Gengtype : strange code in output_type_enum

[Laurynas Biveinis]

2010/8/27  <jeremie.salvucci@free.fr>:

> We think that the enum type_kind discriminates fields union in struct type. So for TYPE_PARAM_STRUCT we believe that
> the param_struct field of union u inside struct type is used. If this is true, the test s->u.s.line.file != NULL is meaningless when s->kind == TYPE_PARAM_STRUCT, it should be s->u.param_struct.line.file != NULL instead in our opinion.
>
>
> Or can a type have a kind TYPE_PARAM_STRUCT and only have s->u.s valid? It might be related to the code in new_structure near line  638 of gengtype.c which sets ls->kind = TYPE_LANG_STRUCT.
>
> Perhaps TYPE_PARAM_STRUCT has two different roles. If that is indeed the case, we have to distinguish them when serializing gengtype's state.

I don't have time to investigate this right now to come up with an
answer, but did you try producing gengtype debugging dump and looking
there for structs that have these combinations of properties?
Especially since -

> However, the existing code appears to work but we don't understand why.

Cheers,
-- 
Laurynas

Re: Gengtype : strange code in output_type_enum

[jeremie.salvucci]

"Or can a type have a kind TYPE_PARAM_STRUCT and only have s->u.s valid? It might be related to the code in new_structure near line  638 of gengtype.c which sets ls->kind = TYPE_LANG_STRUCT."

Forget about this sentence, Basile messed up TYPE_PARAM_STRUCT & TYPE_LANG_STRUCT (and is typing this).

Cheers

-- 

Jeremie Salvucci & Basile Starynkevitch

Re: Gengtype : strange code in output_type_enum

[jeremie.salvucci]

We recompiled GCC-trunk r162692 with the following modification :

In function output_type_enum of gcc/gengtype.c, we replaced 

-  if (s->kind == TYPE_PARAM_STRUCT && s->u.s.line.file != NULL)
+  if (s->kind == TYPE_PARAM_STRUCT && s->u.param_struct.line.file != NULL)

And Gengtype works like before with c,c++, lto enabled.

Do you think we have to submit a one line patch (if yes, could it be reviewed quickly)? We don't know why the old version works, and we think writing u.s.line.file is incorrect for TYPE_PARAM_STRUCT (even if it happens to work by luck), since the union u.param_struct member is the only valid for TYPE_PARAM_STRUCT. 



-- 

Jeremie Salvucci & Basile Starynkevitch

Re: Gengtype : strange code in output_type_enum

[Arnaud Charlet]

> In function output_type_enum of gcc/gengtype.c, we replaced 
> 
> -  if (s->kind == TYPE_PARAM_STRUCT && s->u.s.line.file != NULL)
> +  if (s->kind == TYPE_PARAM_STRUCT && s->u.param_struct.line.file !=
> NULL)
> 
> And Gengtype works like before with c,c++, lto enabled.
> 
> Do you think we have to submit a one line patch (if yes, could it be reviewed

Sure, one line patches are actually welcome since they are well isolated and
easy to review, as opposed to large big patches containing unrelated stuff
which have basically zero chance to get accepted/reviewed (other than
"please break you patch into multiple pieces).

Arno

Re: Gengtype : strange code in output_type_enum

[Laurynas Biveinis]

2010/8/27  <jeremie.salvucci@free.fr>:
> We recompiled GCC-trunk r162692 with the following modification :
>
> In function output_type_enum of gcc/gengtype.c, we replaced
>
> -  if (s->kind == TYPE_PARAM_STRUCT && s->u.s.line.file != NULL)
> +  if (s->kind == TYPE_PARAM_STRUCT && s->u.param_struct.line.file != NULL)
>
> And Gengtype works like before with c,c++, lto enabled.
>
> Do you think we have to submit a one line patch (if yes, could it be reviewed quickly)? We don't know why the old version works, and we think writing u.s.line.file is incorrect for TYPE_PARAM_STRUCT (even if it happens to work by luck), since the union u.param_struct member is the only valid for TYPE_PARAM_STRUCT.

One-line patches are welcome, but in this instance could you please
find out how the old code worked before changing it (as you admit, you
don't understand it).

-- 
Laurynas

Re: Gengtype : strange code in output_type_enum

[Ian Lance Taylor]

jeremie.salvucci@free.fr writes:

> While hacking on gengtype with Basile, we noticed a strange piece of code at line 2539 in gcc/gengtype.c r162692
>
> static void
> output_type_enum (outf_p of, type_p s)
> {
>   if (s->kind == TYPE_PARAM_STRUCT && s->u.s.line.file != NULL) /* Strange code @@*/
>     {
>       oprintf (of, ", gt_e_");
>       output_mangled_typename (of, s);
>     }
>   else if (UNION_OR_STRUCT_P (s) && s->u.s.line.file != NULL)
>     {
>       oprintf (of, ", gt_ggc_e_");
>       output_mangled_typename (of, s);
>     }
>   else
>     oprintf (of, ", gt_types_enum_last");
> }
>
> We think that the enum type_kind discriminates fields union in struct type. So for TYPE_PARAM_STRUCT we believe that 
> the param_struct field of union u inside struct type is used. If this is true, the test s->u.s.line.file != NULL is meaningless when s->kind == TYPE_PARAM_STRUCT, it should be s->u.param_struct.line.file != NULL instead in our opinion.

I agree that this is wrong.

> However, the existing code appears to work but we don't understand why.

That one is fairly easy.  If you look at the generated code, you will
see that those values are only used to pass to gt_pch_note_object.  From
there they will eventually be passed to either ggc_pch_count_object or
ggc_pch_alloc_object.  The default page allocator ignores this type.
The zone allocator does use the type, but nobody uses that allocator.
And even if you do use the zone allocator, it will work correctly if
perhaps suboptimally as long as it always gets the same type for a given
struct, which I believe will happen.

You should send in a tested patch to fix that problem (and nothing
else).

Ian

Re: Gengtype : strange code in output_type_enum

[Basile Starynkevitch]

On Fri, 2010-08-27 at 17:25 +0300, Laurynas Biveinis wrote:
> 2010/8/27  <jeremie.salvucci@free.fr>:
> > We recompiled GCC-trunk r162692 with the following modification :
> >
> > In function output_type_enum of gcc/gengtype.c, we replaced
> >
> > -  if (s->kind == TYPE_PARAM_STRUCT && s->u.s.line.file != NULL)
> > +  if (s->kind == TYPE_PARAM_STRUCT && s->u.param_struct.line.file != NULL)
> >
> > And Gengtype works like before with c,c++, lto enabled.
> >
> > Do you think we have to submit a one line patch (if yes, could it be reviewed quickly)? We don't know why the old version works, and we think writing u.s.line.file is incorrect for TYPE_PARAM_STRUCT (even if it happens to work by luck), since the union u.param_struct member is the only valid for TYPE_PARAM_STRUCT.
> 
> One-line patches are welcome, but in this instance could you please
> find out how the old code worked before changing it (as you admit, you
> don't understand it).

My impression is that s->u.s.line.file usually happens to have the same
offset (at least on GNU/Linux/AMD64=x86_64) as
s->u.param_struct.param[0] and that for every type concerned by
output_type_enum  its param[0] subfield happens to be non-null. This
explains that it worked by accident.

Is such an heuristic explanation enough to propose a patch? I am not
sure to be able to provide a better one quickly (so if the explanation
is not enough, I am not sure to want to propose a half-line patch).

By the way, what is the good way to find out exactly what svn commit
introduced the bogus line?

What surprises me much more is that the s->u.s.line.file != NULL test
has been accepted long time ago. From what we understand of gengtype, it
could never have made any sense (because conceptually s->u.s does not
exist for TYPE_PARAM_STRUCT!), even if it happens to work by pure luck.



I am quite surprised (but I admit I only looked a few pages) that there
does not seems to be any rules regarding use of union in C code inside
GNU.  My personal requirement is that a union is only usable if it is
inside a structure and is discriminated by a field of this structure
(the usual case of a union of sub-structures each starting with a
discriminant logically fits that requirement) or by a simple pure
fonction depending of such a field 
(in ML or Ocaml parlance, a union is a discriminated sum type; Also,
rpcxdr from Sun twenty years ago had a similar requirement...). 

But I see no such rules within GCC, and I even saw several unions not
used that way. My perhaps excessive opinion is that such union abuse
always gives unmaintainable code (and I am in the minority which wants
GCC code to be more easily maintainable & readable & hackable by new
contributors, even at the expense of raw performance; I feel that
competitors's free compilers like LLVM are much better in that aspect.).

###



For the curious people, our current work on gengtype is available as
http://starynkevitch.net/Basile/gengtype-r163582-27-august-2010.diff
As usual, this is a temporary URL.  Our patch is not yet ready for
submission.

I have to clean up the code, correct a bug or two, understand how
exactly is the s->u.param_struct.line.file field set in present
gengtype. I also have to split our work into several patches, and I am
very afraid of not being able to make a sequence of small patches such
that each change make still gengtype work for entire GCC!  I have no
idea if this is even doable (since gengtype is a code generator with
*global* side effects on GCC code; it could happen that some partial
change work for C but not C++ or Ada parts.).



I will perhaps propose a few *related* patches on gengtype this
week-end, if I am motivated enough to work on it.



Cheers.
-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mine, sont seulement les miennes} ***

Re: Gengtype : strange code in output_type_enum

[Ian Lance Taylor]

Basile Starynkevitch <basile@starynkevitch.net> writes:

> My impression is that s->u.s.line.file usually happens to have the same
> offset (at least on GNU/Linux/AMD64=x86_64) as
> s->u.param_struct.param[0] and that for every type concerned by
> output_type_enum  its param[0] subfield happens to be non-null. This
> explains that it worked by accident.

No, that is not the case.  But I already explained why this error
doesn't matter:
    http://gcc.gnu.org/ml/gcc/2010-08/msg00396.html

> By the way, what is the good way to find out exactly what svn commit
> introduced the bogus line?

svn blame

Ian