* Gengtype : strange code in output_type_enum @ 2010-08-27 12:49 jeremie.salvucci 2010-08-27 13:40 ` Laurynas Biveinis ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: jeremie.salvucci @ 2010-08-27 12:49 UTC (permalink / raw) To: gcc; +Cc: laurynas, basile 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Gengtype : strange code in output_type_enum 2010-08-27 12:49 Gengtype : strange code in output_type_enum jeremie.salvucci @ 2010-08-27 13:40 ` Laurynas Biveinis 2010-08-27 13:44 ` jeremie.salvucci 2010-08-27 15:02 ` Ian Lance Taylor 2 siblings, 0 replies; 9+ messages in thread From: Laurynas Biveinis @ 2010-08-27 13:40 UTC (permalink / raw) To: jeremie.salvucci; +Cc: gcc, basile 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Gengtype : strange code in output_type_enum 2010-08-27 12:49 Gengtype : strange code in output_type_enum jeremie.salvucci 2010-08-27 13:40 ` Laurynas Biveinis @ 2010-08-27 13:44 ` jeremie.salvucci 2010-08-27 14:25 ` jeremie.salvucci 2010-08-27 15:02 ` Ian Lance Taylor 2 siblings, 1 reply; 9+ messages in thread From: jeremie.salvucci @ 2010-08-27 13:44 UTC (permalink / raw) To: jeremie salvucci; +Cc: gcc, laurynas, basile "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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Gengtype : strange code in output_type_enum 2010-08-27 13:44 ` jeremie.salvucci @ 2010-08-27 14:25 ` jeremie.salvucci 2010-08-27 14:43 ` Arnaud Charlet 2010-08-27 14:47 ` Laurynas Biveinis 0 siblings, 2 replies; 9+ messages in thread From: jeremie.salvucci @ 2010-08-27 14:25 UTC (permalink / raw) To: jeremie salvucci; +Cc: gcc, laurynas, basile 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Gengtype : strange code in output_type_enum 2010-08-27 14:25 ` jeremie.salvucci @ 2010-08-27 14:43 ` Arnaud Charlet 2010-08-27 14:47 ` Laurynas Biveinis 1 sibling, 0 replies; 9+ messages in thread From: Arnaud Charlet @ 2010-08-27 14:43 UTC (permalink / raw) To: jeremie.salvucci; +Cc: gcc, laurynas, basile > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Gengtype : strange code in output_type_enum 2010-08-27 14:25 ` jeremie.salvucci 2010-08-27 14:43 ` Arnaud Charlet @ 2010-08-27 14:47 ` Laurynas Biveinis 2010-08-27 18:25 ` Basile Starynkevitch 1 sibling, 1 reply; 9+ messages in thread From: Laurynas Biveinis @ 2010-08-27 14:47 UTC (permalink / raw) To: jeremie.salvucci; +Cc: gcc, basile 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Gengtype : strange code in output_type_enum 2010-08-27 14:47 ` Laurynas Biveinis @ 2010-08-27 18:25 ` Basile Starynkevitch 2010-08-27 18:29 ` Ian Lance Taylor 0 siblings, 1 reply; 9+ messages in thread From: Basile Starynkevitch @ 2010-08-27 18:25 UTC (permalink / raw) To: Laurynas Biveinis; +Cc: jeremie.salvucci, gcc 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} *** ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Gengtype : strange code in output_type_enum 2010-08-27 18:25 ` Basile Starynkevitch @ 2010-08-27 18:29 ` Ian Lance Taylor 0 siblings, 0 replies; 9+ messages in thread From: Ian Lance Taylor @ 2010-08-27 18:29 UTC (permalink / raw) To: basile; +Cc: Laurynas Biveinis, jeremie.salvucci, gcc 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Gengtype : strange code in output_type_enum 2010-08-27 12:49 Gengtype : strange code in output_type_enum jeremie.salvucci 2010-08-27 13:40 ` Laurynas Biveinis 2010-08-27 13:44 ` jeremie.salvucci @ 2010-08-27 15:02 ` Ian Lance Taylor 2 siblings, 0 replies; 9+ messages in thread From: Ian Lance Taylor @ 2010-08-27 15:02 UTC (permalink / raw) To: jeremie.salvucci; +Cc: gcc, laurynas, basile 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-08-27 18:16 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-08-27 12:49 Gengtype : strange code in output_type_enum jeremie.salvucci 2010-08-27 13:40 ` Laurynas Biveinis 2010-08-27 13:44 ` jeremie.salvucci 2010-08-27 14:25 ` jeremie.salvucci 2010-08-27 14:43 ` Arnaud Charlet 2010-08-27 14:47 ` Laurynas Biveinis 2010-08-27 18:25 ` Basile Starynkevitch 2010-08-27 18:29 ` Ian Lance Taylor 2010-08-27 15:02 ` Ian Lance Taylor
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).