From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 9FFD23858C53 for ; Fri, 26 Jan 2024 21:16:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9FFD23858C53 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 9FFD23858C53 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706303829; cv=none; b=xjQpLZ3C06udj1O30KffLLzzkdUumxQ5lr5CFAwTiRqV92uDrU2fAktivDr0pmmMx0oa8D4lMooFOf7QDjgfwx5Obpj5RbAT6g2TADQJyz5bvJvYcsdsle+q6JSsrqqGpDsWOULP/IlSgcC+MJN8r2w8d0qKCYiYrFpqBBsnrPU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706303829; c=relaxed/simple; bh=o7Nwzgk/0hAlenmQIg2EyY6XeVTGqaOCGdJYdEJB5Mk=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=pUhNiljLwe48jXX2Td62Gew5iA5icA+3A7h1yHvfI4kLbOH/CoI04jVkZwokcoFOjl3e2UrfA9FI6e7osk8A5yHqyBSerukWI63CRZnGiKRzgeeZFqEzt/o0J0+4PU4TSApUmxgHnUnHFO4RrRFRgfp1Jb9jXFgWiIrCYflbFWk= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1706303818; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=gDIj/wTAcqLxtx8N4mx5ARK8m0opF58CUdJu6BHnt20=; b=d0KYe+iIXOD/6sm4J027gURnt8NT/BS61JwPYhh6JbeyZmlqKf+WpiNjpNavgEC/0jjon0 NviW3GielSUYv8JD3XPffETbdmnBCNVrxDOgEdwDrxFFQQo2rUEvtJGKbqzhwHa5J2BjOe CXb2zAqJy6uwdmwbL5Yuiap/5xcJZ4Y= Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-352-Tz_uTkboNwW5YFH8Li3fYA-1; Fri, 26 Jan 2024 16:16:57 -0500 X-MC-Unique: Tz_uTkboNwW5YFH8Li3fYA-1 Received: by mail-qv1-f69.google.com with SMTP id 6a1803df08f44-6887759ff05so14595146d6.1 for ; Fri, 26 Jan 2024 13:16:57 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706303816; x=1706908616; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=gDIj/wTAcqLxtx8N4mx5ARK8m0opF58CUdJu6BHnt20=; b=mFYp+6gsh8xpDCWjSGc+JyuEaF1qwcKDiiGRyPxse7dgM8vR7PAHPLEBp6UF7BstH9 cACc9A3AUxWUgtfqxeGlXFnYXAPMKnU6zRBxwSV+OtMr0LTLLCiEfhSueLjhEvpvSG9D eG0GKVt6FS0xd3b18OzEHaayrtYsgOPI0apEPfGtg+TAEOEPWi3BKc9izbvd3KJnhT4B GR0qu3xxjw2TC6NMTOaISiTDdy5GVaRwZWFzPzNYI3J0/9dsTey0xi4Xa8fsdNzdRGKq eNpvK3JHSBtrTS5tLDao5nCwkjdm56GLvLUKAVFw0z2y24qQamKU+qRZGNP+dpTgmXPZ mwmQ== X-Gm-Message-State: AOJu0YxxyMrJJguWWWz9MQTgr7rnA9iJFQdg+1mgz9i5M1OZHlii1fHY mcWQPi/61Gut5U1Nl3l5vkWVp0+NZ8qwXcbTAhe3quQftDtcxtvcxExaR/EDRyZIL/mSnGPlgqi EXsg3j2FcnFxfYJornQb4xOvKbY/Hslw7WDgoo2ntkXzF8LavxmLPQRn0Ch/zVHw= X-Received: by 2002:ad4:5c6c:0:b0:68c:35ea:e137 with SMTP id i12-20020ad45c6c000000b0068c35eae137mr774943qvh.41.1706303816245; Fri, 26 Jan 2024 13:16:56 -0800 (PST) X-Google-Smtp-Source: AGHT+IH8tejDYhYf6dwS7g/bHS8sjR77l8dWsCE5Zs7X8kViJGojZR5BHLJehHLtWCUdEFSX2115Tg== X-Received: by 2002:ad4:5c6c:0:b0:68c:35ea:e137 with SMTP id i12-20020ad45c6c000000b0068c35eae137mr774932qvh.41.1706303815922; Fri, 26 Jan 2024 13:16:55 -0800 (PST) Received: from [192.168.1.108] (130-44-146-16.s12558.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.146.16]) by smtp.gmail.com with ESMTPSA id kd8-20020a056214400800b00686ac3c9db4sm827054qvb.98.2024.01.26.13.16.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 26 Jan 2024 13:16:55 -0800 (PST) Message-ID: <17e2d4b7-1dfe-4e0c-9fea-c5c115ffc66c@redhat.com> Date: Fri, 26 Jan 2024 16:16:54 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] c-family: Fix ICE with large column number after restoring a PCH [PR105608] To: Lewis Hyatt , gcc-patches@gcc.gnu.org References: <20231206015211.682650-1-lhyatt@gmail.com> From: Jason Merrill In-Reply-To: <20231206015211.682650-1-lhyatt@gmail.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,RCVD_IN_SORBS_WEB,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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. */ >