Re: Linemap force location and remove LINEMAP_POSITION_FOR_COLUMN (issue4801090)

[Gabriel Charette <gchare at google dot com>]

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
>