diff mbox

[pph] Stream TREE_TYPE for identifier node (issue4550121)

Message ID 20110607184432.396AB1C3747@gchare.mtv.corp.google.com
State New
Headers show

Commit Message

Gab Charette June 7, 2011, 6:44 p.m. UTC 
We need to stream TREE_TYPE for identifier node.

This fixes some ICEs, but introduces some new assembly mismatch errors.

Here is the testing diff:
47,49d46
< XPASS: g++.dg/pph/x1autometh.cc  -fpph-map=pph.map -I.  (test for bogus messages, line )
< XPASS: g++.dg/pph/x1autometh.cc  -fpph-map=pph.map -I. (test for excess errors)
< FAIL: g++.dg/pph/x1autometh.cc  (assembly mismatch)
51c48
< XPASS: g++.dg/pph/x1functions.cc  -fpph-map=pph.map -I. (test for excess errors)
---
> XPASS: g++.dg/pph/x1functions.cc  -I.  (test for bogus messages, line )
53,54d49
< XPASS: g++.dg/pph/x1special.cc  -fpph-map=pph.map -I.  (test for bogus messages, line )
< XPASS: g++.dg/pph/x1special.cc  -fpph-map=pph.map -I. (test for excess errors)
62d56
< XPASS: g++.dg/pph/x1typerefs.cc  -fpph-map=pph.map -I.  (test for bogus messages, line )
70,74c64,68
< # of expected passes		174
< # of unexpected failures	2
< # of unexpected successes	21
< # of expected failures		37
< /usr/local/google/gchare/gcc/dbg/gcc/testsuite/g++/../../g++  version 4.7.0-pph 20110606 (experimental) (GCC) 
---
> # of expected passes		177
> # of unexpected failures	1
> # of unexpected successes	16
> # of expected failures		46
> /usr/local/google/gchare/gcc-clean/bld/gcc/testsuite/g++/../../g++  version 4.7.0-pph 20110606 (experimental) (GCC)

2011-06-07  Gabriel Charette  <gchare@google.com>

	* gcc/cp/pph-streamer-in.c (pph_read_tree): 
	Read TREE_TYPE from id_node.
	* gcc/cp/pph-streamer-out.c (pph_write_tree):
	Write TREE_TYPE from id_node.
	* gcc/testsuite/g++.dg/pph/x1functions.cc (dg-xfail-if "ICE"): Remove.
	(dg-xfail-if "ERROR"): Add.


--
This patch is available for review at http://codereview.appspot.com/4550121

Comments

Steven Bosscher June 7, 2011, 9:12 p.m. UTC | #1 
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
Diego Novillo June 7, 2011, 9:50 p.m. UTC | #2 
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.
Steven Bosscher June 8, 2011, 10:38 a.m. UTC | #3 
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
Diego Novillo June 8, 2011, 2:12 p.m. UTC | #4 
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.
Steven Bosscher June 8, 2011, 7:31 p.m. UTC | #5 
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
Jason Merrill June 9, 2011, 2:29 a.m. UTC | #6 
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
diff mbox

Patch

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:" }