[pph] Detect #include outside the global context

[Diego Novillo, 2011-08-25 22:31:02]

To prevent PPH images from depend on the context in which they were
generated, we require that the header file be compiled in isolation
and when included, it should only be included in the global context.
That is, things like

namespace Foo {
    #include "bar.h"
    ...
};

should prevent bar.h from becoming a PPH image, since all the symbols
defined in it would belong to Foo, but when bar.pph was generated,
they belonged to ::

This patch detects the use of PPH images inside nested scopes like
that.  It is a bit crude, but it works.  During lexing, it keeps track
of open and close braces (all kinds, not just { }), so when the
#include command is found, it rejects the image if the nesting level
is positive.

Jason, I added a field to scope_chain.  That seemed the cleaner
approach to keep track of the nesting level.  Is that a good place to
keep track of it?  This is the kind of bookkeeping that scope_chain
seems to be used for, but I can put it elsewhere if you want.

This error flagged the usage of sys/types.pph.  This file is included
inside 'extern "C" { }', so strictly speaking it should be rejected.
We can relax these restrictions later.


Tested on x86_64.  Applied to branch.


	* cp-tree.h (struct saved_scope): Add field x_brace_nesting.
	* parser.c (cp_lexer_token_is_open_brace): New.
	(cp_lexer_token_is_close_brace): New.
	(cp_lexer_get_preprocessor_token): Call them.
	Increase scope_chain->x_brace_nesting for open braces, decrease
	for closing braces.
	* pph.c (pph_is_valid_here): New.  Return false if
	scope_chain->x_brace_nesting is greater than 0.
	(pph_include_handler): Call it.


testsuite/ChangeLog.pph

	* g++.dg/pph/pph.exp: Do not create a PPH image for sys/types.h.
	* g++.dg/pph/y8inc-nmspc.cc: Mark fixed.

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 8fb2ccc..e0b67c6 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -973,6 +973,10 @@ struct GTY(()) saved_scope {
   cp_binding_level *bindings;
 
   struct saved_scope *prev;
+
+  /* Used during lexing to validate where PPH images are included, it
+     keeps track of nested bracing.  */
+  unsigned x_brace_nesting;
 };
 
 /* The current open namespace.  */
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 009922e..919671c 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -748,6 +748,31 @@ cp_lexer_saving_tokens (const cp_lexer* lexer)
   return VEC_length (cp_token_position, lexer->saved_tokens) != 0;
 }
 
+
+/* Return true if TOKEN is one of CPP_OPEN_SQUARE, CPP_OPEN_BRACE or
+   CPP_OPEN_PAREN.  */
+
+static inline bool
+cp_lexer_token_is_open_brace (cp_token *token)
+{
+  return token->type == CPP_OPEN_SQUARE
+	 || token->type == CPP_OPEN_BRACE
+	 || token->type == CPP_OPEN_PAREN;
+}
+
+
+/* Return true if TOKEN is one of CPP_CLOSE_SQUARE, CPP_CLOSE_BRACE or
+   CPP_CLOSE_PAREN.  */
+
+static inline bool
+cp_lexer_token_is_close_brace (cp_token *token)
+{
+  return token->type == CPP_CLOSE_SQUARE
+	 || token->type == CPP_CLOSE_BRACE
+	 || token->type == CPP_CLOSE_PAREN;
+}
+
+
 /* Store the next token from the preprocessor in *TOKEN.  Return true
    if we reach EOF.  If LEXER is NULL, assume we are handling an
    initial #pragma pch_preprocess, and thus want the lexer to return
@@ -835,8 +860,13 @@ cp_lexer_get_preprocessor_token (cp_lexer *lexer, cp_token
*token)
 			    TREE_INT_CST_LOW (token->u.value));
       token->u.value = NULL_TREE;
     }
+  else if (cp_lexer_token_is_open_brace (token))
+    scope_chain->x_brace_nesting++;
+  else if (cp_lexer_token_is_close_brace (token))
+    scope_chain->x_brace_nesting--;
 }
 
+
 /* Update the globals input_location and the input file stack from TOKEN.  */
 static inline void
 cp_lexer_set_source_position_from_token (cp_token *token)
diff --git a/gcc/cp/pph.c b/gcc/cp/pph.c
index 9b1c63c..cbd9c24 100644
--- a/gcc/cp/pph.c
+++ b/gcc/cp/pph.c
@@ -94,11 +94,30 @@ pph_dump_namespace (FILE *file, tree ns)
 }
 
 
+/* Return true if PPH image NAME can be used at the point of inclusion
+   (given by LOC).  */
+
+static bool
+pph_is_valid_here (const char *name, location_t loc)
+{
+  /* If we are inside a scope, reject the image.  We could be inside a
+     namespace or a structure which changes the parsing context for
+     the original text file.  */
+  if (scope_chain->x_brace_nesting > 0)
+    {
+      error_at (loc, "PPH file %s not included at global scope", name);
+      return false;
+    }
+
+  return true;
+}
+
+
 /* Record a #include or #include_next for PPH.  */
 
 static bool
 pph_include_handler (cpp_reader *reader,
-                     location_t loc ATTRIBUTE_UNUSED,
+                     location_t loc,
                      const unsigned char *dname,
                      const char *name,
                      int angle_brackets,
@@ -117,7 +136,9 @@ pph_include_handler (cpp_reader *reader,
 
   read_text_file_p = true;
   pph_file = query_pph_include_map (name);
-  if (pph_file != NULL && !cpp_included_before (reader, name, input_location))
+  if (pph_file != NULL
+      && pph_is_valid_here (name, loc)
+      && !cpp_included_before (reader, name, input_location))
     {
       /* Hack. We do this to mimic what the non-pph compiler does in
 	_cpp_stack_include as our goal is to have identical line_tables.  */
diff --git a/gcc/testsuite/g++.dg/pph/pph.exp b/gcc/testsuite/g++.dg/pph/pph.exp
index 542cf3e..a632365 100644
--- a/gcc/testsuite/g++.dg/pph/pph.exp
+++ b/gcc/testsuite/g++.dg/pph/pph.exp
@@ -38,7 +38,6 @@ exec echo "math.h	math.pph" >> pph.map
 exec echo "stdio.h	stdio.pph" >> pph.map
 exec echo "stdlib.h	stdlib.pph" >> pph.map
 exec echo "string.h	string.pph" >> pph.map
-exec echo "sys/types.h	types.pph" >> pph.map
 
 set mapflag -fpph-map=pph.map
 
diff --git a/gcc/testsuite/g++.dg/pph/y8inc-nmspc.cc
b/gcc/testsuite/g++.dg/pph/y8inc-nmspc.cc
index a8784f7..70b209a 100644
--- a/gcc/testsuite/g++.dg/pph/y8inc-nmspc.cc
+++ b/gcc/testsuite/g++.dg/pph/y8inc-nmspc.cc
@@ -1,4 +1,3 @@
 namespace smother {
-#include "x1struct1.h"
-// { dg-error "pph file not included at global scope" "" { xfail *-*-* } }
+#include "x1struct1.h" // { dg-error "PPH file .* not included at global scope"
"" }
 }

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

[Gabriel Charette, 2011-08-25 22:57:02]

I'm getting the following pph test failure output after this patch
(I'll commit my patch on top of it anyways as I get the same errors
with a clean build and with my patch, which itself had a clean test
output before this recent pull before my commit).

XPASS: g++.dg/pph/x7rtti.cc -fno-dwarf2-cfi-asm  -fpph-map=pph.map -I.
 (test for bogus messages, line )
XPASS: g++.dg/pph/x7rtti.cc -fno-dwarf2-cfi-asm  -fpph-map=pph.map -I.
 (test for bogus messages, line )
XPASS: g++.dg/pph/x7rtti.cc -fno-dwarf2-cfi-asm  -fpph-map=pph.map -I.
 (test for bogus messages, line )
XPASS: g++.dg/pph/x7rtti.cc -fno-dwarf2-cfi-asm  -fpph-map=pph.map -I.
 (test for bogus messages, line )
XPASS: g++.dg/pph/x7rtti.cc -fno-dwarf2-cfi-asm  -fpph-map=pph.map -I.
 (test for bogus messages, line )
XPASS: g++.dg/pph/x7rtti.cc -fno-dwarf2-cfi-asm  -fpph-map=pph.map -I.
 (test for bogus messages, line )
XPASS: g++.dg/pph/x7rtti.cc -fno-dwarf2-cfi-asm  -fpph-map=pph.map -I.
 (test for bogus messages, line )
XPASS: g++.dg/pph/x7rtti.cc -fno-dwarf2-cfi-asm  -fpph-map=pph.map -I.
 (test for bogus messages, line )
XPASS: g++.dg/pph/x7rtti.cc -fno-dwarf2-cfi-asm  -fpph-map=pph.map -I.
 (test for bogus messages, line )
XPASS: g++.dg/pph/x7rtti.cc -fno-dwarf2-cfi-asm  -fpph-map=pph.map -I.
 (test for bogus messages, line )
XPASS: g++.dg/pph/x7rtti.cc -fno-dwarf2-cfi-asm  -fpph-map=pph.map -I.
 (test for bogus messages, line )
XPASS: g++.dg/pph/x7rtti.cc -fno-dwarf2-cfi-asm  -fpph-map=pph.map -I.
 (test for bogus messages, line )
XPASS: g++.dg/pph/x7rtti.cc -fno-dwarf2-cfi-asm  -fpph-map=pph.map -I.
 (test for bogus messages, line )
# of expected passes		336
# of unexpected successes	35
# of expected failures		45
# of unresolved testcases	1

Cheers,
Gab

On Thu, Aug 25, 2011 at 3:30 PM, Diego Novillo <dnovillo@google.com> wrote:
>
> To prevent PPH images from depend on the context in which they were
> generated, we require that the header file be compiled in isolation
> and when included, it should only be included in the global context.
> That is, things like
>
> namespace Foo {
>    #include "bar.h"
>    ...
> };
>
> should prevent bar.h from becoming a PPH image, since all the symbols
> defined in it would belong to Foo, but when bar.pph was generated,
> they belonged to ::
>
> This patch detects the use of PPH images inside nested scopes like
> that.  It is a bit crude, but it works.  During lexing, it keeps track
> of open and close braces (all kinds, not just { }), so when the
> #include command is found, it rejects the image if the nesting level
> is positive.
>
> Jason, I added a field to scope_chain.  That seemed the cleaner
> approach to keep track of the nesting level.  Is that a good place to
> keep track of it?  This is the kind of bookkeeping that scope_chain
> seems to be used for, but I can put it elsewhere if you want.
>
> This error flagged the usage of sys/types.pph.  This file is included
> inside 'extern "C" { }', so strictly speaking it should be rejected.
> We can relax these restrictions later.
>
>
> Tested on x86_64.  Applied to branch.
>
>
>        * cp-tree.h (struct saved_scope): Add field x_brace_nesting.
>        * parser.c (cp_lexer_token_is_open_brace): New.
>        (cp_lexer_token_is_close_brace): New.
>        (cp_lexer_get_preprocessor_token): Call them.
>        Increase scope_chain->x_brace_nesting for open braces, decrease
>        for closing braces.
>        * pph.c (pph_is_valid_here): New.  Return false if
>        scope_chain->x_brace_nesting is greater than 0.
>        (pph_include_handler): Call it.
>
>
> testsuite/ChangeLog.pph
>
>        * g++.dg/pph/pph.exp: Do not create a PPH image for sys/types.h.
>        * g++.dg/pph/y8inc-nmspc.cc: Mark fixed.
>
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 8fb2ccc..e0b67c6 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -973,6 +973,10 @@ struct GTY(()) saved_scope {
>   cp_binding_level *bindings;
>
>   struct saved_scope *prev;
> +
> +  /* Used during lexing to validate where PPH images are included, it
> +     keeps track of nested bracing.  */
> +  unsigned x_brace_nesting;
>  };
>
>  /* The current open namespace.  */
> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> index 009922e..919671c 100644
> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -748,6 +748,31 @@ cp_lexer_saving_tokens (const cp_lexer* lexer)
>   return VEC_length (cp_token_position, lexer->saved_tokens) != 0;
>  }
>
> +
> +/* Return true if TOKEN is one of CPP_OPEN_SQUARE, CPP_OPEN_BRACE or
> +   CPP_OPEN_PAREN.  */
> +
> +static inline bool
> +cp_lexer_token_is_open_brace (cp_token *token)
> +{
> +  return token->type == CPP_OPEN_SQUARE
> +        || token->type == CPP_OPEN_BRACE
> +        || token->type == CPP_OPEN_PAREN;
> +}
> +
> +
> +/* Return true if TOKEN is one of CPP_CLOSE_SQUARE, CPP_CLOSE_BRACE or
> +   CPP_CLOSE_PAREN.  */
> +
> +static inline bool
> +cp_lexer_token_is_close_brace (cp_token *token)
> +{
> +  return token->type == CPP_CLOSE_SQUARE
> +        || token->type == CPP_CLOSE_BRACE
> +        || token->type == CPP_CLOSE_PAREN;
> +}
> +
> +
>  /* Store the next token from the preprocessor in *TOKEN.  Return true
>    if we reach EOF.  If LEXER is NULL, assume we are handling an
>    initial #pragma pch_preprocess, and thus want the lexer to return
> @@ -835,8 +860,13 @@ cp_lexer_get_preprocessor_token (cp_lexer *lexer,
cp_token *token)
>                            TREE_INT_CST_LOW (token->u.value));
>       token->u.value = NULL_TREE;
>     }
> +  else if (cp_lexer_token_is_open_brace (token))
> +    scope_chain->x_brace_nesting++;
> +  else if (cp_lexer_token_is_close_brace (token))
> +    scope_chain->x_brace_nesting--;
>  }
>
> +
>  /* Update the globals input_location and the input file stack from TOKEN.  */
>  static inline void
>  cp_lexer_set_source_position_from_token (cp_token *token)
> diff --git a/gcc/cp/pph.c b/gcc/cp/pph.c
> index 9b1c63c..cbd9c24 100644
> --- a/gcc/cp/pph.c
> +++ b/gcc/cp/pph.c
> @@ -94,11 +94,30 @@ pph_dump_namespace (FILE *file, tree ns)
>  }
>
>
> +/* Return true if PPH image NAME can be used at the point of inclusion
> +   (given by LOC).  */
> +
> +static bool
> +pph_is_valid_here (const char *name, location_t loc)
> +{
> +  /* If we are inside a scope, reject the image.  We could be inside a
> +     namespace or a structure which changes the parsing context for
> +     the original text file.  */
> +  if (scope_chain->x_brace_nesting > 0)
> +    {
> +      error_at (loc, "PPH file %s not included at global scope", name);
> +      return false;
> +    }
> +
> +  return true;
> +}
> +
> +
>  /* Record a #include or #include_next for PPH.  */
>
>  static bool
>  pph_include_handler (cpp_reader *reader,
> -                     location_t loc ATTRIBUTE_UNUSED,
> +                     location_t loc,
>                      const unsigned char *dname,
>                      const char *name,
>                      int angle_brackets,
> @@ -117,7 +136,9 @@ pph_include_handler (cpp_reader *reader,
>
>   read_text_file_p = true;
>   pph_file = query_pph_include_map (name);
> -  if (pph_file != NULL && !cpp_included_before (reader, name,
input_location))
> +  if (pph_file != NULL
> +      && pph_is_valid_here (name, loc)
> +      && !cpp_included_before (reader, name, input_location))
>     {
>       /* Hack. We do this to mimic what the non-pph compiler does in
>        _cpp_stack_include as our goal is to have identical line_tables.  */
> diff --git a/gcc/testsuite/g++.dg/pph/pph.exp
b/gcc/testsuite/g++.dg/pph/pph.exp
> index 542cf3e..a632365 100644
> --- a/gcc/testsuite/g++.dg/pph/pph.exp
> +++ b/gcc/testsuite/g++.dg/pph/pph.exp
> @@ -38,7 +38,6 @@ exec echo "math.h     math.pph" >> pph.map
>  exec echo "stdio.h     stdio.pph" >> pph.map
>  exec echo "stdlib.h    stdlib.pph" >> pph.map
>  exec echo "string.h    string.pph" >> pph.map
> -exec echo "sys/types.h types.pph" >> pph.map
>
>  set mapflag -fpph-map=pph.map
>
> diff --git a/gcc/testsuite/g++.dg/pph/y8inc-nmspc.cc
b/gcc/testsuite/g++.dg/pph/y8inc-nmspc.cc
> index a8784f7..70b209a 100644
> --- a/gcc/testsuite/g++.dg/pph/y8inc-nmspc.cc
> +++ b/gcc/testsuite/g++.dg/pph/y8inc-nmspc.cc
> @@ -1,4 +1,3 @@
>  namespace smother {
> -#include "x1struct1.h"
> -// { dg-error "pph file not included at global scope" "" { xfail *-*-* } }
> +#include "x1struct1.h" // { dg-error "PPH file .* not included at global
scope" "" }
>  }
>
> --
> This patch is available for review at http://codereview.appspot.com/4958045

[Diego Novillo, 2011-08-26 04:27:29]

On 11-08-25 18:56 , Gabriel Charette wrote:

> XPASS: g++.dg/pph/x7rtti.cc -fno-dwarf2-cfi-asm  -fpph-map=pph.map -I.
>   (test for bogus messages, line )
 > [ ... ]

Oops, my fault.  When I changed x7rtti.cc into an executable test, I 
altered line numbers and forgot to update the dg markers.  Fixed.


Diego.