[pph/libcpp] Allow include callback to not read the file

[Diego Novillo, 2011-04-12 20:28:47]

During pph processing, when we find an included file that we are going
to instantiate from an image, we don't want libcpp to stack and read
it.

I've implemented this by allowing the 'include' callback to return a
boolean value.  If it returns true, then we call _cpp_stack_include.
Otherwise, the file is ignored.

Tom, I believe this is the right approach, but I'm not sure.  It does
what I want, though.


Thanks.  Diego.

libcpp/ChangeLog.pph
2011-04-12  Diego Novillo  <dnovillo@google.com>

	* directives.c (do_include_common): If the callback
	pfile->cb.include returns falose do not call
	_cpp_stack_include.
	* include/cpplib.h (struct cpp_callbacks): Change return
	type of field 'include' to bool.

gcc/cp/ChangeLog.pph
2011-04-12  Diego Novillo  <dnovillo@google.com>

	* pph.c (pth_include_handler): Return true.
	(pph_include_handler): If the header file exists as a PPH
	image, return false.

diff --git a/gcc/cp/pph.c b/gcc/cp/pph.c
index 74d1d50..6584e72 100644
--- a/gcc/cp/pph.c
+++ b/gcc/cp/pph.c
@@ -1984,9 +2008,10 @@ pph_read_file (const char *filename)
     error ("Cannot open PPH file for reading: %s: %m", filename);
 }
 
+
 /* Record a #include or #include_next for PTH.  */
 
-static void
+static bool
 pth_include_handler (cpp_reader *reader ATTRIBUTE_UNUSED,
 	             location_t loc ATTRIBUTE_UNUSED,
 	             const unsigned char *dname,
@@ -2012,11 +2037,14 @@ pth_include_handler (cpp_reader *reader
ATTRIBUTE_UNUSED,
 
   state->new_angle_brackets = angle_brackets;
   state->new_iname = name;
+
+  return true;
 }
 
+
 /* Record a #include or #include_next for PPH.  */
 
-static void
+static bool
 pph_include_handler (cpp_reader *reader,
                      location_t loc ATTRIBUTE_UNUSED,
                      const unsigned char *dname,
@@ -2025,6 +2053,7 @@ pph_include_handler (cpp_reader *reader,
                      const cpp_token **tok_p ATTRIBUTE_UNUSED)
 {
   const char *pph_file;
+  bool read_text_file_p;
 
   if (flag_pph_debug >= 1)
     {
@@ -2034,9 +2063,15 @@ pph_include_handler (cpp_reader *reader,
       fprintf (pph_logfile, "%c\n", angle_brackets ? '>' : '"');
     }
 
+  read_text_file_p = true;
   pph_file = query_pph_include_map (name);
   if (pph_file != NULL && !cpp_included_before (reader, name, input_location))
-    pph_read_file (pph_file);
+    {
+      pph_read_file (pph_file);
+      read_text_file_p = false;
+    }
+
+  return read_text_file_p;
 }
 
 
diff --git a/libcpp/directives.c b/libcpp/directives.c
index 7812b57..09c7686 100644
--- a/libcpp/directives.c
+++ b/libcpp/directives.c
@@ -788,15 +788,19 @@ do_include_common (cpp_reader *pfile, enum include_type
type)
     cpp_error (pfile, CPP_DL_ERROR, "#include nested too deeply");
   else
     {
+      bool do_stack_include;
+
       /* Get out of macro context, if we are.  */
       skip_rest_of_line (pfile);
 
+      do_stack_include = true;
       if (pfile->cb.include)
-	pfile->cb.include (pfile, pfile->directive_line,
-			   pfile->directive->name, fname, angle_brackets,
-			   buf);
+	do_stack_include = pfile->cb.include (pfile, pfile->directive_line,
+					      pfile->directive->name,
+					      fname, angle_brackets, buf);
 
-      _cpp_stack_include (pfile, NULL, fname, angle_brackets, type);
+      if (do_stack_include)
+	_cpp_stack_include (pfile, NULL, fname, angle_brackets, type);
     }
 
   XDELETEVEC (fname);
diff --git a/libcpp/include/cpplib.h b/libcpp/include/cpplib.h
index 3dc4139..c9f3dfb 100644
--- a/libcpp/include/cpplib.h
+++ b/libcpp/include/cpplib.h
@@ -480,7 +480,9 @@ struct cpp_callbacks
   void (*file_change) (cpp_reader *, const struct line_map *);
 
   void (*dir_change) (cpp_reader *, const char *);
-  void (*include) (cpp_reader *, unsigned int, const unsigned char *,
+  /* Called when processing a #include directive.  If the handler
+     returns false, the file will not be read.  */
+  bool (*include) (cpp_reader *, unsigned int, const unsigned char *,
 		   const char *, int, const cpp_token **);
   void (*define) (cpp_reader *, unsigned int, cpp_hashnode *);
   void (*undef) (cpp_reader *, unsigned int, cpp_hashnode *);

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

[Lawrence Crowl, 2011-04-13 00:41:43]

This change is not source compatible with existing code using
the callbacks (which may not be in the gcc svn).  Perhaps a new
callback is needed?

On 4/12/11, Diego Novillo <dnovillo@google.com> wrote:
>
> During pph processing, when we find an included file that we are going
> to instantiate from an image, we don't want libcpp to stack and read
> it.
>
> I've implemented this by allowing the 'include' callback to return a
> boolean value.  If it returns true, then we call _cpp_stack_include.
> Otherwise, the file is ignored.
>
> Tom, I believe this is the right approach, but I'm not sure.  It does
> what I want, though.
>
>
> Thanks.  Diego.
>
> libcpp/ChangeLog.pph
> 2011-04-12  Diego Novillo  <dnovillo@google.com>
>
> 	* directives.c (do_include_common): If the callback
> 	pfile->cb.include returns falose do not call
> 	_cpp_stack_include.
> 	* include/cpplib.h (struct cpp_callbacks): Change return
> 	type of field 'include' to bool.
>
> gcc/cp/ChangeLog.pph
> 2011-04-12  Diego Novillo  <dnovillo@google.com>
>
> 	* pph.c (pth_include_handler): Return true.
> 	(pph_include_handler): If the header file exists as a PPH
> 	image, return false.
>
> diff --git a/gcc/cp/pph.c b/gcc/cp/pph.c
> index 74d1d50..6584e72 100644
> --- a/gcc/cp/pph.c
> +++ b/gcc/cp/pph.c
> @@ -1984,9 +2008,10 @@ pph_read_file (const char *filename)
>      error ("Cannot open PPH file for reading: %s: %m", filename);
>  }
>
> +
>  /* Record a #include or #include_next for PTH.  */
>
> -static void
> +static bool
>  pth_include_handler (cpp_reader *reader ATTRIBUTE_UNUSED,
>  	             location_t loc ATTRIBUTE_UNUSED,
>  	             const unsigned char *dname,
> @@ -2012,11 +2037,14 @@ pth_include_handler (cpp_reader *reader
> ATTRIBUTE_UNUSED,
>
>    state->new_angle_brackets = angle_brackets;
>    state->new_iname = name;
> +
> +  return true;
>  }
>
> +
>  /* Record a #include or #include_next for PPH.  */
>
> -static void
> +static bool
>  pph_include_handler (cpp_reader *reader,
>                       location_t loc ATTRIBUTE_UNUSED,
>                       const unsigned char *dname,
> @@ -2025,6 +2053,7 @@ pph_include_handler (cpp_reader *reader,
>                       const cpp_token **tok_p ATTRIBUTE_UNUSED)
>  {
>    const char *pph_file;
> +  bool read_text_file_p;
>
>    if (flag_pph_debug >= 1)
>      {
> @@ -2034,9 +2063,15 @@ pph_include_handler (cpp_reader *reader,
>        fprintf (pph_logfile, "%c\n", angle_brackets ? '>' : '"');
>      }
>
> +  read_text_file_p = true;
>    pph_file = query_pph_include_map (name);
>    if (pph_file != NULL && !cpp_included_before (reader, name,
> input_location))
> -    pph_read_file (pph_file);
> +    {
> +      pph_read_file (pph_file);
> +      read_text_file_p = false;
> +    }
> +
> +  return read_text_file_p;
>  }
>
>
> diff --git a/libcpp/directives.c b/libcpp/directives.c
> index 7812b57..09c7686 100644
> --- a/libcpp/directives.c
> +++ b/libcpp/directives.c
> @@ -788,15 +788,19 @@ do_include_common (cpp_reader *pfile, enum
> include_type type)
>      cpp_error (pfile, CPP_DL_ERROR, "#include nested too deeply");
>    else
>      {
> +      bool do_stack_include;
> +
>        /* Get out of macro context, if we are.  */
>        skip_rest_of_line (pfile);
>
> +      do_stack_include = true;
>        if (pfile->cb.include)
> -	pfile->cb.include (pfile, pfile->directive_line,
> -			   pfile->directive->name, fname, angle_brackets,
> -			   buf);
> +	do_stack_include = pfile->cb.include (pfile, pfile->directive_line,
> +					      pfile->directive->name,
> +					      fname, angle_brackets, buf);
>
> -      _cpp_stack_include (pfile, NULL, fname, angle_brackets, type);
> +      if (do_stack_include)
> +	_cpp_stack_include (pfile, NULL, fname, angle_brackets, type);
>      }
>
>    XDELETEVEC (fname);
> diff --git a/libcpp/include/cpplib.h b/libcpp/include/cpplib.h
> index 3dc4139..c9f3dfb 100644
> --- a/libcpp/include/cpplib.h
> +++ b/libcpp/include/cpplib.h
> @@ -480,7 +480,9 @@ struct cpp_callbacks
>    void (*file_change) (cpp_reader *, const struct line_map *);
>
>    void (*dir_change) (cpp_reader *, const char *);
> -  void (*include) (cpp_reader *, unsigned int, const unsigned char *,
> +  /* Called when processing a #include directive.  If the handler
> +     returns false, the file will not be read.  */
> +  bool (*include) (cpp_reader *, unsigned int, const unsigned char *,
>  		   const char *, int, const cpp_token **);
>    void (*define) (cpp_reader *, unsigned int, cpp_hashnode *);
>    void (*undef) (cpp_reader *, unsigned int, cpp_hashnode *);
>
> --
> This patch is available for review at http://codereview.appspot.com/4388057
>


-- 
Lawrence Crowl

[Diego Novillo, 2011-04-13 00:50:42]

On Tue, Apr 12, 2011 at 20:41, Lawrence Crowl <crowl@google.com> wrote:
> This change is not source compatible with existing code using
> the callbacks (which may not be in the gcc svn).  Perhaps a new
> callback is needed?

Well, it only changes the return value for the callback.  Existing
users do not really need to be changed.

I don't think we want a new callback.  The callback would do exactly
what cb.include does.


Diego.

[tromey_redhat.com, 2011-04-13 16:20:56]

>>>>> "Diego" == Diego Novillo <dnovillo@google.com> writes:

Diego> During pph processing, when we find an included file that we are going
Diego> to instantiate from an image, we don't want libcpp to stack and read
Diego> it.

Diego> I've implemented this by allowing the 'include' callback to return a
Diego> boolean value.  If it returns true, then we call _cpp_stack_include.
Diego> Otherwise, the file is ignored.

Diego> Tom, I believe this is the right approach, but I'm not sure.  It does
Diego> what I want, though.

It seems reasonable enough to me.

Tom

[tromey_redhat.com, 2011-04-13 16:21:58]

>>>>> "Diego" == Diego Novillo <dnovillo@google.com> writes:

Lawrence> This change is not source compatible with existing code using
Lawrence> the callbacks (which may not be in the gcc svn).  Perhaps a new
Lawrence> callback is needed?

Diego> Well, it only changes the return value for the callback.  Existing
Diego> users do not really need to be changed.

Diego> I don't think we want a new callback.  The callback would do exactly
Diego> what cb.include does.

My grep shows 2 places that set this: c-family/c-ppoutput.c and
fortran/cpp.c.  It seems like it would be simple to just update those
two functions to account for the type change.

Tom

[Diego Novillo, 2011-04-13 17:23:33]

On Wed, Apr 13, 2011 at 12:21, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Diego" == Diego Novillo <dnovillo@google.com> writes:
>
> Lawrence> This change is not source compatible with existing code using
> Lawrence> the callbacks (which may not be in the gcc svn).  Perhaps a new
> Lawrence> callback is needed?
>
> Diego> Well, it only changes the return value for the callback.  Existing
> Diego> users do not really need to be changed.
>
> Diego> I don't think we want a new callback.  The callback would do exactly
> Diego> what cb.include does.
>
> My grep shows 2 places that set this: c-family/c-ppoutput.c and
> fortran/cpp.c.  It seems like it would be simple to just update those
> two functions to account for the type change.

Oh, right.  Here I was thinking of users outside of gcc, but of course
there aren't those.  I'll fix the calls in the existing front ends.


Diego.