c-family: Fix ICE with large column number after restoring a PCH [PR105608]
Checks
Commit Message
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
Comments
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 <lhyatt@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. */
Hello-
May I please ping this small patch? Thanks....
https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639467.html
-Lewis
On Wed, Dec 20, 2023 at 8:02 PM Lewis Hyatt <lhyatt@gmail.com> wrote:
>
> 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 <lhyatt@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. */
On 12/5/23 20:52, Lewis Hyatt 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!
OK for trunk and branches, thanks!
> -- >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. */
>
On Fri, Jan 26, 2024 at 04:16:54PM -0500, Jason Merrill wrote:
> On 12/5/23 20:52, Lewis Hyatt 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!
>
> OK for trunk and branches, thanks!
>
Thanks for the review! That is all taken care of. I have one more request if
you don't mind please... There have been some further comments on the PR
indicating that the new xfailed testcase I added is failing in an unexpected
way on at least one architecture. To recap, the idea here was that
1) libcpp needs new logic to be able to output correct locations for this
case. That will be some new code that is suitable for stage 1, not now.
2) In the meantime, we fixed things up enough to avoid an ICE that showed up
in GCC 11, and added an xfailed testcase to remind about #1.
The problem is that, the reason that libcpp outputs the wrong locations, is
that it has always used a location from the old line_map instance to index
into the new line_map instance, and so the exact details of the wrong
locations it outputs depend on the state of those two line maps, which may
differ depending on system includes and things like that. So I was hoping to
make one further one-line change to libcpp, not yet to output correct
locations, but at least to output one which is the same always and doesn't
depend on random things. This would assign all restored macros to a
consistent location, one line following the #include that triggered the PCH
process. I think this probably shouldn't be backported but it would be nice
to get into GCC 14, while nothing critical, at least it would avoid the new
test failure that's being reported. But more generally, I think using a
location from a totally different line map is dangerous and could have worse
consequences that haven't been seen yet. Does it look OK please? Thanks!
-Lewis
[PATCH] libcpp: Stabilize the location for macros restored after PCH [PR105608]
libcpp currently lacks the infrastructure to assign correct locations to
macros that were defined prior to loading a PCH and then restored
afterwards. While I plan to address that for GCC 15, this one-line patch
improves things by using at least a valid location, even if it's not the
best one. Without this change, libcpp uses pfile->directive_line as the
location for the restored macros, but this location_t applies to the old
line map, not the one that was just restored from the PCH, so the resulting
location is unpredictable and depends on what was stored in the line maps
before. With this change, all restored macros get assigned locations at
line_table->highest_line, which is the first line after the #include that
triggered the PCH restore. A future patch will store the actual file name
and line number of the definition and then synthesize locations in the new
line map pointing to the right place.
libcpp/ChangeLog:
PR preprocessor/105608
* pch.cc (cpp_read_state): Set a valid location for restored
macros.
gcc/testsuite/ChangeLog:
PR preprocessor/105608
* g++.dg/pch/line-map-3.C: Adjust to expect the new location.
diff --git a/libcpp/pch.cc b/libcpp/pch.cc
index e156fe257b3..f2f74ed6ea9 100644
--- a/libcpp/pch.cc
+++ b/libcpp/pch.cc
@@ -838,7 +838,14 @@ cpp_read_state (cpp_reader *r, const char *name, FILE *f,
!= NULL)
{
_cpp_clean_line (r);
- if (!_cpp_create_definition (r, h, 0))
+
+ /* ??? 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))
abort ();
_cpp_pop_buffer (r);
}
diff --git a/gcc/testsuite/g++.dg/pch/line-map-3.C b/gcc/testsuite/g++.dg/pch/line-map-3.C
index 3390d7adba2..1ebabd0a5bf 100644
--- a/gcc/testsuite/g++.dg/pch/line-map-3.C
+++ b/gcc/testsuite/g++.dg/pch/line-map-3.C
@@ -1,5 +1,7 @@
#define UNUSED_MACRO /* { dg-error "UNUSED_MACRO" "" { xfail *-*-* } } */
-#include "line-map-3.H" /* { dg-bogus "-:UNUSED_MACRO" "" { xfail *-*-* } } */
+#include "line-map-3.H"
+/* { dg-line wrong_line } */
+/* { dg-bogus "-:UNUSED_MACRO" "" { xfail *-*-* } wrong_line } */
/* { dg-do compile } */
/* { dg-additional-options "-Werror=unused-macros" } */
@@ -14,7 +16,7 @@
/* 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 } */
+/* { dg-bogus "-:with_PCH" "" { xfail *-*-* } wrong_line } */
/* 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
On 1/30/24 21:49, Lewis Hyatt wrote:
> On Fri, Jan 26, 2024 at 04:16:54PM -0500, Jason Merrill wrote:
>> On 12/5/23 20:52, Lewis Hyatt 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!
>>
>> OK for trunk and branches, thanks!
>>
>
> Thanks for the review! That is all taken care of. I have one more request if
> you don't mind please... There have been some further comments on the PR
> indicating that the new xfailed testcase I added is failing in an unexpected
> way on at least one architecture. To recap, the idea here was that
>
> 1) libcpp needs new logic to be able to output correct locations for this
> case. That will be some new code that is suitable for stage 1, not now.
>
> 2) In the meantime, we fixed things up enough to avoid an ICE that showed up
> in GCC 11, and added an xfailed testcase to remind about #1.
>
> The problem is that, the reason that libcpp outputs the wrong locations, is
> that it has always used a location from the old line_map instance to index
> into the new line_map instance, and so the exact details of the wrong
> locations it outputs depend on the state of those two line maps, which may
> differ depending on system includes and things like that. So I was hoping to
> make one further one-line change to libcpp, not yet to output correct
> locations, but at least to output one which is the same always and doesn't
> depend on random things. This would assign all restored macros to a
> consistent location, one line following the #include that triggered the PCH
> process. I think this probably shouldn't be backported but it would be nice
> to get into GCC 14, while nothing critical, at least it would avoid the new
> test failure that's being reported. But more generally, I think using a
> location from a totally different line map is dangerous and could have worse
> consequences that haven't been seen yet. Does it look OK please? Thanks!
Can we use the line of the #include, as the test expects, rather than
the following line?
Jason
On Wed, Jan 31, 2024 at 03:18:01PM -0500, Jason Merrill wrote:
> On 1/30/24 21:49, Lewis Hyatt wrote:
> > On Fri, Jan 26, 2024 at 04:16:54PM -0500, Jason Merrill wrote:
> > > On 12/5/23 20:52, Lewis Hyatt 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!
> > >
> > > OK for trunk and branches, thanks!
> > >
> >
> > Thanks for the review! That is all taken care of. I have one more request if
> > you don't mind please... There have been some further comments on the PR
> > indicating that the new xfailed testcase I added is failing in an unexpected
> > way on at least one architecture. To recap, the idea here was that
> >
> > 1) libcpp needs new logic to be able to output correct locations for this
> > case. That will be some new code that is suitable for stage 1, not now.
> >
> > 2) In the meantime, we fixed things up enough to avoid an ICE that showed up
> > in GCC 11, and added an xfailed testcase to remind about #1.
> >
> > The problem is that, the reason that libcpp outputs the wrong locations, is
> > that it has always used a location from the old line_map instance to index
> > into the new line_map instance, and so the exact details of the wrong
> > locations it outputs depend on the state of those two line maps, which may
> > differ depending on system includes and things like that. So I was hoping to
> > make one further one-line change to libcpp, not yet to output correct
> > locations, but at least to output one which is the same always and doesn't
> > depend on random things. This would assign all restored macros to a
> > consistent location, one line following the #include that triggered the PCH
> > process. I think this probably shouldn't be backported but it would be nice
> > to get into GCC 14, while nothing critical, at least it would avoid the new
> > test failure that's being reported. But more generally, I think using a
> > location from a totally different line map is dangerous and could have worse
> > consequences that haven't been seen yet. Does it look OK please? Thanks!
>
> Can we use the line of the #include, as the test expects, rather than the
> following line?
Thanks, yes, that will work too, it just needs a few changes to
c-family/c-pch.cc to set the location there and then increment it
after. Patch which does that is attached. (This is a new one based on
master, not incremental to the prior patch.) The testcase does not require
any changes this way, and bootstrap + regtest looks good.
-Lewis
[PATCH] c-family: Stabilize the location for macros restored after PCH load [PR105608]
libcpp currently lacks the infrastructure to assign correct locations to
macros that were defined prior to loading a PCH and then restored
afterwards. While I plan to address that fully for GCC 15, this patch
improves things by using at least a valid location, even if it's not the
best one. Without this change, libcpp uses pfile->directive_line as the
location for the restored macros, but this location_t applies to the old
line map, not the one that was just restored from the PCH, so the resulting
location is unpredictable and depends on what was stored in the line maps
before. With this change, all restored macros get assigned locations at the
line of the #include that triggered the PCH restore. A future patch will
store the actual file name and line number of each definition and then
synthesize locations in the new line map pointing to the right place.
gcc/c-family/ChangeLog:
PR preprocessor/105608
* c-pch.cc (c_common_read_pch): Adjust line map so that libcpp
assigns a location to restored macros which is the same location
that triggered the PCH include.
libcpp/ChangeLog:
PR preprocessor/105608
* pch.cc (cpp_read_state): Set a valid location for restored
macros.
diff --git a/gcc/c-family/c-pch.cc b/gcc/c-family/c-pch.cc
index 79b4f88fe4d..971af90be0d 100644
--- a/gcc/c-family/c-pch.cc
+++ b/gcc/c-family/c-pch.cc
@@ -318,6 +318,7 @@ c_common_read_pch (cpp_reader *pfile, const char *name,
struct save_macro_data *smd;
expanded_location saved_loc;
bool saved_trace_includes;
+ int cpp_result;
timevar_push (TV_PCH_RESTORE);
@@ -343,20 +344,26 @@ c_common_read_pch (cpp_reader *pfile, const char *name,
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);
+
+ /* Set the current location to the line containing the #include (or the
+ #pragma GCC_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);
timevar_push (TV_PCH_CPP_RESTORE);
- if (cpp_read_state (pfile, name, f, smd) != 0)
- {
- fclose (f);
- timevar_pop (TV_PCH_CPP_RESTORE);
- goto end;
- }
- timevar_pop (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);
+ timevar_pop (TV_PCH_CPP_RESTORE);
fclose (f);
+ if (cpp_result != 0)
+ goto end;
+
/* Give the front end a chance to take action after a PCH file has
been loaded. */
if (lang_post_pch_load)
diff --git a/libcpp/pch.cc b/libcpp/pch.cc
index e156fe257b3..f2f74ed6ea9 100644
--- a/libcpp/pch.cc
+++ b/libcpp/pch.cc
@@ -838,7 +838,14 @@ cpp_read_state (cpp_reader *r, const char *name, FILE *f,
!= NULL)
{
_cpp_clean_line (r);
- if (!_cpp_create_definition (r, h, 0))
+
+ /* ??? 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))
abort ();
_cpp_pop_buffer (r);
}
On 1/31/24 21:09, Lewis Hyatt wrote:
> On Wed, Jan 31, 2024 at 03:18:01PM -0500, Jason Merrill wrote:
>> On 1/30/24 21:49, Lewis Hyatt wrote:
>>> On Fri, Jan 26, 2024 at 04:16:54PM -0500, Jason Merrill wrote:
>>>> On 12/5/23 20:52, Lewis Hyatt 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!
>>>>
>>>> OK for trunk and branches, thanks!
>>>>
>>>
>>> Thanks for the review! That is all taken care of. I have one more request if
>>> you don't mind please... There have been some further comments on the PR
>>> indicating that the new xfailed testcase I added is failing in an unexpected
>>> way on at least one architecture. To recap, the idea here was that
>>>
>>> 1) libcpp needs new logic to be able to output correct locations for this
>>> case. That will be some new code that is suitable for stage 1, not now.
>>>
>>> 2) In the meantime, we fixed things up enough to avoid an ICE that showed up
>>> in GCC 11, and added an xfailed testcase to remind about #1.
>>>
>>> The problem is that, the reason that libcpp outputs the wrong locations, is
>>> that it has always used a location from the old line_map instance to index
>>> into the new line_map instance, and so the exact details of the wrong
>>> locations it outputs depend on the state of those two line maps, which may
>>> differ depending on system includes and things like that. So I was hoping to
>>> make one further one-line change to libcpp, not yet to output correct
>>> locations, but at least to output one which is the same always and doesn't
>>> depend on random things. This would assign all restored macros to a
>>> consistent location, one line following the #include that triggered the PCH
>>> process. I think this probably shouldn't be backported but it would be nice
>>> to get into GCC 14, while nothing critical, at least it would avoid the new
>>> test failure that's being reported. But more generally, I think using a
>>> location from a totally different line map is dangerous and could have worse
>>> consequences that haven't been seen yet. Does it look OK please? Thanks!
>>
>> Can we use the line of the #include, as the test expects, rather than the
>> following line?
>
> Thanks, yes, that will work too, it just needs a few changes to
> c-family/c-pch.cc to set the location there and then increment it
> after. Patch which does that is attached. (This is a new one based on
> master, not incremental to the prior patch.) The testcase does not require
> any changes this way, and bootstrap + regtest looks good.
OK, thanks.
Jason
Hi Lewis,
> On Fri, Jan 26, 2024 at 04:16:54PM -0500, Jason Merrill wrote:
>> On 12/5/23 20:52, Lewis Hyatt 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!
>>
>> OK for trunk and branches, thanks!
>>
>
> Thanks for the review! That is all taken care of. I have one more request if
> you don't mind please... There have been some further comments on the PR
> indicating that the new xfailed testcase I added is failing in an unexpected
> way on at least one architecture. To recap, the idea here was that
>
> 1) libcpp needs new logic to be able to output correct locations for this
> case. That will be some new code that is suitable for stage 1, not now.
>
> 2) In the meantime, we fixed things up enough to avoid an ICE that showed up
> in GCC 11, and added an xfailed testcase to remind about #1.
>
> The problem is that, the reason that libcpp outputs the wrong locations, is
> that it has always used a location from the old line_map instance to index
> into the new line_map instance, and so the exact details of the wrong
> locations it outputs depend on the state of those two line maps, which may
> differ depending on system includes and things like that. So I was hoping to
> make one further one-line change to libcpp, not yet to output correct
> locations, but at least to output one which is the same always and doesn't
> depend on random things. This would assign all restored macros to a
> consistent location, one line following the #include that triggered the PCH
> process. I think this probably shouldn't be backported but it would be nice
> to get into GCC 14, while nothing critical, at least it would avoid the new
> test failure that's being reported. But more generally, I think using a
> location from a totally different line map is dangerous and could have worse
> consequences that haven't been seen yet. Does it look OK please? Thanks!
FWIW, I've tested this (the initial) version of this patch on
sparc-sun-solaris2.11 (PASSes as before) and i386-pc-solaris2.11 (PASSes
now unlike before).
Thanks.
Rainer
On Thu, Feb 1, 2024 at 7:24 AM Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote:
>
> Hi Lewis,
>
> > On Fri, Jan 26, 2024 at 04:16:54PM -0500, Jason Merrill wrote:
> >> On 12/5/23 20:52, Lewis Hyatt 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!
> >>
> >> OK for trunk and branches, thanks!
> >>
> >
> > Thanks for the review! That is all taken care of. I have one more request if
> > you don't mind please... There have been some further comments on the PR
> > indicating that the new xfailed testcase I added is failing in an unexpected
> > way on at least one architecture. To recap, the idea here was that
> >
> > 1) libcpp needs new logic to be able to output correct locations for this
> > case. That will be some new code that is suitable for stage 1, not now.
> >
> > 2) In the meantime, we fixed things up enough to avoid an ICE that showed up
> > in GCC 11, and added an xfailed testcase to remind about #1.
> >
> > The problem is that, the reason that libcpp outputs the wrong locations, is
> > that it has always used a location from the old line_map instance to index
> > into the new line_map instance, and so the exact details of the wrong
> > locations it outputs depend on the state of those two line maps, which may
> > differ depending on system includes and things like that. So I was hoping to
> > make one further one-line change to libcpp, not yet to output correct
> > locations, but at least to output one which is the same always and doesn't
> > depend on random things. This would assign all restored macros to a
> > consistent location, one line following the #include that triggered the PCH
> > process. I think this probably shouldn't be backported but it would be nice
> > to get into GCC 14, while nothing critical, at least it would avoid the new
> > test failure that's being reported. But more generally, I think using a
> > location from a totally different line map is dangerous and could have worse
> > consequences that haven't been seen yet. Does it look OK please? Thanks!
>
> FWIW, I've tested this (the initial) version of this patch on
> sparc-sun-solaris2.11 (PASSes as before) and i386-pc-solaris2.11 (PASSes
> now unlike before).
>
> Thanks.
> Rainer
Thanks a lot! And sorry for that issue. I will push the updated
version of that patch shortly.
@@ -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)
new file mode 100644
@@ -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"
new file mode 100644
@@ -0,0 +1 @@
+/* This space intentionally left blank. */
new file mode 100644
@@ -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 "." } */
new file mode 100644
@@ -0,0 +1 @@
+/* This space intentionally left blank. */
new file mode 100644
@@ -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} } */
new file mode 100644
@@ -0,0 +1 @@
+/* This space intentionally left blank. */