Hello- May I please ping this PCH patch? Thanks! https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639467.html
-Lewis On Tue, Dec 5, 2023 at 8:52 PM Lewis Hyatt <lhy...@gmail.com> wrote: > > Hello- > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105608 > > There are two related issues here really, a regression since GCC 11 where we > can ICE after restoring a PCH, and a deeper issue with bogus locations > assigned to macros that were defined prior to restoring a PCH. This patch > fixes the ICE regression with a simple change, and I think it's appropriate > for GCC 14 as well as backport to 11, 12, 13. The bad locations (wrong, but > not generally causing an ICE, and mostly affecting only the output of > -Wunused-macros) are not as problematic, and will be harder to fix. I could > take a stab at that for GCC 15. In the meantime the patch adds XFAILed > tests for the wrong locations (as well as passing tests for the regression > fix). Does it look OK please? Bootstrap + regtest all languages on x86-64 > Linux. Thanks! > > -Lewis > > -- >8 -- > > Users are allowed to define macros prior to restoring a precompiled header > file, as long as those macros are not defined (or are defined identically) > in the PCH. However, the PCH restoration process destroys all the macro > definitions, so libcpp has to record them before restoring the PCH and then > redefine them afterward. > > This process does not currently assign great locations to the macros after > redefining them. Some work is needed to also remember the original locations > and get the line_maps instance in the right state (since, like all other > data structures, the line_maps instance is also reset after restoring a PCH). > The new testcase line-map-3.C contains XFAILed examples where the locations > are wrong. > > This patch addresses a more pressing issue, which is that we ICE in some > cases since GCC 11, hitting an assert in line-maps.cc. It happens if the > first line encountered after the PCH restore requires an LC_RENAME map, such > as will happen if the line is sufficiently long. This is much easier to > fix, since we just need to call linemap_line_start before asking libcpp to > redefine the stored macros, instead of afterward, to avoid the unexpected > need for an LC_RENAME before an LC_ENTER has been seen. > > gcc/c-family/ChangeLog: > > PR preprocessor/105608 > * c-pch.cc (c_common_read_pch): Start a new line map before asking > libcpp to restore macros defined prior to reading the PCH, instead > of afterward. > > gcc/testsuite/ChangeLog: > > PR preprocessor/105608 > * g++.dg/pch/line-map-1.C: New test. > * g++.dg/pch/line-map-1.Hs: New test. > * g++.dg/pch/line-map-2.C: New test. > * g++.dg/pch/line-map-2.Hs: New test. > * g++.dg/pch/line-map-3.C: New test. > * g++.dg/pch/line-map-3.Hs: New test. > --- > gcc/c-family/c-pch.cc | 5 ++--- > gcc/testsuite/g++.dg/pch/line-map-1.C | 4 ++++ > gcc/testsuite/g++.dg/pch/line-map-1.Hs | 1 + > gcc/testsuite/g++.dg/pch/line-map-2.C | 6 ++++++ > gcc/testsuite/g++.dg/pch/line-map-2.Hs | 1 + > gcc/testsuite/g++.dg/pch/line-map-3.C | 23 +++++++++++++++++++++++ > gcc/testsuite/g++.dg/pch/line-map-3.Hs | 1 + > 7 files changed, 38 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/pch/line-map-1.C > create mode 100644 gcc/testsuite/g++.dg/pch/line-map-1.Hs > create mode 100644 gcc/testsuite/g++.dg/pch/line-map-2.C > create mode 100644 gcc/testsuite/g++.dg/pch/line-map-2.Hs > create mode 100644 gcc/testsuite/g++.dg/pch/line-map-3.C > create mode 100644 gcc/testsuite/g++.dg/pch/line-map-3.Hs > > diff --git a/gcc/c-family/c-pch.cc b/gcc/c-family/c-pch.cc > index 2f014fca210..9ee6f179002 100644 > --- a/gcc/c-family/c-pch.cc > +++ b/gcc/c-family/c-pch.cc > @@ -342,6 +342,8 @@ c_common_read_pch (cpp_reader *pfile, const char *name, > gt_pch_restore (f); > cpp_set_line_map (pfile, line_table); > rebuild_location_adhoc_htab (line_table); > + line_table->trace_includes = saved_trace_includes; > + linemap_add (line_table, LC_ENTER, 0, saved_loc.file, saved_loc.line); > > timevar_push (TV_PCH_CPP_RESTORE); > if (cpp_read_state (pfile, name, f, smd) != 0) > @@ -355,9 +357,6 @@ c_common_read_pch (cpp_reader *pfile, const char *name, > > fclose (f); > > - line_table->trace_includes = saved_trace_includes; > - linemap_add (line_table, LC_ENTER, 0, saved_loc.file, saved_loc.line); > - > /* Give the front end a chance to take action after a PCH file has > been loaded. */ > if (lang_post_pch_load) > diff --git a/gcc/testsuite/g++.dg/pch/line-map-1.C > b/gcc/testsuite/g++.dg/pch/line-map-1.C > new file mode 100644 > index 00000000000..9d1ac6d1683 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pch/line-map-1.C > @@ -0,0 +1,4 @@ > +/* PR preprocessor/105608 */ > +/* { dg-do compile } */ > +#define MACRO_ON_A_LONG_LINE "this line is long enough that it forces the > line table to create an LC_RENAME map, which formerly triggered an ICE after > PCH restore" > +#include "line-map-1.H" > diff --git a/gcc/testsuite/g++.dg/pch/line-map-1.Hs > b/gcc/testsuite/g++.dg/pch/line-map-1.Hs > new file mode 100644 > index 00000000000..3b6178bfae0 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pch/line-map-1.Hs > @@ -0,0 +1 @@ > +/* This space intentionally left blank. */ > diff --git a/gcc/testsuite/g++.dg/pch/line-map-2.C > b/gcc/testsuite/g++.dg/pch/line-map-2.C > new file mode 100644 > index 00000000000..0be035781c8 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pch/line-map-2.C > @@ -0,0 +1,6 @@ > +/* PR preprocessor/105608 */ > +/* { dg-do compile } */ > +/* { dg-additional-options "-save-temps" } */ > +#define MACRO_ON_A_LONG_LINE "this line is long enough that it forces the > line table to create an LC_RENAME map, which formerly triggered an ICE after > PCH restore" > +#include "line-map-2.H" > +#error "suppress PCH assembly comparison, which does not work with > -save-temps" /* { dg-error "." } */ > diff --git a/gcc/testsuite/g++.dg/pch/line-map-2.Hs > b/gcc/testsuite/g++.dg/pch/line-map-2.Hs > new file mode 100644 > index 00000000000..3b6178bfae0 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pch/line-map-2.Hs > @@ -0,0 +1 @@ > +/* This space intentionally left blank. */ > diff --git a/gcc/testsuite/g++.dg/pch/line-map-3.C > b/gcc/testsuite/g++.dg/pch/line-map-3.C > new file mode 100644 > index 00000000000..3390d7adba2 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pch/line-map-3.C > @@ -0,0 +1,23 @@ > +#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} } */ > diff --git a/gcc/testsuite/g++.dg/pch/line-map-3.Hs > b/gcc/testsuite/g++.dg/pch/line-map-3.Hs > new file mode 100644 > index 00000000000..3b6178bfae0 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pch/line-map-3.Hs > @@ -0,0 +1 @@ > +/* This space intentionally left blank. */
