Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(222)

Issue 5846054: [pph] Tagless Types; Macro Redefines (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 7 months ago by Lawrence Crowl
Modified:
5 years, 4 months ago
Reviewers:
Diego Novillo
CC:
gcc-patches_gcc.gnu.org
Base URL:
svn+ssh://gcc.gnu.org/svn/gcc/branches/pph/
Visibility:
Public.

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+145 lines, -43 lines) Patch
M gcc/cp/class.c View 1 chunk +2 lines, -1 line 0 comments Download
M gcc/cp/cp-tree.h View 1 chunk +1 line, -1 line 0 comments Download
M gcc/cp/decl.c View 1 chunk +2 lines, -1 line 0 comments Download
M gcc/cp/error.c View 1 chunk +1 line, -1 line 0 comments Download
M gcc/cp/name-lookup.c View 2 chunks +43 lines, -3 lines 0 comments Download
M gcc/cp/parser.c View 3 chunks +17 lines, -6 lines 0 comments Download
M gcc/cp/semantics.c View 2 chunks +2 lines, -2 lines 0 comments Download
M gcc/testsuite/g++.dg/pph/README View 1 chunk +4 lines, -3 lines 0 comments Download
M gcc/testsuite/g++.dg/pph/c1anonymous1.h View 1 chunk +1 line, -4 lines 0 comments Download
M gcc/testsuite/g++.dg/pph/c1anonymous2.h View 1 chunk +2 lines, -5 lines 0 comments Download
M gcc/testsuite/g++.dg/pph/c7features.cc View 1 chunk +0 lines, -5 lines 0 comments Download
A gcc/testsuite/g++.dg/pph/p0anonretag.h View 1 chunk +19 lines, -0 lines 0 comments Download
A gcc/testsuite/g++.dg/pph/p1anonretag.cc View 1 chunk +11 lines, -0 lines 0 comments Download
M gcc/testsuite/g++.dg/pph/x0tmpldfltparm.h View 1 chunk +2 lines, -2 lines 0 comments Download
M gcc/testsuite/g++.dg/pph/x7rtti.cc View 1 chunk +1 line, -4 lines 0 comments Download
M libcpp/symtab.c View 3 chunks +37 lines, -5 lines 0 comments Download

Messages

Total messages: 1
Lawrence Crowl
5 years, 7 months ago (2012-03-17 02:08:17 UTC) #1
This patch addresses two main problems.

(1) We were mishandling tagless types.  The root of the problem was that
tagless types were given tags based on a sequence number.  Two different PPH
files would thus have the same manufactured tag name for different types,
leading to inappropriate collisions in lookup.  The solution is to manufacture
the tag from the source location rather than from a sequence number.  All of
this construction must match the pattern when the name is later strcmp'ed to
see if it is actually tagless.

Tests c1anonymous1.h and c1anonymous2.h are now passing.  Added tests
p0anonretag.h and p1anonretag.cc to ensure that the pattern matcher works.

(2) We were getting macro redefinition errors when two PPH files textually
included the same #defines because each PPH file replays its definitions.
The applied fix is to not do idempotent defines.

Test c7features.cc is now passing.  Test x7rtti.cc does not have as many
errors. 

This patch also addresses a minor problem.

(3) Debugging test x0tmpldfltparm.h was difficult because the same template
parameter identifier was used in different places.  Instead, use unique names.

(4) Some debug/dump/print routines were missing null pointer checks.  Add them.

Tested on x64.


Index: gcc/testsuite/ChangeLog.pph

2012-03-16   Lawrence Crowl  <crowl@google.com>

	* g++.dg/pph/README: Clarify p category.
	* g++.dg/pph/c1anonymous1.h: Mark passing.
	* g++.dg/pph/c1anonymous2.h: Mark passing.
	* g++.dg/pph/c7features.cc: Mark passing.
	* g++.dg/pph/p0anonretag.h: New.
	* g++.dg/pph/p1anonretag.cc: New.
	* g++.dg/pph/x0tmpldfltparm.h: Disambiguate template param names.
	* g++.dg/pph/x7rtti.cc: Remove some xfails.

Index: gcc/cp/ChangeLog.pph

2012-03-16   Lawrence Crowl  <crowl@google.com>

	* cp-tree.h (make_anon_name): Add location parameter.
	* name-lookup.c (static const char anon_char): New.
	(static const char anon_prefix): New.
	(edit_anon_name): New.
	(make_anon_name): Add location parameter.  Use location in creating
	names for anonymous (tagless) types when using PPH.
	(make_lambda_name): Fix comment for later.
	* class.c (layout_class_type): Pass location to make_anon_name.
	* decl.c (start_enum): Pass location to make_anon_name.
	* semantics.c (finish_compound_literal): Pass location to
	make_anon_name.
	(begin_class_definition): Pass location to make_anon_name.
	* error.c (dump_type): Protect call against null pointer.
	* parser.c (cp_lexer_print_token): Protect against null values.
	(cp_parser_enum_specifier): Pass location to make_anon_name.
	(cp_parser_class_head): Pass location to make_anon_name.

Index: libcpp/ChangeLog.pph

2012-03-16   Lawrence Crowl  <crowl@google.com>

	* symtab.c (cpp_lt_already_done): New.
	(cpp_lt_replay): Avoid idempotent macro defines.


Index: gcc/testsuite/g++.dg/pph/c1anonymous1.h
===================================================================
--- gcc/testsuite/g++.dg/pph/c1anonymous1.h	(revision 185481)
+++ gcc/testsuite/g++.dg/pph/c1anonymous1.h	(working copy)
@@ -1,11 +1,8 @@
-// {    xfail-if "ANONYMOUS MERGING" { "*-*-*" } { "-fpph-map=pph.map" } }
-// { dg-bogus "c0anonymous.h:4:16: error: 'anon_t' has a previous declaration
here" "" { xfail *-*-* } 0 }
-
 #ifndef	C1ANONYMOUS
 #define	C1ANONYMOUS
 
 #include "c0anonymous.h"
 
-enum { first, second }; // { dg-bogus "'anon_t' referred to as enum" "" { xfail
*-*-* } }
+enum { first, second };
 
 #endif
Index: gcc/testsuite/g++.dg/pph/c1anonymous2.h
===================================================================
--- gcc/testsuite/g++.dg/pph/c1anonymous2.h	(revision 185481)
+++ gcc/testsuite/g++.dg/pph/c1anonymous2.h	(working copy)
@@ -1,14 +1,11 @@
-// {    xfail-if "ANONYMOUS MERGING" { "*-*-*" } { "-fpph-map=pph.map" } }
-// { dg-bogus "c0anonymous.h:4:16: error: 'struct anon_t' has a previous
declaration as 'struct anon_t'" "" { xfail *-*-* } 0 }
-
 #ifndef C1ANONYMOUS2_H
 #define C1ANONYMOUS2_H
 
 #include "c0anonymous.h"
 
-typedef struct { // { dg-bogus "conflicting declaration 'struct<anonymous>'" ""
{ xfail *-*-* } }
+typedef struct {
 
     char *field;
-} anon2_t; // { dg-bogus "invalid type in declaration before ';' token" "" {
xfail *-*-* } }
+} anon2_t;
 
 #endif
Index: gcc/testsuite/g++.dg/pph/README
===================================================================
--- gcc/testsuite/g++.dg/pph/README	(revision 185481)
+++ gcc/testsuite/g++.dg/pph/README	(working copy)
@@ -7,9 +7,10 @@ names.
     c - positive tests for C-level headers and sources
     d - negative tests for C-level headers and sources
     e - C-level tests for non-sharable headers
-    p - positive tests for what would be c-level code, but which
-        due to C++ standard namespace games are not quite C level
-        tests
+    p - positive tests for what would be c-level code, but which due to
+	  C++ standard namespace games and
+	  implicit tagging from typedefs
+	are not quite C level tests
     x - C++-level positive tests
     y - C++-level negative tests
     z - C++-level tests for non-sharable headers
Index: gcc/testsuite/g++.dg/pph/x0tmpldfltparm.h
===================================================================
--- gcc/testsuite/g++.dg/pph/x0tmpldfltparm.h	(revision 185481)
+++ gcc/testsuite/g++.dg/pph/x0tmpldfltparm.h	(working copy)
@@ -1,12 +1,12 @@
 #ifndef X0TMPLDFLTPARM_H
 #define X0TMPLDFLTPARM_H
 
-template< typename T >
+template< typename A >
 struct auxillary
 {
 };
 
-template< typename T, typename U = auxillary< T > >
+template< typename P, typename Q = auxillary< P > >
 struct primary
 {
 };
Index: gcc/testsuite/g++.dg/pph/x7rtti.cc
===================================================================
--- gcc/testsuite/g++.dg/pph/x7rtti.cc	(revision 185481)
+++ gcc/testsuite/g++.dg/pph/x7rtti.cc	(working copy)
@@ -1,8 +1,5 @@
 // FIXME pph: This should be a { dg=do run } (with '=' replaced by '-')
-// { xfail-if "UNKNOWN MACRO AND BOGUS RTTI" { "*-*-*" } { "-fpph-map=pph.map"
} }
-// { dg-bogus "warning: .__STDC_IEC_559_COMPLEX__. redefined" "" { xfail *-*-*
} 0 }
-// { dg-bogus "warning: .__STDC_ISO_10646__. redefined" "" { xfail *-*-* } 0 }
-// { dg-bogus "warning: .__STDC_IEC_559__. redefined" "" { xfail *-*-* } 0 }
+// { xfail-if "BOGUS RTTI" { "*-*-*" } { "-fpph-map=pph.map" } }
 
 #include "x5rtti1.h"
 #include "x5rtti2.h"
Index: gcc/testsuite/g++.dg/pph/c7features.cc
===================================================================
--- gcc/testsuite/g++.dg/pph/c7features.cc	(revision 185481)
+++ gcc/testsuite/g++.dg/pph/c7features.cc	(working copy)
@@ -1,7 +1,2 @@
-// {    xfail-if "UNKNOWN" { "*-*-*" } { "-fpph-map=pph.map" } }
-// { dg-bogus "warning: .__STDC_IEC_559_COMPLEX__. redefined" "" { xfail *-*-*
} 0 }
-// { dg-bogus "warning: .__STDC_ISO_10646__. redefined" "" { xfail *-*-* } 0 }
-// { dg-bogus "warning: .__STDC_IEC_559__. redefined" "" { xfail *-*-* } 0 }
-
 #include "c5features1.h"
 #include "c5features2.h"
Index: gcc/testsuite/g++.dg/pph/p1anonretag.cc
===================================================================
--- gcc/testsuite/g++.dg/pph/p1anonretag.cc	(revision 0)
+++ gcc/testsuite/g++.dg/pph/p1anonretag.cc	(revision 0)
@@ -0,0 +1,11 @@
+#include "p0anonretag.h"
+
+int F1 (S1 *p)
+{
+  return p != 0;
+}
+
+int F2 (S2 *p)
+{
+  return p != 0;
+}
Index: gcc/testsuite/g++.dg/pph/p0anonretag.h
===================================================================
--- gcc/testsuite/g++.dg/pph/p0anonretag.h	(revision 0)
+++ gcc/testsuite/g++.dg/pph/p0anonretag.h	(revision 0)
@@ -0,0 +1,19 @@
+#ifndef P0ANONRETAG_H
+#define P0ANONRETAG_H
+
+typedef struct S1
+{
+  unsigned long s1;
+  struct S1 *s2;
+  char *s3;
+} S1;
+
+typedef struct
+{
+  unsigned int s4;
+  unsigned int s5;
+  int s6;
+  unsigned int *s7;
+} S2;
+
+#endif
Index: gcc/cp/class.c
===================================================================
--- gcc/cp/class.c	(revision 185481)
+++ gcc/cp/class.c	(working copy)
@@ -5640,8 +5640,9 @@ layout_class_type (tree t, tree *virtual
 	     temporarily give the field a name.  */
 	  if (PCC_BITFIELD_TYPE_MATTERS && !DECL_NAME (field))
 	    {
+	      location_t loc = DECL_SOURCE_LOCATION (field);
 	      was_unnamed_p = true;
-	      DECL_NAME (field) = make_anon_name ();
+	      DECL_NAME (field) = make_anon_name (loc);
 	    }
 #endif
 	  DECL_SIZE (field) = TYPE_SIZE (integer_type);
Index: gcc/cp/decl.c
===================================================================
--- gcc/cp/decl.c	(revision 185481)
+++ gcc/cp/decl.c	(working copy)
@@ -12093,7 +12093,8 @@ start_enum (tree name, tree enumtype, tr
 	 continue.  */
       if (enumtype == error_mark_node)
 	{
-	  name = make_anon_name ();
+	  location_t loc = DECL_SOURCE_LOCATION (TYPE_MAIN_DECL (enumtype));
+	  name = make_anon_name (loc);
 	  enumtype = NULL_TREE;
 	}
 
Index: gcc/cp/error.c
===================================================================
--- gcc/cp/error.c	(revision 185481)
+++ gcc/cp/error.c	(working copy)
@@ -485,10 +485,10 @@ dump_type (tree t, int flags)
       {
 	tree decl = TYPE_NAME (t);
 	pp_cxx_cv_qualifier_seq (cxx_pp, t);
-	dump_location_qualifier (decl, flags);
 	if (decl)
 	  {
 	    tree ident = DECL_NAME (decl);
+	    dump_location_qualifier (decl, flags);
 	    if (ident)
 	      pp_cxx_tree_identifier (cxx_pp, ident);
 	    else
Index: gcc/cp/semantics.c
===================================================================
--- gcc/cp/semantics.c	(revision 185481)
+++ gcc/cp/semantics.c	(working copy)
@@ -2416,7 +2416,7 @@ finish_compound_literal (tree type, tree
 	}
       cp_apply_type_quals_to_decl (cp_type_quals (type), decl);
       decl = pushdecl_top_level (decl);
-      DECL_NAME (decl) = make_anon_name ();
+      DECL_NAME (decl) = make_anon_name (DECL_SOURCE_LOCATION (decl));
       SET_DECL_ASSEMBLER_NAME (decl, DECL_NAME (decl));
       return decl;
     }
@@ -2561,7 +2561,7 @@ begin_class_definition (tree t, tree att
   if (t == error_mark_node || ! MAYBE_CLASS_TYPE_P (t))
     {
       t = make_class_type (RECORD_TYPE);
-      pushtag (make_anon_name (), t, /*tag_scope=*/ts_current);
+      pushtag (make_anon_name (input_location), t, /*tag_scope=*/ts_current);
     }
 
   if (TYPE_BEING_DEFINED (t))
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 185481)
+++ gcc/cp/parser.c	(working copy)
@@ -1193,11 +1193,22 @@ cp_lexer_print_token (FILE * stream, cp_
     case CPP_KEYWORD:
       /* Some keywords have a value that is not an IDENTIFIER_NODE.
 	 For example, `struct' is mapped to an INTEGER_CST.  */
-      if (TREE_CODE (token->u.value) != IDENTIFIER_NODE)
-	break;
-      /* else fall through */
+      if (token->u.value)
+	{
+	  if (TREE_CODE (token->u.value) != IDENTIFIER_NODE)
+	    break;
+	  /* else fall through */
+	}
+      else
+	{
+	  fputs ("<nil>", stream);
+	  break;
+	}
     case CPP_NAME:
-      fputs (IDENTIFIER_POINTER (token->u.value), stream);
+      if (token->u.value)
+	fputs (IDENTIFIER_POINTER (token->u.value), stream);
+      else
+	fputs ("<nil>", stream);
       break;
 
     case CPP_STRING:
@@ -14446,7 +14457,7 @@ cp_parser_enum_specifier (cp_parser* par
 	identifier = cp_parser_identifier (parser);
       else
 	{
-	  identifier = make_anon_name ();
+	  identifier = make_anon_name (input_location);
 	  is_anonymous = true;
 	}
     }
@@ -18636,7 +18647,7 @@ cp_parser_class_head (cp_parser* parser,
     {
       /* If the class was unnamed, create a dummy name.  */
       if (!id)
-	id = make_anon_name ();
+	id = make_anon_name (input_location);
       type = xref_tag (class_key, id, /*tag_scope=*/ts_current,
 		       parser->num_template_parameter_lists);
     }
Index: gcc/cp/cp-tree.h
===================================================================
--- gcc/cp/cp-tree.h	(revision 185481)
+++ gcc/cp/cp-tree.h	(working copy)
@@ -5000,7 +5000,7 @@ extern tree pushdecl				(tree);
 extern tree pushdecl_maybe_friend		(tree, bool);
 extern void maybe_push_cleanup_level		(tree);
 extern tree pushtag				(tree, tree, tag_scope);
-extern tree make_anon_name			(void);
+extern tree make_anon_name			(location_t);
 extern tree pushdecl_top_level_maybe_friend	(tree, bool);
 extern tree pushdecl_top_level_and_finish	(tree, tree);
 extern tree check_for_out_of_scope_variable	(tree);
Index: gcc/cp/name-lookup.c
===================================================================
--- gcc/cp/name-lookup.c	(revision 185481)
+++ gcc/cp/name-lookup.c	(working copy)
@@ -1978,15 +1978,54 @@ constructor_name_p (tree name, tree type
 
 static GTY(()) int anon_cnt;
 
+/* Anonymous names may allow a character outside the C++ set.  */
+
+# ifdef JOINER
+static const char anon_char = JOINER;
+static const char anon_prefix[] = { JOINER, '_' };
+# else
+static const char anon_char = '_';
+static const char anon_prefix = ANON_AGGRNAME_PREFIX;
+# endif
+
+/* Edit a BUFFER so that every character
+   is within the assembler identifier set.  */
+
+static void
+edit_anon_name (char *buffer)
+{
+  char value;
+  for (value = *buffer; value != '\0'; value = *++buffer)
+    {
+      if (   ('0' <= value && value <= '9')
+	  || ('A' <= value && value <= 'Z')
+	  || ('a' <= value && value <= 'z')
+	  || (value == '_'))
+	{
+	  /* The character is already within the identifier set.  */
+	  continue;
+	}
+      *buffer = anon_char; /* Change bad character to good one.  */
+    }
+}
+
 /* Return an IDENTIFIER which can be used as a name for
    anonymous structs and unions.  */
 
 tree
-make_anon_name (void)
+make_anon_name (location_t loc)
 {
-  char buf[32];
+  char buf[MAXPATHLEN + 64];
 
-  sprintf (buf, ANON_AGGRNAME_FORMAT, anon_cnt++);
+  if (pph_enabled_p ())
+    {
+      expanded_location xloc = expand_location (loc);
+      sprintf (buf, "%s%s:%d:%d",
+	       anon_prefix, xloc.file, xloc.line, xloc.column);
+      edit_anon_name (buf);
+    }
+  else
+    sprintf (buf, ANON_AGGRNAME_FORMAT, anon_cnt++);
   return get_identifier (buf);
 }
 
@@ -2003,6 +2042,7 @@ make_lambda_name (void)
 {
   char buf[32];
 
+  /* FIXME pph: We will want to tie this name to location for PPH.  */
   sprintf (buf, LAMBDANAME_FORMAT, lambda_cnt++);
   return get_identifier (buf);
 }
Index: libcpp/symtab.c
===================================================================
--- libcpp/symtab.c	(revision 185481)
+++ libcpp/symtab.c	(working copy)
@@ -817,6 +817,30 @@ cpp_lt_define_syntax (char *needed, cons
   *needed++ = '\0';
 }
 
+
+/* Return true if a replay in the READER for a definition of the macro IDENT
+   to a WANTED value has already been done.  */
+
+static bool
+cpp_lt_define_done (cpp_reader *reader,
+		    const char *ident_str, unsigned int ident_len,
+		    const char *wanted_str)
+{
+  hashnode node;
+  cpp_hashnode *cpp_node;
+  const char *existing_str;
+  const unsigned char *ident_alt = (const unsigned char *)ident_str;
+  node = ht_lookup (reader->hash_table, ident_alt, ident_len, HT_NO_INSERT);
+  if (!node)
+    return false;
+  cpp_node = CPP_HASHNODE (node);
+  if (cpp_node->type != NT_MACRO)
+    return false;
+  existing_str = lt_query_macro (reader, cpp_node);
+  return strcmp (wanted_str, existing_str) == 0;
+}
+
+
 /* Replay the macro definitions captured by the table of IDENTIFIERS
    into the READER state.  If LOC is non-null, assign *LOC as the
    source_location to all macro definitions replayed.  */
@@ -855,8 +879,12 @@ cpp_lt_replay (cpp_reader *reader, cpp_i
         {
           if (after_str != NULL)
             {
-              cpp_lt_define_syntax (buffer, ident_str, after_str);
-              cpp_define (reader, buffer);
+              if (!cpp_lt_define_done (reader, ident_str, entry->ident_len,
+				       after_str))
+		{
+		  cpp_lt_define_syntax (buffer, ident_str, after_str);
+		  cpp_define (reader, buffer);
+		}
             }
           /* else consistently not macros */
         }
@@ -868,9 +896,13 @@ cpp_lt_replay (cpp_reader *reader, cpp_i
             }
           else if (strcmp (before_str, after_str) != 0)
             {
-              cpp_undef (reader, ident_str);
-              cpp_lt_define_syntax (buffer, ident_str, after_str);
-              cpp_define (reader, buffer);
+              if (!cpp_lt_define_done (reader, ident_str, entry->ident_len,
+				       after_str))
+		{
+		  cpp_undef (reader, ident_str);
+		  cpp_lt_define_syntax (buffer, ident_str, after_str);
+		  cpp_define (reader, buffer);
+		}
             }
           /* else macro with the same definition */
         }

--
This patch is available for review at http://codereview.appspot.com/5846054
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 80a51fa-tainted