On Wed, Aug 10, 2011 at 8:22 PM, Gabriel Charette <gch...@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 <gch...@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
>
