Linemap force location and remove LINEMAP_POSITION_FOR_COLUMN

[Gabriel Charette, 2011-08-10 18:22:56]

There was a bug where c_finish_options would create some builtins and assign
them source_locations in the linemap other than BUILTINS_LOCATION == 1.

Thus, when calling DECL_IS_BUILTIN to know if a decl is a builtin, some of them
would return false as they had a source_location other than BUILTINS_LOCATION
within the line_map entry that was incorrectly created in c_finish_options.

The problem to fix this is that _cpp_lex_direct just gets the location of the
currently lexed token by calling linemap_position_for_column. It has no notion
of whether this token is a builtin or whatever else it may be.

My solution is to add a forced_location field to the line_table which when set
is returned by default by linemap_position_for_column instead of getting a new
loc from the line_table.

Furthermore, this mechanism will allow us to inject locations for a similar
problem we have in pph when lexing replayed pre-processor definitions (which as
it is now are getting assigned new source_locations which is creating problems
for us).

I'm open to other solutions if anyone sees another way to do it. We could also
leave linemap_position_for_column as is and have a separate inline function, say
linemap_pos_for_col_or_forced, which makes the forced_location check and simply
calls linemap_position_for_column when no forced_location.

I also removed LINEMAP_POSITION_FOR_COLUMN, it did the EXACT same thing as
linemap_position_for_column, so maintaining both in parallel seems like overkill
to me. The only thing I can think of is that it's more optimal as it's inlined
(but if that's really needed we can always make linemap_position_for_column an
inline function).

Gabriel

2011-08-10  Gabriel Charette  <gchare@google.com>

	* c-opts.c (c_finish_options): Don't create built-in line_table entry;
	instead force BUILTINS_LOCATION when creating builtins.

	* include/line-map.h (struct line_maps): Add field forced_location.
	(LINEMAP_POSITION_FOR_COLUMN): Remove.
	* line-map.c (linemap_init): Init forced_location to 0.
	(linemap_position_for_column): Return forced_location by default if set

diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
index 3227f7b..1af8e7b 100644
--- a/gcc/c-family/c-opts.c
+++ b/gcc/c-family/c-opts.c
@@ -1306,13 +1306,15 @@ c_finish_options (void)
     {
       size_t i;
 
-      cb_file_change (parse_in,
-		      linemap_add (line_table, LC_RENAME, 0,
-				   _("<built-in>"), 0));
+      /* Make sure all of the builtins about to be declared have
+         BUILTINS_LOCATION has their source_location.  */
+      line_table->forced_location = BUILTINS_LOCATION;
 
       cpp_init_builtins (parse_in, flag_hosted);
       c_cpp_builtins (parse_in);
 
+      line_table->forced_location = 0;
+
       /* We're about to send user input to cpplib, so make it warn for
 	 things that we previously (when we sent it internal definitions)
 	 told it to not warn.
diff --git a/gcc/go/gofrontend/lex.cc b/gcc/go/gofrontend/lex.cc
index 9f26911..167c7dd 100644
--- a/gcc/go/gofrontend/lex.cc
+++ b/gcc/go/gofrontend/lex.cc
@@ -518,9 +518,7 @@ Lex::require_line()
 source_location
 Lex::location() const
 {
-  source_location location;
-  LINEMAP_POSITION_FOR_COLUMN(location, line_table, this->lineoff_ + 1);
-  return location;
+  return linemap_position_for_column (line_table, this->lineoff_ + 1);
 }
 
 // Get a location slightly before the current one.  This is used for
@@ -529,9 +527,7 @@ Lex::location() const
 source_location
 Lex::earlier_location(int chars) const
 {
-  source_location location;
-  LINEMAP_POSITION_FOR_COLUMN(location, line_table, this->lineoff_ + 1 -
chars);
-  return location;
+  return linemap_position_for_column (line_table, this->lineoff_ + 1 - chars);
 }
 
 // Get the next token.
diff --git a/libcpp/directives-only.c b/libcpp/directives-only.c
index e19f806..c6772af 100644
--- a/libcpp/directives-only.c
+++ b/libcpp/directives-only.c
@@ -142,7 +142,7 @@ _cpp_preprocess_dir_only (cpp_reader *pfile,
 	    flags |= DO_LINE_COMMENT;
 	  else if (!(flags & DO_SPECIAL))
 	    /* Mark the position for possible error reporting. */
-	    LINEMAP_POSITION_FOR_COLUMN (loc, pfile->line_table, col);
+	    loc = linemap_position_for_column (pfile->line_table, col);
 
 	  break;
 
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index f1d5bee..d14528e 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -95,6 +95,11 @@ struct GTY(()) line_maps {
   /* If non-null, the allocator to use when resizing 'maps'.  If null,
      xrealloc is used.  */
   line_map_realloc reallocator;
+
+  /* If non-zero, linemap_position_for_column automatically returns
+     the value stored at this memory location, instead of caclulating
+     a new source_location.  */
+  source_location forced_location;
 };
 
 /* Initialize a line map set.  */
@@ -165,23 +170,6 @@ extern const struct line_map *linemap_lookup
 /* Nonzero if the map is at the bottom of the include stack.  */
 #define MAIN_FILE_P(MAP) ((MAP)->included_from < 0)
 
-/* Set LOC to a source position that is the same line as the most recent
-   linemap_line_start, but with the specified TO_COLUMN column number.  */
-
-#define LINEMAP_POSITION_FOR_COLUMN(LOC, SET, TO_COLUMN) do { \
-  unsigned int to_column = (TO_COLUMN); \
-  struct line_maps *set = (SET); \
-  if (__builtin_expect (to_column >= set->max_column_hint, 0)) \
-    (LOC) = linemap_position_for_column (set, to_column); \
-  else { \
-    source_location r = set->highest_line; \
-    r = r + to_column; \
-    if (r >= set->highest_location) \
-      set->highest_location = r; \
-    (LOC) = r;			 \
-  }} while (0)
-    
-
 extern source_location
 linemap_position_for_column (struct line_maps *set, unsigned int to_column);
 
diff --git a/libcpp/lex.c b/libcpp/lex.c
index d29f36d..d460b98 100644
--- a/libcpp/lex.c
+++ b/libcpp/lex.c
@@ -1975,8 +1975,8 @@ _cpp_lex_direct (cpp_reader *pfile)
     }
   c = *buffer->cur++;
 
-  LINEMAP_POSITION_FOR_COLUMN (result->src_loc, pfile->line_table,
-			       CPP_BUF_COLUMN (buffer, buffer->cur));
+  result->src_loc = linemap_position_for_column (pfile->line_table,
+			                                    CPP_BUF_COLUMN (buffer, buffer->cur));
 
   switch (c)
     {
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index dd3f11c..31ab672 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -41,6 +41,7 @@ linemap_init (struct line_maps *set)
   set->highest_line = RESERVED_LOCATION_COUNT - 1;
   set->max_column_hint = 0;
   set->reallocator = 0;
+  set->forced_location = 0;
 }
 
 /* Check for and warn about line_maps entered but not exited.  */
@@ -245,6 +246,9 @@ linemap_line_start (struct line_maps *set, linenum_type
to_line,
 source_location
 linemap_position_for_column (struct line_maps *set, unsigned int to_column)
 {
+  if (set->forced_location)
+    return set->forced_location;
+
   source_location r = set->highest_line;
   if (to_column >= set->max_column_hint)
     {

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

[Gabriel Charette, 2011-08-10 18:29:08]

Tested with bootstrap and full regression testing on x64.

On Wed, Aug 10, 2011 at 11:22 AM, Gabriel Charette <gchare@google.com> wrote:
> There was a bug where c_finish_options would create some builtins and assign
them source_locations in the linemap other than BUILTINS_LOCATION == 1.
>
> Thus, when calling DECL_IS_BUILTIN to know if a decl is a builtin, some of
them would return false as they had a source_location other than
BUILTINS_LOCATION within the line_map entry that was incorrectly created in
c_finish_options.
>
> The problem to fix this is that _cpp_lex_direct just gets the location of the
currently lexed token by calling linemap_position_for_column. It has no notion
of whether this token is a builtin or whatever else it may be.
>
> My solution is to add a forced_location field to the line_table which when set
is returned by default by linemap_position_for_column instead of getting a new
loc from the line_table.
>
> Furthermore, this mechanism will allow us to inject locations for a similar
problem we have in pph when lexing replayed pre-processor definitions (which as
it is now are getting assigned new source_locations which is creating problems
for us).
>
> I'm open to other solutions if anyone sees another way to do it. We could also
leave linemap_position_for_column as is and have a separate inline function, say
linemap_pos_for_col_or_forced, which makes the forced_location check and simply
calls linemap_position_for_column when no forced_location.
>
> I also removed LINEMAP_POSITION_FOR_COLUMN, it did the EXACT same thing as
linemap_position_for_column, so maintaining both in parallel seems like overkill
to me. The only thing I can think of is that it's more optimal as it's inlined
(but if that's really needed we can always make linemap_position_for_column an
inline function).
>
> Gabriel
>
> 2011-08-10  Gabriel Charette  <gchare@google.com>
>
>        * c-opts.c (c_finish_options): Don't create built-in line_table entry;
>        instead force BUILTINS_LOCATION when creating builtins.
>
>        * include/line-map.h (struct line_maps): Add field forced_location.
>        (LINEMAP_POSITION_FOR_COLUMN): Remove.
>        * line-map.c (linemap_init): Init forced_location to 0.
>        (linemap_position_for_column): Return forced_location by default if set
>
> diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
> index 3227f7b..1af8e7b 100644
> --- a/gcc/c-family/c-opts.c
> +++ b/gcc/c-family/c-opts.c
> @@ -1306,13 +1306,15 @@ c_finish_options (void)
>     {
>       size_t i;
>
> -      cb_file_change (parse_in,
> -                     linemap_add (line_table, LC_RENAME, 0,
> -                                  _("<built-in>"), 0));
> +      /* Make sure all of the builtins about to be declared have
> +         BUILTINS_LOCATION has their source_location.  */
> +      line_table->forced_location = BUILTINS_LOCATION;
>
>       cpp_init_builtins (parse_in, flag_hosted);
>       c_cpp_builtins (parse_in);
>
> +      line_table->forced_location = 0;
> +
>       /* We're about to send user input to cpplib, so make it warn for
>         things that we previously (when we sent it internal definitions)
>         told it to not warn.
> diff --git a/gcc/go/gofrontend/lex.cc b/gcc/go/gofrontend/lex.cc
> index 9f26911..167c7dd 100644
> --- a/gcc/go/gofrontend/lex.cc
> +++ b/gcc/go/gofrontend/lex.cc
> @@ -518,9 +518,7 @@ Lex::require_line()
>  source_location
>  Lex::location() const
>  {
> -  source_location location;
> -  LINEMAP_POSITION_FOR_COLUMN(location, line_table, this->lineoff_ + 1);
> -  return location;
> +  return linemap_position_for_column (line_table, this->lineoff_ + 1);
>  }
>
>  // Get a location slightly before the current one.  This is used for
> @@ -529,9 +527,7 @@ Lex::location() const
>  source_location
>  Lex::earlier_location(int chars) const
>  {
> -  source_location location;
> -  LINEMAP_POSITION_FOR_COLUMN(location, line_table, this->lineoff_ + 1 -
chars);
> -  return location;
> +  return linemap_position_for_column (line_table, this->lineoff_ + 1 -
chars);
>  }
>
>  // Get the next token.
> diff --git a/libcpp/directives-only.c b/libcpp/directives-only.c
> index e19f806..c6772af 100644
> --- a/libcpp/directives-only.c
> +++ b/libcpp/directives-only.c
> @@ -142,7 +142,7 @@ _cpp_preprocess_dir_only (cpp_reader *pfile,
>            flags |= DO_LINE_COMMENT;
>          else if (!(flags & DO_SPECIAL))
>            /* Mark the position for possible error reporting. */
> -           LINEMAP_POSITION_FOR_COLUMN (loc, pfile->line_table, col);
> +           loc = linemap_position_for_column (pfile->line_table, col);
>
>          break;
>
> diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
> index f1d5bee..d14528e 100644
> --- a/libcpp/include/line-map.h
> +++ b/libcpp/include/line-map.h
> @@ -95,6 +95,11 @@ struct GTY(()) line_maps {
>   /* If non-null, the allocator to use when resizing 'maps'.  If null,
>      xrealloc is used.  */
>   line_map_realloc reallocator;
> +
> +  /* If non-zero, linemap_position_for_column automatically returns
> +     the value stored at this memory location, instead of caclulating
> +     a new source_location.  */
> +  source_location forced_location;
>  };
>
>  /* Initialize a line map set.  */
> @@ -165,23 +170,6 @@ extern const struct line_map *linemap_lookup
>  /* Nonzero if the map is at the bottom of the include stack.  */
>  #define MAIN_FILE_P(MAP) ((MAP)->included_from < 0)
>
> -/* Set LOC to a source position that is the same line as the most recent
> -   linemap_line_start, but with the specified TO_COLUMN column number.  */
> -
> -#define LINEMAP_POSITION_FOR_COLUMN(LOC, SET, TO_COLUMN) do { \
> -  unsigned int to_column = (TO_COLUMN); \
> -  struct line_maps *set = (SET); \
> -  if (__builtin_expect (to_column >= set->max_column_hint, 0)) \
> -    (LOC) = linemap_position_for_column (set, to_column); \
> -  else { \
> -    source_location r = set->highest_line; \
> -    r = r + to_column; \
> -    if (r >= set->highest_location) \
> -      set->highest_location = r; \
> -    (LOC) = r;                  \
> -  }} while (0)
> -
> -
>  extern source_location
>  linemap_position_for_column (struct line_maps *set, unsigned int to_column);
>
> diff --git a/libcpp/lex.c b/libcpp/lex.c
> index d29f36d..d460b98 100644
> --- a/libcpp/lex.c
> +++ b/libcpp/lex.c
> @@ -1975,8 +1975,8 @@ _cpp_lex_direct (cpp_reader *pfile)
>     }
>   c = *buffer->cur++;
>
> -  LINEMAP_POSITION_FOR_COLUMN (result->src_loc, pfile->line_table,
> -                              CPP_BUF_COLUMN (buffer, buffer->cur));
> +  result->src_loc = linemap_position_for_column (pfile->line_table,
> +                                                           CPP_BUF_COLUMN
(buffer, buffer->cur));
>
>   switch (c)
>     {
> diff --git a/libcpp/line-map.c b/libcpp/line-map.c
> index dd3f11c..31ab672 100644
> --- a/libcpp/line-map.c
> +++ b/libcpp/line-map.c
> @@ -41,6 +41,7 @@ linemap_init (struct line_maps *set)
>   set->highest_line = RESERVED_LOCATION_COUNT - 1;
>   set->max_column_hint = 0;
>   set->reallocator = 0;
> +  set->forced_location = 0;
>  }
>
>  /* Check for and warn about line_maps entered but not exited.  */
> @@ -245,6 +246,9 @@ linemap_line_start (struct line_maps *set, linenum_type
to_line,
>  source_location
>  linemap_position_for_column (struct line_maps *set, unsigned int to_column)
>  {
> +  if (set->forced_location)
> +    return set->forced_location;
> +
>   source_location r = set->highest_line;
>   if (to_column >= set->max_column_hint)
>     {
>
> --
> This patch is available for review at http://codereview.appspot.com/4801090
>

[richard.guenther_gmail.com, 2011-08-11 07:27:59]

On Wed, Aug 10, 2011 at 8:22 PM, Gabriel Charette <gchare@google.com> wrote:
> There was a bug where c_finish_options would create some builtins and assign
them source_locations in the linemap other than BUILTINS_LOCATION == 1.
>
> Thus, when calling DECL_IS_BUILTIN to know if a decl is a builtin, some of
them would return false as they had a source_location other than
BUILTINS_LOCATION within the line_map entry that was incorrectly created in
c_finish_options.

DECL_IS_BUILTIN is almost never the appropriate thing to use, instead
you should use DECL_BUILT_IN (and grepping, I see some suspicious uses
...).

Richard.

> The problem to fix this is that _cpp_lex_direct just gets the location of the
currently lexed token by calling linemap_position_for_column. It has no notion
of whether this token is a builtin or whatever else it may be.
>
> My solution is to add a forced_location field to the line_table which when set
is returned by default by linemap_position_for_column instead of getting a new
loc from the line_table.
>
> Furthermore, this mechanism will allow us to inject locations for a similar
problem we have in pph when lexing replayed pre-processor definitions (which as
it is now are getting assigned new source_locations which is creating problems
for us).
>
> I'm open to other solutions if anyone sees another way to do it. We could also
leave linemap_position_for_column as is and have a separate inline function, say
linemap_pos_for_col_or_forced, which makes the forced_location check and simply
calls linemap_position_for_column when no forced_location.
>
> I also removed LINEMAP_POSITION_FOR_COLUMN, it did the EXACT same thing as
linemap_position_for_column, so maintaining both in parallel seems like overkill
to me. The only thing I can think of is that it's more optimal as it's inlined
(but if that's really needed we can always make linemap_position_for_column an
inline function).
>
> Gabriel
>
> 2011-08-10  Gabriel Charette  <gchare@google.com>
>
>        * c-opts.c (c_finish_options): Don't create built-in line_table entry;
>        instead force BUILTINS_LOCATION when creating builtins.
>
>        * include/line-map.h (struct line_maps): Add field forced_location.
>        (LINEMAP_POSITION_FOR_COLUMN): Remove.
>        * line-map.c (linemap_init): Init forced_location to 0.
>        (linemap_position_for_column): Return forced_location by default if set
>
> diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
> index 3227f7b..1af8e7b 100644
> --- a/gcc/c-family/c-opts.c
> +++ b/gcc/c-family/c-opts.c
> @@ -1306,13 +1306,15 @@ c_finish_options (void)
>     {
>       size_t i;
>
> -      cb_file_change (parse_in,
> -                     linemap_add (line_table, LC_RENAME, 0,
> -                                  _("<built-in>"), 0));
> +      /* Make sure all of the builtins about to be declared have
> +         BUILTINS_LOCATION has their source_location.  */
> +      line_table->forced_location = BUILTINS_LOCATION;
>
>       cpp_init_builtins (parse_in, flag_hosted);
>       c_cpp_builtins (parse_in);
>
> +      line_table->forced_location = 0;
> +
>       /* We're about to send user input to cpplib, so make it warn for
>         things that we previously (when we sent it internal definitions)
>         told it to not warn.
> diff --git a/gcc/go/gofrontend/lex.cc b/gcc/go/gofrontend/lex.cc
> index 9f26911..167c7dd 100644
> --- a/gcc/go/gofrontend/lex.cc
> +++ b/gcc/go/gofrontend/lex.cc
> @@ -518,9 +518,7 @@ Lex::require_line()
>  source_location
>  Lex::location() const
>  {
> -  source_location location;
> -  LINEMAP_POSITION_FOR_COLUMN(location, line_table, this->lineoff_ + 1);
> -  return location;
> +  return linemap_position_for_column (line_table, this->lineoff_ + 1);
>  }
>
>  // Get a location slightly before the current one.  This is used for
> @@ -529,9 +527,7 @@ Lex::location() const
>  source_location
>  Lex::earlier_location(int chars) const
>  {
> -  source_location location;
> -  LINEMAP_POSITION_FOR_COLUMN(location, line_table, this->lineoff_ + 1 -
chars);
> -  return location;
> +  return linemap_position_for_column (line_table, this->lineoff_ + 1 -
chars);
>  }
>
>  // Get the next token.
> diff --git a/libcpp/directives-only.c b/libcpp/directives-only.c
> index e19f806..c6772af 100644
> --- a/libcpp/directives-only.c
> +++ b/libcpp/directives-only.c
> @@ -142,7 +142,7 @@ _cpp_preprocess_dir_only (cpp_reader *pfile,
>            flags |= DO_LINE_COMMENT;
>          else if (!(flags & DO_SPECIAL))
>            /* Mark the position for possible error reporting. */
> -           LINEMAP_POSITION_FOR_COLUMN (loc, pfile->line_table, col);
> +           loc = linemap_position_for_column (pfile->line_table, col);
>
>          break;
>
> diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
> index f1d5bee..d14528e 100644
> --- a/libcpp/include/line-map.h
> +++ b/libcpp/include/line-map.h
> @@ -95,6 +95,11 @@ struct GTY(()) line_maps {
>   /* If non-null, the allocator to use when resizing 'maps'.  If null,
>      xrealloc is used.  */
>   line_map_realloc reallocator;
> +
> +  /* If non-zero, linemap_position_for_column automatically returns
> +     the value stored at this memory location, instead of caclulating
> +     a new source_location.  */
> +  source_location forced_location;
>  };
>
>  /* Initialize a line map set.  */
> @@ -165,23 +170,6 @@ extern const struct line_map *linemap_lookup
>  /* Nonzero if the map is at the bottom of the include stack.  */
>  #define MAIN_FILE_P(MAP) ((MAP)->included_from < 0)
>
> -/* Set LOC to a source position that is the same line as the most recent
> -   linemap_line_start, but with the specified TO_COLUMN column number.  */
> -
> -#define LINEMAP_POSITION_FOR_COLUMN(LOC, SET, TO_COLUMN) do { \
> -  unsigned int to_column = (TO_COLUMN); \
> -  struct line_maps *set = (SET); \
> -  if (__builtin_expect (to_column >= set->max_column_hint, 0)) \
> -    (LOC) = linemap_position_for_column (set, to_column); \
> -  else { \
> -    source_location r = set->highest_line; \
> -    r = r + to_column; \
> -    if (r >= set->highest_location) \
> -      set->highest_location = r; \
> -    (LOC) = r;                  \
> -  }} while (0)
> -
> -
>  extern source_location
>  linemap_position_for_column (struct line_maps *set, unsigned int to_column);
>
> diff --git a/libcpp/lex.c b/libcpp/lex.c
> index d29f36d..d460b98 100644
> --- a/libcpp/lex.c
> +++ b/libcpp/lex.c
> @@ -1975,8 +1975,8 @@ _cpp_lex_direct (cpp_reader *pfile)
>     }
>   c = *buffer->cur++;
>
> -  LINEMAP_POSITION_FOR_COLUMN (result->src_loc, pfile->line_table,
> -                              CPP_BUF_COLUMN (buffer, buffer->cur));
> +  result->src_loc = linemap_position_for_column (pfile->line_table,
> +                                                           CPP_BUF_COLUMN
(buffer, buffer->cur));
>
>   switch (c)
>     {
> diff --git a/libcpp/line-map.c b/libcpp/line-map.c
> index dd3f11c..31ab672 100644
> --- a/libcpp/line-map.c
> +++ b/libcpp/line-map.c
> @@ -41,6 +41,7 @@ linemap_init (struct line_maps *set)
>   set->highest_line = RESERVED_LOCATION_COUNT - 1;
>   set->max_column_hint = 0;
>   set->reallocator = 0;
> +  set->forced_location = 0;
>  }
>
>  /* Check for and warn about line_maps entered but not exited.  */
> @@ -245,6 +246,9 @@ linemap_line_start (struct line_maps *set, linenum_type
to_line,
>  source_location
>  linemap_position_for_column (struct line_maps *set, unsigned int to_column)
>  {
> +  if (set->forced_location)
> +    return set->forced_location;
> +
>   source_location r = set->highest_line;
>   if (to_column >= set->max_column_hint)
>     {
>
> --
> This patch is available for review at http://codereview.appspot.com/4801090
>

[Diego Novillo, 2011-08-11 13:01:49]

On Thu, Aug 11, 2011 at 03:27, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Wed, Aug 10, 2011 at 8:22 PM, Gabriel Charette <gchare@google.com> wrote:
>> There was a bug where c_finish_options would create some builtins and assign
them source_locations in the linemap other than BUILTINS_LOCATION == 1.
>>
>> Thus, when calling DECL_IS_BUILTIN to know if a decl is a builtin, some of
them would return false as they had a source_location other than
BUILTINS_LOCATION within the line_map entry that was incorrectly created in
c_finish_options.
>
> DECL_IS_BUILTIN is almost never the appropriate thing to use, instead
> you should use DECL_BUILT_IN (and grepping, I see some suspicious uses
> ...).

Gah.  Then we need to get rid of one of these two.  Whichever is used
fewer times, I suppose.


Diego.

[richard.guenther_gmail.com, 2011-08-11 13:32:20]

On Thu, Aug 11, 2011 at 3:01 PM, Diego Novillo <dnovillo@google.com> wrote:
> On Thu, Aug 11, 2011 at 03:27, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Wed, Aug 10, 2011 at 8:22 PM, Gabriel Charette <gchare@google.com> wrote:
>>> There was a bug where c_finish_options would create some builtins and assign
them source_locations in the linemap other than BUILTINS_LOCATION == 1.
>>>
>>> Thus, when calling DECL_IS_BUILTIN to know if a decl is a builtin, some of
them would return false as they had a source_location other than
BUILTINS_LOCATION within the line_map entry that was incorrectly created in
c_finish_options.
>>
>> DECL_IS_BUILTIN is almost never the appropriate thing to use, instead
>> you should use DECL_BUILT_IN (and grepping, I see some suspicious uses
>> ...).
>
> Gah.  Then we need to get rid of one of these two.  Whichever is used
> fewer times, I suppose.

Well, they both make sense - you should just not confuse them.
DECL_IS_BUILTIN should probably renamed to
DECL_HAS_BUILTIN_SOURCE_LOCATION or similar.

I'm testing a patch that fixes some bogus uses.

Richard.

>
> Diego.
>

[Diego Novillo, 2011-08-11 13:37:30]

On Thu, Aug 11, 2011 at 09:32, Richard Guenther
<richard.guenther@gmail.com> wrote:

> Well, they both make sense - you should just not confuse them.
> DECL_IS_BUILTIN should probably renamed to
> DECL_HAS_BUILTIN_SOURCE_LOCATION or similar.

Exactly.  The problem is that now they both *seem* to have the same semantics.

> I'm testing a patch that fixes some bogus uses.

Thanks.


Diego.

[Gabriel Charette, 2011-08-11 16:52:17]

On Thu, Aug 11, 2011 at 7:22 AM, Dodji Seketeli <dodji@seketeli.org> wrote:
> Hello,
>
> gchare@google.com (Gabriel Charette) a écrit:
>
>> diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
>> index 3227f7b..1af8e7b 100644
>> --- a/gcc/c-family/c-opts.c
>> +++ b/gcc/c-family/c-opts.c
>> @@ -1306,13 +1306,15 @@ c_finish_options (void)
>>      {
>>        size_t i;
>>
>> -      cb_file_change (parse_in,
>> -                   linemap_add (line_table, LC_RENAME, 0,
>> -                                _("<built-in>"), 0));
>> +      /* Make sure all of the builtins about to be declared have
>> +         BUILTINS_LOCATION has their source_location.  */
>> +      line_table->forced_location = BUILTINS_LOCATION;
>>
>>        cpp_init_builtins (parse_in, flag_hosted);
>>        c_cpp_builtins (parse_in);
>>
>> +      line_table->forced_location = 0;
>> +
>
> FWIW, Since libcpp already knows about the concept of a builtin macro,
> maybe we could add a "builtin_location" member to cpp_reader, as well as
> "builtin_location" parameter to cpp_init_builtins.  cpp_init_builtin
> would then set pfile->builtin_location and _cpp_define_builtin would
> then make sure the builtins have that builtin_location.
>
> That way, each FE would just have to call
>
>    cpp_init_builtins (parse_i, flasg_hosted, BUILTIN_LOCATION);
>
> for their builtin macros have the proper location.  And the semantics
> would be clearer than what we'd get by setting a "magical"
> line_table->forced_location at that point.
>
> Just my 2 euro cents (that aren't worth much these days)
>

That could work given _cpp_lex_direct does receive a cpp_reader as a
parameter. We would need to add logic in _cpp_lex_direct to support
this new field.

As I mentioned, we have the same problem in pph where we need to force
a location (i.e. the lexer is assigning new locations, but we don't
want it to when we are replaying pre-processor tokens), so just a
"builtin_location" field is potentially insufficient.

Either way, a "magic" field in cpp_reader  handled with logic in
_cpp_lex_direct, or a "magic" field in line_table handled in
linemap_position_for_column.

I agree that the field in cpp_reader might be clearer as the logic is
handled in _cpp_lex_direct, where we want the change to occur, not in
some dark corner of libcpp (linemap_position_for_column).

Gabriel

[Gabriel Charette, 2011-08-11 16:54:24]

On Thu, Aug 11, 2011 at 12:27 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Wed, Aug 10, 2011 at 8:22 PM, Gabriel Charette <gchare@google.com> wrote:
>> There was a bug where c_finish_options would create some builtins and assign
them source_locations in the linemap other than BUILTINS_LOCATION == 1.
>>
>> Thus, when calling DECL_IS_BUILTIN to know if a decl is a builtin, some of
them would return false as they had a source_location other than
BUILTINS_LOCATION within the line_map entry that was incorrectly created in
c_finish_options.
>
> DECL_IS_BUILTIN is almost never the appropriate thing to use, instead
> you should use DECL_BUILT_IN (and grepping, I see some suspicious uses
> ...).

Why don't all builtins have BUILTINS_LOCATION as their location? It
doesn't make sense to me that we need to create a line_table entry for
builtins as they don't have line/col information.

Gabriel

[richard.guenther_gmail.com, 2011-08-11 17:14:27]

On Thu, Aug 11, 2011 at 6:54 PM, Gabriel Charette <gchare@google.com> wrote:
> On Thu, Aug 11, 2011 at 12:27 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Wed, Aug 10, 2011 at 8:22 PM, Gabriel Charette <gchare@google.com> wrote:
>>> There was a bug where c_finish_options would create some builtins and assign
them source_locations in the linemap other than BUILTINS_LOCATION == 1.
>>>
>>> Thus, when calling DECL_IS_BUILTIN to know if a decl is a builtin, some of
them would return false as they had a source_location other than
BUILTINS_LOCATION within the line_map entry that was incorrectly created in
c_finish_options.
>>
>> DECL_IS_BUILTIN is almost never the appropriate thing to use, instead
>> you should use DECL_BUILT_IN (and grepping, I see some suspicious uses
>> ...).
>
> Why don't all builtins have BUILTINS_LOCATION as their location? It
> doesn't make sense to me that we need to create a line_table entry for
> builtins as they don't have line/col information.

Because we want the source location of the actual declaration if there was any,
like when including math.h.

Richard.

> Gabriel
>

[dodji_seketeli.org, 2011-08-11 17:15:26]

Gabriel Charette <gchare@google.com> a écrit:

> On Thu, Aug 11, 2011 at 12:27 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Wed, Aug 10, 2011 at 8:22 PM, Gabriel Charette <gchare@google.com> wrote:
>>> There was a bug where c_finish_options would create some builtins and assign
them source_locations in the linemap other than BUILTINS_LOCATION == 1.
>>>
>>> Thus, when calling DECL_IS_BUILTIN to know if a decl is a builtin, some of
them would return false as they had a source_location other than
BUILTINS_LOCATION within the line_map entry that was incorrectly created in
c_finish_options.
>>
>> DECL_IS_BUILTIN is almost never the appropriate thing to use, instead
>> you should use DECL_BUILT_IN (and grepping, I see some suspicious uses
>> ...).
>
> Why don't all builtins have BUILTINS_LOCATION as their location? It
> doesn't make sense to me that we need to create a line_table entry for
> builtins as they don't have line/col information.

Good observation.  I wonder the same thing.  FWIW, record_builtin_type
in c-decl.c set the location of that the TYPE_DECL of builtin types to
UNKNOWN_LOCATION, whereas the record_builtin_type of cp/decl.c correctly
sets it to BUILTINS_LOCATION.  I think the c-decls.c case should be
changed to use BUILTINS_LOCATION too.

-- 
		Dodji

[dodji_seketeli.org, 2011-08-11 17:45:18]

Gabriel Charette <gchare@google.com> a écrit:

> That could work given _cpp_lex_direct does receive a cpp_reader as a
> parameter. We would need to add logic in _cpp_lex_direct to support
> this new field.

Correct.

>
> As I mentioned, we have the same problem in pph where we need to force
> a location (i.e. the lexer is assigning new locations, but we don't
> want it to when we are replaying pre-processor tokens), so just a
> "builtin_location" field is potentially insufficient.

I see.  So maybe a cpp_reader::forced_token_location, initialized to -1
rather than 0 (in case someone wants to force a location to
UNKNOWN_LOCATION, which is zero for the C/C++ FE at least).  There would
then still be a new parm added to cpp_init_builtins, that would be the
value of the location of the builtin macros to build.  cpp_init_builtins
would then just set cpp_reader::force_token_location to that value.

If the API you are using is the lexer and you want to force the location
of the tokens it's handing out, then I guess setting
cpp_reader::forced_token_location directly is OK, IMHO.

>
> Either way, a "magic" field in cpp_reader  handled with logic in
> _cpp_lex_direct, or a "magic" field in line_table handled in
> linemap_position_for_column.

I guess what I was trying to say is that IMHO the line map module ought
to provide mechanisms for mapping line/column pairs to source locations.
The policy of how/if the source location is assigned to a token should
be left the code using the line map module.  And that new field and the
logic governing it did sound like a adding policy there.

> I agree that the field in cpp_reader might be clearer as the logic is
> handled in _cpp_lex_direct, where we want the change to occur, not in
> some dark corner of libcpp (linemap_position_for_column).

Indeed.

-- 
		Dodji

[Gabriel Charette, 2011-08-11 18:04:01]

On Thu, Aug 11, 2011 at 10:14 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Thu, Aug 11, 2011 at 6:54 PM, Gabriel Charette <gchare@google.com> wrote:
>> On Thu, Aug 11, 2011 at 12:27 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Wed, Aug 10, 2011 at 8:22 PM, Gabriel Charette <gchare@google.com> wrote:
>>>> There was a bug where c_finish_options would create some builtins and
assign them source_locations in the linemap other than BUILTINS_LOCATION == 1.
>>>>
>>>> Thus, when calling DECL_IS_BUILTIN to know if a decl is a builtin, some of
them would return false as they had a source_location other than
BUILTINS_LOCATION within the line_map entry that was incorrectly created in
c_finish_options.
>>>
>>> DECL_IS_BUILTIN is almost never the appropriate thing to use, instead
>>> you should use DECL_BUILT_IN (and grepping, I see some suspicious uses
>>> ...).
>>
>> Why don't all builtins have BUILTINS_LOCATION as their location? It
>> doesn't make sense to me that we need to create a line_table entry for
>> builtins as they don't have line/col information.
>
> Because we want the source location of the actual declaration if there was
any,
> like when including math.h.
>

Ah ok in this case I could see why a builtin would have a
source_location other than BUILTINS_LOCATION, but I don't think that
justifies having a <built-in> entry in the line_table (which was only
used by the builtins created in cpp_init_builtins which is what I'm
patching as I think those builtins should also have
BUILTINS_LOCATION).

The builtins created in math.h I would assume are associated with a
source_location in math.h, and that's OK I think (although this might
be harder for us to handle in pph, I don't think we want to change
this behavior in trunk, Diego?)

In pph we probably still want to serialize those builtins that are
instantiated from includes and not by default anyways. The only
builtins that are constant across every compilation from my
understanding are those which have BUILTINS_LOCATION has their
source_location (and the ones instantiated by cpp_init_builtins should
be part of this set I think).

Gabriel

[Gabriel Charette, 2011-08-11 19:59:49]

On Thu, Aug 11, 2011 at 10:45 AM, Dodji Seketeli <dodji@seketeli.org> wrote:
>> As I mentioned, we have the same problem in pph where we need to force
>> a location (i.e. the lexer is assigning new locations, but we don't
>> want it to when we are replaying pre-processor tokens), so just a
>> "builtin_location" field is potentially insufficient.
>
> I see.  So maybe a cpp_reader::forced_token_location, initialized to -1
> rather than 0 (in case someone wants to force a location to
> UNKNOWN_LOCATION, which is zero for the C/C++ FE at least).  There would
> then still be a new parm added to cpp_init_builtins, that would be the
> value of the location of the builtin macros to build.  cpp_init_builtins
> would then just set cpp_reader::force_token_location to that value.
>

Yes, I did think about using -1 to represent no-force. The problem is,
source_location is defined as unsigned int... So my solution was to
use a source_location* which when non-null means you want to force the
location to the referenced source_location. For some reason however
when I add a simple field `source_location *forced_location` to struct
line_maps I get the following compile error:

libcpp/include/line-map.h:99: field `(*x).forced_location' is pointer
to unimplemented type

Adding a source_location* to cpp_reader compiles fine however. This
must be a tricky corner of the C syntax I'm not aware of...?

Gabriel

[Gabriel Charette, 2011-08-11 20:54:20]

> I also removed LINEMAP_POSITION_FOR_COLUMN, it did the EXACT same thing as
linemap_position_for_column, so maintaining both in parallel seems like overkill
to me. The only thing I can think of is that it's more optimal as it's inlined
(but if that's really needed we can always make linemap_position_for_column an
inline function).
>

This section of the patch is somewhat independent and has not been
discussed in this thread, I pulled it out and submitted a separate
patch (issue4874043) while I'm working on making this patch force
location from cpp_reader.

Gabriel

[Gabriel Charette, 2011-08-16 00:37:33]

Here is the updated patch.

It nows exposes two libcpp functions to force the source_location for tokens
when desired.

The lexer then checks for a value set by these functions in cpp_reader and acts
accordingly when needing a location for a new token (either using the
forced_location or calling the linemap as it used to).

It turns out the fortran library made the same mistake of creating a line_table
entry for builtins, I fixed it as well in this patch.

Tested on x64 for c++,fortran.

(fyi: I moved the removal of LINEMAP_POSITION_FOR_COLUMN to a separate patch
which is checked-in already; thus it doesn't show up in this updated patch
obviously.
)

Ok for trunk?

Gabriel

2011-08-15  Gabriel Charette  <gchare@google.com>

	gcc/c-family/ChangeLog
	* c-opts.c (c_finish_options): Force BUILTINS_LOCATION for tokens
	defined in cpp_init_builtins and c_cpp_builtins.

	gcc/fortran/ChangeLog
	* cpp.c (gfc_cpp_init): Force BUILTINS_LOCATION for tokens
	defined in cpp_define_builtins.

	libcpp/ChangeLog
	* init.c (cpp_create_reader): Inititalize forced_token_location_p.
	* internal.h (struct cpp_reader): Add field forced_token_location_p.
	* lex.c (_cpp_lex_direct): Use forced_token_location_p.
	(cpp_force_token_locations): New.
	(cpp_stop_forcing_token_locations): New.

diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
index 3227f7b..49ff80d 100644
--- a/gcc/c-family/c-opts.c
+++ b/gcc/c-family/c-opts.c
@@ -1306,12 +1306,17 @@ c_finish_options (void)
     {
       size_t i;
 
-      cb_file_change (parse_in,
-		      linemap_add (line_table, LC_RENAME, 0,
-				   _("<built-in>"), 0));
+      {
+	/* Make sure all of the builtins about to be declared have
+	  BUILTINS_LOCATION has their source_location.  */
+	source_location builtins_loc = BUILTINS_LOCATION;
+	cpp_force_token_locations (parse_in, &builtins_loc);
 
-      cpp_init_builtins (parse_in, flag_hosted);
-      c_cpp_builtins (parse_in);
+	cpp_init_builtins (parse_in, flag_hosted);
+	c_cpp_builtins (parse_in);
+
+	cpp_stop_forcing_token_locations (parse_in);
+      }
 
       /* We're about to send user input to cpplib, so make it warn for
 	 things that we previously (when we sent it internal definitions)
diff --git a/gcc/fortran/cpp.c b/gcc/fortran/cpp.c
index a40442e..9368d89 100644
--- a/gcc/fortran/cpp.c
+++ b/gcc/fortran/cpp.c
@@ -565,9 +565,17 @@ gfc_cpp_init (void)
   if (gfc_option.flag_preprocessed)
     return;
 
-  cpp_change_file (cpp_in, LC_RENAME, _("<built-in>"));
   if (!gfc_cpp_option.no_predefined)
-    cpp_define_builtins (cpp_in);
+    {
+      /* Make sure all of the builtins about to be declared have
+	BUILTINS_LOCATION has their source_location.  */
+      source_location builtins_loc = BUILTINS_LOCATION;
+      cpp_force_token_locations (cpp_in, &builtins_loc);
+
+      cpp_define_builtins (cpp_in);
+
+      cpp_stop_forcing_token_locations (cpp_in);
+    }
 
   /* Handle deferred options from command-line.  */
   cpp_change_file (cpp_in, LC_RENAME, _("<command-line>"));
diff --git a/libcpp/include/cpplib.h b/libcpp/include/cpplib.h
index 55b0f1b..6ad0345 100644
--- a/libcpp/include/cpplib.h
+++ b/libcpp/include/cpplib.h
@@ -985,4 +985,8 @@ extern void cpp_prepare_state (cpp_reader *, struct
save_macro_data **);
 extern int cpp_read_state (cpp_reader *, const char *, FILE *,
 			   struct save_macro_data *);
 
+/* In lex.c */
+extern void cpp_force_token_locations (cpp_reader *, source_location *);
+extern void cpp_stop_forcing_token_locations (cpp_reader *);
+
 #endif /* ! LIBCPP_CPPLIB_H */
diff --git a/libcpp/init.c b/libcpp/init.c
index 5ba6666..b3d4c8c 100644
--- a/libcpp/init.c
+++ b/libcpp/init.c
@@ -221,6 +221,9 @@ cpp_create_reader (enum c_lang lang, hash_table *table,
   /* Initialize table for push_macro/pop_macro.  */
   pfile->pushed_macros = 0;
 
+  /* Do not force token locations by default.  */
+  pfile->forced_token_location_p = NULL;
+
   /* The expression parser stack.  */
   _cpp_expand_op_stack (pfile);
 
diff --git a/libcpp/internal.h b/libcpp/internal.h
index d2872c4..6c423f0 100644
--- a/libcpp/internal.h
+++ b/libcpp/internal.h
@@ -499,6 +499,10 @@ struct cpp_reader
 
   /* List of saved macros by push_macro.  */
   struct def_pragma_macro *pushed_macros;
+
+  /* If non-null, the lexer will use this location for the next token
+     instead of getting a location from the linemap.  */
+  source_location *forced_token_location_p;
 };
 
 /* Character classes.  Based on the more primitive macros in safe-ctype.h.
diff --git a/libcpp/lex.c b/libcpp/lex.c
index d460b98..244b14d 100644
--- a/libcpp/lex.c
+++ b/libcpp/lex.c
@@ -1975,8 +1975,11 @@ _cpp_lex_direct (cpp_reader *pfile)
     }
   c = *buffer->cur++;
 
-  result->src_loc = linemap_position_for_column (pfile->line_table,
-			                                    CPP_BUF_COLUMN (buffer, buffer->cur));
+  if (pfile->forced_token_location_p)
+    result->src_loc = *pfile->forced_token_location_p;
+  else
+    result->src_loc = linemap_position_for_column (pfile->line_table,
+					  CPP_BUF_COLUMN (buffer, buffer->cur));
 
   switch (c)
     {
@@ -2837,3 +2840,19 @@ cpp_token_val_index (cpp_token *tok)
       return CPP_TOKEN_FLD_NONE;
     }
 }
+
+/* All tokens lexed in R after calling this function will be forced to have
+   their source_location the same as the location referenced by P, until
+   cpp_stop_forcing_token_locations is called for R.  */
+
+void cpp_force_token_locations (cpp_reader *r, source_location *p)
+{
+  r->forced_token_location_p = p;
+}
+
+/* Go back to assigning locations naturally for lexed tokens.  */
+
+void cpp_stop_forcing_token_locations (cpp_reader *r)
+{
+  r->forced_token_location_p = NULL;
+}

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

[dodji_seketeli.org, 2011-08-17 20:04:29]

Hello Gabriel,

gchare@google.com (Gabriel Charette) a écrit:

> Here is the updated patch.
>
> It nows exposes two libcpp functions to force the source_location for tokens
when desired.
>
> The lexer then checks for a value set by these functions in cpp_reader and
acts accordingly when needing a location for a new token (either using the
forced_location or calling the linemap as it used to).
>
> It turns out the fortran library made the same mistake of creating a
line_table entry for builtins, I fixed it as well in this patch.
>
> Tested on x64 for c++,fortran.
>
> (fyi: I moved the removal of LINEMAP_POSITION_FOR_COLUMN to a separate patch
which is checked-in already; thus it doesn't show up in this updated patch
obviously.
> )
>
> Ok for trunk?
>
> Gabriel
>
> 2011-08-15  Gabriel Charette  <gchare@google.com>
>
> 	gcc/c-family/ChangeLog
> 	* c-opts.c (c_finish_options): Force BUILTINS_LOCATION for tokens
> 	defined in cpp_init_builtins and c_cpp_builtins.
>
> 	gcc/fortran/ChangeLog
> 	* cpp.c (gfc_cpp_init): Force BUILTINS_LOCATION for tokens
> 	defined in cpp_define_builtins.
>
> 	libcpp/ChangeLog
> 	* init.c (cpp_create_reader): Inititalize forced_token_location_p.
> 	* internal.h (struct cpp_reader): Add field forced_token_location_p.
> 	* lex.c (_cpp_lex_direct): Use forced_token_location_p.
> 	(cpp_force_token_locations): New.
> 	(cpp_stop_forcing_token_locations): New.

I cannot approve or reject this patch, but FWIW, it looks OK to me.

Thanks.

-- 
		Dodji

[Gabriel Charette, 2011-08-18 16:47:36]

Tom: ok for trunk?

fortran@: The fortran change just reflects the fix from libcpp,
fortran bootstrap and tests passed.

Thanks,
Gabriel

On Wed, Aug 17, 2011 at 1:04 PM, Dodji Seketeli <dodji@seketeli.org> wrote:
> Hello Gabriel,
>
> gchare@google.com (Gabriel Charette) a écrit:
>
>> Here is the updated patch.
>>
>> It nows exposes two libcpp functions to force the source_location for tokens
when desired.
>>
>> The lexer then checks for a value set by these functions in cpp_reader and
acts accordingly when needing a location for a new token (either using the
forced_location or calling the linemap as it used to).
>>
>> It turns out the fortran library made the same mistake of creating a
line_table entry for builtins, I fixed it as well in this patch.
>>
>> Tested on x64 for c++,fortran.
>>
>> (fyi: I moved the removal of LINEMAP_POSITION_FOR_COLUMN to a separate patch
which is checked-in already; thus it doesn't show up in this updated patch
obviously.
>> )
>>
>> Ok for trunk?
>>
>> Gabriel
>>
>> 2011-08-15  Gabriel Charette  <gchare@google.com>
>>
>>       gcc/c-family/ChangeLog
>>       * c-opts.c (c_finish_options): Force BUILTINS_LOCATION for tokens
>>       defined in cpp_init_builtins and c_cpp_builtins.
>>
>>       gcc/fortran/ChangeLog
>>       * cpp.c (gfc_cpp_init): Force BUILTINS_LOCATION for tokens
>>       defined in cpp_define_builtins.
>>
>>       libcpp/ChangeLog
>>       * init.c (cpp_create_reader): Inititalize forced_token_location_p.
>>       * internal.h (struct cpp_reader): Add field forced_token_location_p.
>>       * lex.c (_cpp_lex_direct): Use forced_token_location_p.
>>       (cpp_force_token_locations): New.
>>       (cpp_stop_forcing_token_locations): New.
>
> I cannot approve or reject this patch, but FWIW, it looks OK to me.
>
> Thanks.
>
> --
>                Dodji
>

[burnus, 2011-08-18 17:11:57]

On 08/18/2011 06:47 PM, Gabriel Charette wrote:
 > Tom: ok for trunk?
 >
 > fortran@: The fortran change just reflects the fix from libcpp,
 > fortran bootstrap and tests passed.

The Fortran change is OK.

Thanks for the patch!

Tobias

On 08/16/2011 02:37 AM, Gabriel Charette wrote:
> Here is the updated patch.
>
> It nows exposes two libcpp functions to force the source_location for tokens
when desired.
>
> The lexer then checks for a value set by these functions in cpp_reader and
acts accordingly when needing a location for a new token (either using the
forced_location or calling the linemap as it used to).
>
> It turns out the fortran library made the same mistake of creating a
line_table entry for builtins, I fixed it as well in this patch.
>
> Tested on x64 for c++,fortran.
> Ok for trunk?
>
> Gabriel
>
> 2011-08-15  Gabriel Charette<gchare@google.com>
>
> 	gcc/c-family/ChangeLog
> 	* c-opts.c (c_finish_options): Force BUILTINS_LOCATION for tokens
> 	defined in cpp_init_builtins and c_cpp_builtins.
>
> 	gcc/fortran/ChangeLog
> 	* cpp.c (gfc_cpp_init): Force BUILTINS_LOCATION for tokens
> 	defined in cpp_define_builtins.
>
> 	libcpp/ChangeLog
> 	* init.c (cpp_create_reader): Inititalize forced_token_location_p.
> 	* internal.h (struct cpp_reader): Add field forced_token_location_p.
> 	* lex.c (_cpp_lex_direct): Use forced_token_location_p.
> 	(cpp_force_token_locations): New.
> 	(cpp_stop_forcing_token_locations): New.
>
> diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
> index 3227f7b..49ff80d 100644
> --- a/gcc/c-family/c-opts.c
> +++ b/gcc/c-family/c-opts.c
> @@ -1306,12 +1306,17 @@ c_finish_options (void)
>       {
>         size_t i;
>
> -      cb_file_change (parse_in,
> -		      linemap_add (line_table, LC_RENAME, 0,
> -				   _("<built-in>"), 0));
> +      {
> +	/* Make sure all of the builtins about to be declared have
> +	  BUILTINS_LOCATION has their source_location.  */
> +	source_location builtins_loc = BUILTINS_LOCATION;
> +	cpp_force_token_locations (parse_in,&builtins_loc);
>
> -      cpp_init_builtins (parse_in, flag_hosted);
> -      c_cpp_builtins (parse_in);
> +	cpp_init_builtins (parse_in, flag_hosted);
> +	c_cpp_builtins (parse_in);
> +
> +	cpp_stop_forcing_token_locations (parse_in);
> +      }
>
>         /* We're about to send user input to cpplib, so make it warn for
>   	 things that we previously (when we sent it internal definitions)
> diff --git a/gcc/fortran/cpp.c b/gcc/fortran/cpp.c
> index a40442e..9368d89 100644
> --- a/gcc/fortran/cpp.c
> +++ b/gcc/fortran/cpp.c
> @@ -565,9 +565,17 @@ gfc_cpp_init (void)
>     if (gfc_option.flag_preprocessed)
>       return;
>
> -  cpp_change_file (cpp_in, LC_RENAME, _("<built-in>"));
>     if (!gfc_cpp_option.no_predefined)
> -    cpp_define_builtins (cpp_in);
> +    {
> +      /* Make sure all of the builtins about to be declared have
> +	BUILTINS_LOCATION has their source_location.  */
> +      source_location builtins_loc = BUILTINS_LOCATION;
> +      cpp_force_token_locations (cpp_in,&builtins_loc);
> +
> +      cpp_define_builtins (cpp_in);
> +
> +      cpp_stop_forcing_token_locations (cpp_in);
> +    }
>
>     /* Handle deferred options from command-line.  */
>     cpp_change_file (cpp_in, LC_RENAME, _("<command-line>"));
[...]

[tromey_redhat.com, 2011-08-19 16:21:32]

>>>>> "Gabriel" == Gabriel Charette <gchare@google.com> writes:

Gabriel> It nows exposes two libcpp functions to force the
Gabriel> source_location for tokens when desired.

I am not really a fan of this approach, but I see why you did it this
way -- anything else would be very invasive.

I can only approve the libcpp parts.

Gabriel> +void cpp_force_token_locations (cpp_reader *r, source_location *p)

Newline after "void".

Gabriel> +void cpp_stop_forcing_token_locations (cpp_reader *r)

Likewise.

The libcpp parts are ok with those changes.

Tom