Message ID | 20110607184432.396AB1C3747@gchare.mtv.corp.google.com |
---|---|
State | New |
Headers | show |
On Tue, Jun 7, 2011 at 8:44 PM, Gabriel Charette <gchare@google.com> wrote:
> We need to stream TREE_TYPE for identifier node.
That seems unlikely, as identifiers do not have a type. There is some
TREE_TYPE abuse in cp-tree.h, perhaps you should find out what you're
streaming.
Why are you not using accessor macros for the other fields of
lang_identifier, e.g. not id->label_value but
IDENTIFIER_LABEL_VALUE(id)?
Ciao!
Steven
On Tue, Jun 7, 2011 at 14:12, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > On Tue, Jun 7, 2011 at 8:44 PM, Gabriel Charette <gchare@google.com> wrote: >> We need to stream TREE_TYPE for identifier node. > > That seems unlikely, as identifiers do not have a type. There is some > TREE_TYPE abuse in cp-tree.h, perhaps you should find out what you're > streaming. It's used by the C++ parser, so it needs to be streamed in pph. > Why are you not using accessor macros for the other fields of > lang_identifier, e.g. not id->label_value but > IDENTIFIER_LABEL_VALUE(id)? Yes, this needs to be fixed. Diego.
On Tue, Jun 7, 2011 at 11:50 PM, Diego Novillo <dnovillo@google.com> wrote: > On Tue, Jun 7, 2011 at 14:12, Steven Bosscher <stevenb.gcc@gmail.com> wrote: >> On Tue, Jun 7, 2011 at 8:44 PM, Gabriel Charette <gchare@google.com> wrote: >>> We need to stream TREE_TYPE for identifier node. >> >> That seems unlikely, as identifiers do not have a type. There is some >> TREE_TYPE abuse in cp-tree.h, perhaps you should find out what you're >> streaming. > > It's used by the C++ parser, so it needs to be streamed in pph. Yes, but you should not stream TREE_TYPE but use the accessor macro that uses TREE_TYPE. Otherwise, if someone gets around to making IDENTIFIER nodes non-typed trees (and update cp-tree.h accordingly) the streaming will break. Ciao! Steven
On Wed, Jun 8, 2011 at 03:38, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > On Tue, Jun 7, 2011 at 11:50 PM, Diego Novillo <dnovillo@google.com> wrote: >> On Tue, Jun 7, 2011 at 14:12, Steven Bosscher <stevenb.gcc@gmail.com> wrote: >>> On Tue, Jun 7, 2011 at 8:44 PM, Gabriel Charette <gchare@google.com> wrote: >>>> We need to stream TREE_TYPE for identifier node. >>> >>> That seems unlikely, as identifiers do not have a type. There is some >>> TREE_TYPE abuse in cp-tree.h, perhaps you should find out what you're >>> streaming. >> >> It's used by the C++ parser, so it needs to be streamed in pph. > > Yes, but you should not stream TREE_TYPE but use the accessor macro > that uses TREE_TYPE. Otherwise, if someone gets around to making > IDENTIFIER nodes non-typed trees (and update cp-tree.h accordingly) > the streaming will break. It does, actually: cp/rtti.c: bool emit_tinfo_decl (tree decl) { tree type = TREE_TYPE (DECL_NAME (decl)); Diego.
On Wed, Jun 8, 2011 at 4:12 PM, Diego Novillo wrote: >>>> That seems unlikely, as identifiers do not have a type. There is some >>>> TREE_TYPE abuse in cp-tree.h, perhaps you should find out what you're >>>> streaming. >>> >>> It's used by the C++ parser, so it needs to be streamed in pph. >> >> Yes, but you should not stream TREE_TYPE but use the accessor macro >> that uses TREE_TYPE. Otherwise, if someone gets around to making >> IDENTIFIER nodes non-typed trees (and update cp-tree.h accordingly) >> the streaming will break. > > It does, actually: > > cp/rtti.c: > bool > emit_tinfo_decl (tree decl) > { > tree type = TREE_TYPE (DECL_NAME (decl)); Well, I wasn't saying that TREE_TYPE isn't used, just that it is not actually TREE_TYPE. Not very thorough, but consider this: stevenb@gcc17:~/devel/trunk/gcc$ egrep "TREE_TYPE.*DECL_NAME" *.[ch] {fortran,java,ada/gcc-interface,cp,objc}/*.[ch] fortran/f95-lang.c: TYPE_NAME (TREE_TYPE (decl)) = DECL_NAME (decl); cp/cp-tree.h: (DECL_CONV_FN_P (FN) ? TREE_TYPE (DECL_NAME (FN)) : NULL_TREE) cp/decl2.c: tree underlying_type = TREE_TYPE (DECL_NAME (decl)); cp/decl2.c: (decl, type_visibility (TREE_TYPE (DECL_NAME (decl)))); cp/decl2.c: && CLASS_TYPE_P (TREE_TYPE (DECL_NAME (decl))) cp/decl2.c: && !CLASSTYPE_VISIBILITY_SPECIFIED (TREE_TYPE (DECL_NAME (decl)))) cp/decl2.c: tree type = TREE_TYPE (DECL_NAME (decl)); cp/decl.c: if (TREE_TYPE (DECL_NAME (decl)) && TREE_TYPE (decl) != type) cp/repo.c: type = TREE_TYPE (DECL_NAME (decl)); cp/rtti.c: tree type = TREE_TYPE (DECL_NAME (decl)); The one in fortran looks like a mistake (this is in pushdecl which was copy-and-pasted long ago from treelang or something). The documentation in doc/generic.text is pretty clear about it: TYPE_NAME of a TYPE_DECL is not an IDENTIFIER node. There should probably be a checker for that, some kind of negative tree code check perhaps... The rest are all in cp/. It looks like g++ uses TREE_TYPE as a cache for name lookups. Perhaps Jason can comment. Obviously not a front end I know very well, but let's look at them one at a time: cp/cp-tree.h: (DECL_CONV_FN_P (FN) ? TREE_TYPE (DECL_NAME (FN)) : NULL_TREE) Apparently g++ puts the type of an operator in TREE_TYPE of an IDENTIFIER_NODE. This should probably be using REAL_IDENTIFIER_TYPE_VALUE() instead of TREE_TYPE(). cp/decl.c: if (TREE_TYPE (DECL_NAME (decl)) && TREE_TYPE (decl) != type) This is in a warning for a type declaration shadowing a local or class scope. Should use identifier_type_value (or REAL_IDENTIFIER_TYPE_VALUE but I think that's supposed to be used directly only in name-lookup.c??) cp/decl2.c: tree underlying_type = TREE_TYPE (DECL_NAME (decl)); cp/decl2.c: (decl, type_visibility (TREE_TYPE (DECL_NAME (decl)))); cp/decl2.c: && CLASS_TYPE_P (TREE_TYPE (DECL_NAME (decl))) cp/decl2.c: && !CLASSTYPE_VISIBILITY_SPECIFIED (TREE_TYPE (DECL_NAME (decl)))) cp/decl2.c: tree type = TREE_TYPE (DECL_NAME (decl)); cp/repo.c: type = TREE_TYPE (DECL_NAME (decl)); cp/rtti.c: tree type = TREE_TYPE (DECL_NAME (decl)); All of these are covered by a check on DECL_TINFO_P. I am not sure what that means but probably these should also be using identifier_type_value or REAL_IDENTIFIER_TYPE_VALUE instead of TREE_TYPE. Jason? Anyway, it seems that there are already a lot of places where TREE_TYPE is used instead of a separate accessor macro for this overloaded meaning of TREE_TYPE on IDENTIFIER_NODEs. That is no reason to further confuse things with pph. It seems to me that in the long run tree_identifier could (should) be made a non-tree_typed tree... Would you be willing to try if it is sufficient to only stream REAL_IDENTIFIER_TYPE_VALUE? Ciao! Steven
On 06/08/2011 03:31 PM, Steven Bosscher wrote: > The rest are all in cp/. It looks like g++ uses TREE_TYPE as a cache > for name lookups. Perhaps Jason can comment. Obviously not a front end > I know very well, but let's look at them one at a time: > > cp/cp-tree.h: (DECL_CONV_FN_P (FN) ? TREE_TYPE (DECL_NAME (FN)) : NULL_TREE) > > Apparently g++ puts the type of an operator in TREE_TYPE of an > IDENTIFIER_NODE. This should probably be using > REAL_IDENTIFIER_TYPE_VALUE() instead of TREE_TYPE(). Yes, this is to associate the name of a type conversion operator with the type it converts to. Using a different macro would be fine. > cp/decl.c: if (TREE_TYPE (DECL_NAME (decl))&& TREE_TYPE (decl) != type) > > This is in a warning for a type declaration shadowing a local or class > scope. Should use identifier_type_value (or > REAL_IDENTIFIER_TYPE_VALUE but I think that's supposed to be used > directly only in name-lookup.c??) Sure, identifier_type_value would work. But that code looks bitrotted; type is always equal to TREE_TYPE (decl). I'd be inclined to try removing it and seeing if anything breaks. > cp/decl2.c: tree underlying_type = TREE_TYPE (DECL_NAME (decl)); > cp/decl2.c: (decl, type_visibility (TREE_TYPE (DECL_NAME (decl)))); > cp/decl2.c:&& CLASS_TYPE_P (TREE_TYPE (DECL_NAME (decl))) > cp/decl2.c:&& !CLASSTYPE_VISIBILITY_SPECIFIED (TREE_TYPE > (DECL_NAME (decl)))) > cp/decl2.c: tree type = TREE_TYPE (DECL_NAME (decl)); > cp/repo.c: type = TREE_TYPE (DECL_NAME (decl)); > cp/rtti.c: tree type = TREE_TYPE (DECL_NAME (decl)); > > All of these are covered by a check on DECL_TINFO_P. I am not sure > what that means but probably these should also be using > identifier_type_value or REAL_IDENTIFIER_TYPE_VALUE instead of > TREE_TYPE. Jason? This is a way of finding the class a type_info node/vtable pertains to. Using a different lookup strategy would be fine. Jason
Index: gcc/cp/pph-streamer-in.c =================================================================== --- gcc/cp/pph-streamer-in.c (revision 174760) +++ gcc/cp/pph-streamer-in.c (working copy) @@ -1027,6 +1027,7 @@ id->bindings = pph_in_cxx_binding (stream); id->class_template_info = pph_in_tree (stream); id->label_value = pph_in_tree (stream); + TREE_TYPE (expr) = pph_in_tree (stream); } break; Index: gcc/cp/pph-streamer-out.c =================================================================== --- gcc/cp/pph-streamer-out.c (revision 174760) +++ gcc/cp/pph-streamer-out.c (working copy) @@ -983,6 +983,7 @@ pph_out_cxx_binding (stream, id->bindings, ref_p); pph_out_tree_or_ref_1 (stream, id->class_template_info, ref_p, 3); pph_out_tree_or_ref_1 (stream, id->label_value, ref_p, 3); + pph_out_tree_or_ref_1 (stream, TREE_TYPE (expr), ref_p, 3); } break; Index: gcc/testsuite/g++.dg/pph/x1functions.cc =================================================================== --- gcc/testsuite/g++.dg/pph/x1functions.cc (revision 174760) +++ gcc/testsuite/g++.dg/pph/x1functions.cc (working copy) @@ -1,6 +1,5 @@ -// { dg-xfail-if "ICE" { "*-*-*" } { "-fpph-map=pph.map" } } +// { dg-xfail-if "ERROR" { "*-*-*" } { "-fpph-map=pph.map" } } // { dg-bogus "'mbr_decl_inline' was not declared in this scope" "" { xfail *-*-* } 0 } -// { dg-bogus "c1functions.h:8:34: internal compiler error: Segmentation fault" "" { xfail *-*-* } 0 } // { dg-prune-output "In file included from " } // { dg-prune-output "In member function " } // { dg-prune-output "At global scope:" }