| Message ID | 20110810182253.285201C0F8B@gchare.mtv.corp.google.com |
|---|---|
| State | New |
| Headers | show |
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 >
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 >
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.
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. >
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.
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)
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
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
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 >
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.
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.
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
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
> 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
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) {