Hello- https://gcc.gnu.org/pipermail/gcc-patches/2025-July/691235.html
Could I ping this one please? It would be great to close out this PR. Thanks! -Lewis On Sat, Aug 16, 2025 at 8:37 AM Lewis Hyatt <[email protected]> wrote: > > https://gcc.gnu.org/pipermail/gcc-patches/2025-July/691235.html > > Ping please? Thanks! > > -Lewis > > On Wed, Jul 30, 2025 at 7:21 PM Lewis Hyatt <[email protected]> wrote: > > > > Hello- > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105608 > > > > The PR was originally about an ICE in the linemap code that was patched up > > for GCC 14. I left it open as a reminder to finish improving the locations > > assigned to macros that are defined prior to including a PCH. This patch > > provides that improvement... does it look OK please? Bootstrap + regtest all > > languages on x86-64 and aarch64 (Linux). Thanks! > > > > -Lewis > > > > -- >8 -- > > > > It is permissible to define macros prior to including a PCH, as long as > > these definitions are disjoint from or identical to the macros in the > > PCH. The PCH loading process replaces all libcpp data structures with those > > from the PCH, so it is necessary to remember the extra macros separately and > > then restore them after loading the PCH, which all is handled by > > cpp_save_state() and cpp_read_state() in libcpp/pch.cc. The restoration > > process consists of pushing a buffer containing the macro definition and > > then lexing it from there, similar to how a command-line -D option is > > processed. The current implementation does not attempt to set up the > > line_map for this process, and so the locations assigned to the macros are > > often not meaningful. (Similar to what happened in the past with lexing the > > tokens out of a _Pragma string, lexing out of a buffer rather than a file > > produces "sorta" reasonable locations that are often close enough, but not > > reliably correct.) > > > > Fix that up by remembering enough additional information (more or less, an > > expanded_location for each macro definition) to produce a reasonable > > location for the newly restored macros. > > > > One issue that came up is the treatment of command-line-defined macros. From > > the perspective of the generic line_map data structures, the command-line > > location is not distinguishable from other locations; it's just an ordinary > > location created by the front ends with a fake file name by convention. (At > > the moment, it is always the string `<command-line>', subject to > > translation.) Since libcpp needs to assign macros to that location, it > > needs to know what location to use, so I added a new member > > line_maps::cmdline_location for the front ends to set, similar to how > > line_maps::builtin_location is handled. > > > > This revealed a small issue, in c-opts.cc we have: > > > > /* All command line defines must have the same location. */ > > cpp_force_token_locations (parse_in, line_table->highest_line); > > > > But contrary to the comment, all command line defines don't actually end up > > with the same location anymore. This is because libcpp/lex.cc has been > > expanded (r6-4873) to include range information on the returned > > locations. That logic has never been respecting the request of > > cpp_force_token_locations. I believe this was not intentional, and so I have > > corrected that here. Prior to this patch, the range logic has been leading > > to command-line macros all having similar locations in the same line map (or > > ad-hoc locations based from there for sufficiently long tokens); with this > > change, they all have exactly the same location and that location is > > recorded in line_maps::cmdline_location. > > > > With that change, then it works fine for pch.cc to restore macros whether > > they came from the command-line or from the main file. > > > > gcc/c-family/ChangeLog: > > > > PR preprocessor/105608 > > * c-opts.cc (c_finish_options): Set new member > > line_table->cmdline_location. > > * c-pch.cc (c_common_read_pch): Adapt linemap usage to changes in > > libcpp pch.cc; it is now possible that the linemap is in a different > > file after returning from cpp_read_state(). > > > > libcpp/ChangeLog: > > > > PR preprocessor/105608 > > * include/line-map.h: Add new member CMDLINE_LOCATION. > > * lex.cc (get_location_for_byte_range_in_cur_line): Do not expand > > the token location to include range information if token location > > override was requested. > > (warn_about_normalization): Likewise. > > (_cpp_lex_direct): Likewise. > > * pch.cc (struct saved_macro): New local struct. > > (struct save_macro_data): Change DEFNS vector to hold saved_macro > > rather than uchar*. > > (save_macros): Adapt to remember the location information for each > > saved macro in addition to the definition. > > (cpp_prepare_state): Likewise. > > (cpp_read_state): Use the saved location information to generate > > proper locations for the restored macros. > > > > gcc/testsuite/ChangeLog: > > > > PR preprocessor/105608 > > * g++.dg/pch/line-map-3.C: Remove xfails. > > * g++.dg/pch/line-map-4.C: New test. > > * g++.dg/pch/line-map-4.Hs: New test. > > --- > > libcpp/include/line-map.h | 4 + > > libcpp/lex.cc | 6 +- > > libcpp/pch.cc | 125 ++++++++++++++++++++----- > > gcc/c-family/c-opts.cc | 1 + > > gcc/c-family/c-pch.cc | 12 +-- > > gcc/testsuite/g++.dg/pch/line-map-3.C | 26 +---- > > gcc/testsuite/g++.dg/pch/line-map-4.C | 5 + > > gcc/testsuite/g++.dg/pch/line-map-4.Hs | 1 + > > 8 files changed, 130 insertions(+), 50 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/pch/line-map-4.C > > create mode 100644 gcc/testsuite/g++.dg/pch/line-map-4.Hs > > > > diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h > > index 21a59af2236..b4560cf3737 100644 > > --- a/libcpp/include/line-map.h > > +++ b/libcpp/include/line-map.h > > @@ -878,6 +878,10 @@ public: > > built-in tokens. */ > > location_t builtin_location; > > > > + /* The special location value to be used for tokens originating on the > > + command line. */ > > + location_t cmdline_location; > > + > > /* The default value of range_bits in ordinary line maps. */ > > unsigned int default_range_bits; > > > > diff --git a/libcpp/lex.cc b/libcpp/lex.cc > > index e7705a64f39..2474d028cf8 100644 > > --- a/libcpp/lex.cc > > +++ b/libcpp/lex.cc > > @@ -1353,6 +1353,8 @@ get_location_for_byte_range_in_cur_line (cpp_reader > > *pfile, > > const unsigned char *const start, > > size_t num_bytes) > > { > > + if (pfile->forced_token_location) > > + return pfile->forced_token_location; > > gcc_checking_assert (num_bytes > 0); > > > > /* CPP_BUF_COLUMN and linemap_position_for_column both refer > > @@ -2035,6 +2037,7 @@ warn_about_normalization (cpp_reader *pfile, > > /* If possible, create a location range for the token. */ > > if (loc >= RESERVED_LOCATION_COUNT > > && token->type != CPP_EOF > > + && !pfile->forced_token_location > > /* There must be no line notes to process. */ > > && (!(pfile->buffer->cur > > >= pfile->buffer->notes[pfile->buffer->cur_note].pos > > @@ -4401,7 +4404,8 @@ _cpp_lex_direct (cpp_reader *pfile) > > > > /* Potentially convert the location of the token to a range. */ > > if (result->src_loc >= RESERVED_LOCATION_COUNT > > - && result->type != CPP_EOF) > > + && result->type != CPP_EOF > > + && !pfile->forced_token_location) > > { > > /* Ensure that any line notes are processed, so that we have the > > correct physical line/column for the end-point of the token even > > diff --git a/libcpp/pch.cc b/libcpp/pch.cc > > index fa5efc9f102..495fe7f434b 100644 > > --- a/libcpp/pch.cc > > +++ b/libcpp/pch.cc > > @@ -730,9 +730,20 @@ cpp_valid_state (cpp_reader *r, const char *name, int > > fd) > > > > /* Save all the existing macros. */ > > > > +namespace { > > + > > +struct saved_macro > > +{ > > + uchar *defn; > > + location_t loc; > > + expanded_location xloc; > > +}; > > + > > +} /* unnamed namespace */ > > + > > struct save_macro_data > > { > > - uchar **defns; > > + saved_macro *defns; > > size_t count; > > size_t array_size; > > char **saved_pragmas; > > @@ -762,14 +773,29 @@ save_macros (cpp_reader *r, cpp_hashnode *h, void > > *data_p) > > if (data->count == data->array_size) > > { > > data->array_size *= 2; > > - data->defns = XRESIZEVEC (uchar *, data->defns, > > (data->array_size)); > > + data->defns = XRESIZEVEC (saved_macro, data->defns, > > data->array_size); > > } > > > > const uchar * defn = cpp_macro_definition (r, h); > > size_t defnlen = ustrlen (defn); > > > > - data->defns[data->count] = (uchar *) xmemdup (defn, defnlen, defnlen > > + 2); > > - data->defns[data->count][defnlen] = '\n'; > > + const auto d = data->defns + data->count; > > + d->defn = (uchar *) xmemdup (defn, defnlen, defnlen + 2); > > + d->defn[defnlen] = '\n'; > > + d->loc = h->value.macro->line; > > + d->xloc = {}; > > + if (d->loc == r->line_table->cmdline_location) > > + { > > + /* The cmdline_location may be different when it comes time to > > + restore the macros, so use 0 to indicate this. */ > > + d->loc = 0; > > + } > > + else if (d->loc != r->line_table->builtin_location) > > + { > > + const auto map = linemap_lookup (r->line_table, d->loc); > > + gcc_assert (map); > > + d->xloc = linemap_expand_location (r->line_table, map, d->loc); > > + } > > data->count++; > > } > > > > @@ -785,9 +811,22 @@ cpp_prepare_state (cpp_reader *r, struct > > save_macro_data **data) > > struct save_macro_data *d = XNEW (struct save_macro_data); > > > > d->array_size = 512; > > - d->defns = XNEWVEC (uchar *, d->array_size); > > + d->defns = XNEWVEC (saved_macro, d->array_size); > > d->count = 0; > > cpp_forall_identifiers (r, save_macros, d); > > + > > + /* Sort the saved macros in order, so the line map gets built up > > + in chronological order as expected when we load them back in. */ > > + static line_maps *line_table; > > + line_table = r->line_table; > > + qsort (d->defns, d->count, sizeof (saved_macro), > > + [] (const void *x1, const void *x2) > > + { > > + const auto d1 = static_cast<const saved_macro *> (x1); > > + const auto d2 = static_cast<const saved_macro *> (x2); > > + return -linemap_compare_locations (line_table, d1->loc, d2->loc); > > + }); > > + > > d->saved_pragmas = _cpp_save_pragma_names (r); > > *data = d; > > } > > @@ -820,16 +859,16 @@ cpp_read_state (cpp_reader *r, const char *name, FILE > > *f, > > r->state.prevent_expansion = 1; > > r->state.angled_headers = 0; > > > > + const auto old_directive_line = r->directive_line; > > + > > /* Run through the carefully-saved macros, insert them. */ > > + const char *cur_fname = nullptr; > > for (i = 0; i < data->count; i++) > > { > > - cpp_hashnode *h; > > - size_t namelen; > > - uchar *defn; > > - > > - namelen = ustrcspn (data->defns[i], "( \n"); > > - h = cpp_lookup (r, data->defns[i], namelen); > > - defn = data->defns[i] + namelen; > > + const auto d = data->defns + i; > > + const size_t namelen = ustrcspn (d->defn, "( \n"); > > + cpp_hashnode *const h = cpp_lookup (r, d->defn, namelen); > > + const auto defn = d->defn + namelen; > > > > /* The PCH file is valid, so we know that if there is a definition > > from the PCH file it must be the same as the one we had > > @@ -839,28 +878,72 @@ cpp_read_state (cpp_reader *r, const char *name, FILE > > *f, > > if (cpp_push_buffer (r, defn, ustrchr (defn, '\n') - defn, true) > > != NULL) > > { > > - _cpp_clean_line (r); > > + if (d->loc == r->line_table->builtin_location) > > + r->directive_line = r->line_table->builtin_location; > > + else if (d->loc == 0) > > + /* This was a command-line-defined macro, preserve that > > + aspect now. */ > > + r->directive_line = r->line_table->cmdline_location; > > + else > > + { > > + /* In practice, almost all of the locations ought to be > > in the > > + main file, since a PCH cannot be loaded after including > > + other headers. The (probably sole) exception is that > > + implicit preincludes can be loaded, so if an implicit > > + preinclude such as /usr/include/stdc-predef.h has > > changed > > + since the PCH was created (in a way not invalidating > > the > > + PCH), we can end up here with locations that are not > > in the > > + main file. In that case, we may be adding them to the > > + line_map out of order (the implicit preinclude already > > + being represented in the line_map we have loaded from > > the > > + PCH), but it is probably not going to cause any > > observable > > + issue given the constraints on what can appear in a > > header > > + before loading a PCH. */ > > + if (!cur_fname || strcmp (cur_fname, d->xloc.file)) > > + { > > + linemap_add (r->line_table, LC_RENAME, d->xloc.sysp, > > + d->xloc.file, d->xloc.line); > > + cur_fname = d->xloc.file; > > + } > > + r->directive_line = linemap_line_start (r->line_table, > > + d->xloc.line, > > 127); > > + } > > + > > + /* Even when the macro was defined in the main file, we have > > to > > + use cpp_force_token_locations here, so all tokens in the > > macro > > + will have location assigned to the directive line with no > > + column information. We no longer know even which line the > > + tokens appeared on originally now, much less the column, > > since > > + we only stored the macro definition string after lexing > > it. */ > > + cpp_force_token_locations (r, r->directive_line); > > > > - /* ??? Using r->line_table->highest_line is not ideal here, > > but we > > - do need to use some location that is relative to the new > > line > > - map just loaded, not the old one that was in effect when > > these > > - macros were lexed. The proper fix is to remember the file > > name > > - and line number where each macro was defined, and then add > > - these locations into the new line map. See PR105608. */ > > - if (!_cpp_create_definition (r, h, > > r->line_table->highest_line)) > > + _cpp_clean_line (r); > > + if (!_cpp_create_definition (r, h, r->directive_line)) > > abort (); > > + cpp_stop_forcing_token_locations (r); > > _cpp_pop_buffer (r); > > + > > + if (d->loc < RESERVED_LOCATION_COUNT) > > + /* Macros defined on the command line or as builtins are > > always > > + implicitly used. It is necessary to note this here or > > else > > + -Wunused-macros will complain, because these locations do > > + return TRUE for MAIN_FILE_P(). For a command line > > + definition, d->loc was set to 0 when we saved it, so it > > does > > + satisfy the check performed here. */ > > + h->value.macro->used = true; > > } > > else > > abort (); > > } > > > > - free (data->defns[i]); > > + free (data->defns[i].defn); > > } > > r->state = old_state; > > + r->directive_line = old_directive_line; > > > > _cpp_restore_pragma_names (r, data->saved_pragmas); > > > > + XDELETEVEC (data->defns); > > free (data); > > > > if (deps_restore (r->deps, f, CPP_OPTION (r, restore_pch_deps) ? name : > > NULL) > > diff --git a/gcc/c-family/c-opts.cc b/gcc/c-family/c-opts.cc > > index c652e82a8c7..f8011071103 100644 > > --- a/gcc/c-family/c-opts.cc > > +++ b/gcc/c-family/c-opts.cc > > @@ -1681,6 +1681,7 @@ c_finish_options (void) > > bool cxx_assert_seen_p = false; > > > > /* All command line defines must have the same location. */ > > + line_table->cmdline_location = line_table->highest_line; > > cpp_force_token_locations (parse_in, line_table->highest_line); > > for (size_t i = 0; i < deferred_count; i++) > > { > > diff --git a/gcc/c-family/c-pch.cc b/gcc/c-family/c-pch.cc > > index b8f075e29f7..8f0fea82cad 100644 > > --- a/gcc/c-family/c-pch.cc > > +++ b/gcc/c-family/c-pch.cc > > @@ -347,18 +347,18 @@ c_common_read_pch (cpp_reader *pfile, const char > > *name, > > rebuild_location_adhoc_htab (line_table); > > line_table->trace_includes = saved_trace_includes; > > > > - /* Set the current location to the line containing the #include (or the > > - #pragma GCC pch_preprocess) for the purpose of assigning locations to > > any > > - macros that are about to be restored. */ > > - linemap_add (line_table, LC_ENTER, 0, saved_loc.file, > > - saved_loc.line > 1 ? saved_loc.line - 1 : saved_loc.line); > > + /* Set the line_map current location to the start of the file, so that > > things > > + remain in order after cpp_read_state() re-adds any macros that were > > defined > > + prior to calling gt_pch_restore(). */ > > + linemap_add (line_table, LC_ENTER, saved_loc.sysp, saved_loc.file, 0); > > > > timevar_push (TV_PCH_CPP_RESTORE); > > cpp_result = cpp_read_state (pfile, name, f, smd); > > > > /* Set the current location to the line following the #include, where we > > were prior to processing the PCH. */ > > - linemap_line_start (line_table, saved_loc.line, 0); > > + linemap_add (line_table, LC_RENAME, saved_loc.sysp, saved_loc.file, > > + saved_loc.line); > > > > timevar_pop (TV_PCH_CPP_RESTORE); > > > > diff --git a/gcc/testsuite/g++.dg/pch/line-map-3.C > > b/gcc/testsuite/g++.dg/pch/line-map-3.C > > index 3390d7adba2..9def08de974 100644 > > --- a/gcc/testsuite/g++.dg/pch/line-map-3.C > > +++ b/gcc/testsuite/g++.dg/pch/line-map-3.C > > @@ -1,23 +1,5 @@ > > -#define UNUSED_MACRO /* { dg-error "UNUSED_MACRO" "" { xfail *-*-* } } */ > > -#include "line-map-3.H" /* { dg-bogus "-:UNUSED_MACRO" "" { xfail *-*-* } > > } */ > > - > > -/* { dg-do compile } */ > > -/* { dg-additional-options "-Werror=unused-macros" } */ > > - > > /* PR preprocessor/105608 */ > > -/* This test case is currently xfailed and requires work in libcpp/pch.cc > > to > > - resolve. Currently, the macro location is incorrectly assigned to line > > 2 > > - of the header file when read via PCH, because libcpp doesn't try to > > - assign locations relative to the newly loaded line map after restoring > > - the PCH. */ > > - > > -/* In PCH mode we also complain incorrectly about the command line macro > > -Dwith_PCH > > - added by dejagnu; that warning would get suppressed if the macro > > location were > > - correctly restored by libcpp to reflect that it was a command line > > macro. */ > > -/* { dg-bogus "-:with_PCH" "" { xfail *-*-* } 2 } */ > > - > > -/* The reason we used -Werror was to prevent pch.exp from rerunning > > without PCH; > > - in that case we would get unnecessary XPASS outputs since the test does > > work > > - fine without PCH. Once the bug is fixed, remove the -Werror and switch > > to > > - dg-warning. */ > > -/* { dg-regexp {[^[:space:]]*: some warnings being treated as errors} } */ > > +/* { dg-do compile } */ > > +/* { dg-additional-options "-Wunused-macros" } */ > > +#define UNUSED_MACRO /* { dg-warning "-:UNUSED_MACRO" "" } */ > > +#include "line-map-3.H" /* { dg-bogus "-:UNUSED_MACRO" "" } */ > > diff --git a/gcc/testsuite/g++.dg/pch/line-map-4.C > > b/gcc/testsuite/g++.dg/pch/line-map-4.C > > new file mode 100644 > > index 00000000000..cc13a467ca1 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/pch/line-map-4.C > > @@ -0,0 +1,5 @@ > > +/* PR preprocessor/105608 */ > > +/* { dg-do compile } */ > > +#define INT int /* { dg-error "-:declaration does not declare anything" } > > */ > > +#include "line-map-4.H" > > +INT; /* { dg-note "in expansion of macro" } */ > > diff --git a/gcc/testsuite/g++.dg/pch/line-map-4.Hs > > b/gcc/testsuite/g++.dg/pch/line-map-4.Hs > > new file mode 100644 > > index 00000000000..3b6178bfae0 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/pch/line-map-4.Hs > > @@ -0,0 +1 @@ > > +/* This space intentionally left blank. */
