From: Jason Merrill <jason@redhat.com>
To: Lewis Hyatt <lhyatt@gmail.com>, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] c-family: Fix ICE with large column number after restoring a PCH [PR105608]
Date: Fri, 26 Jan 2024 16:16:54 -0500 [thread overview]
Message-ID: <17e2d4b7-1dfe-4e0c-9fea-c5c115ffc66c@redhat.com> (raw)
In-Reply-To: <20231206015211.682650-1-lhyatt@gmail.com>
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. */
>
next prev parent reply other threads:[~2024-01-26 21:16 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-06 1:52 Lewis Hyatt
2023-12-21 1:02 ` Lewis Hyatt
2024-01-25 22:38 ` ping: " Lewis Hyatt
2024-01-26 21:16 ` Jason Merrill [this message]
2024-01-31 2:49 ` Lewis Hyatt
2024-01-31 20:18 ` Jason Merrill
2024-02-01 2:09 ` Lewis Hyatt
2024-02-01 2:45 ` Jason Merrill
2024-02-01 12:24 ` Rainer Orth
2024-02-01 13:23 ` Lewis Hyatt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=17e2d4b7-1dfe-4e0c-9fea-c5c115ffc66c@redhat.com \
--to=jason@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=lhyatt@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).