From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x82f.google.com (mail-qt1-x82f.google.com [IPv6:2607:f8b0:4864:20::82f]) by sourceware.org (Postfix) with ESMTPS id 665693858D37 for ; Thu, 1 Feb 2024 02:09:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 665693858D37 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 665693858D37 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::82f ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706753387; cv=none; b=AwoNNYREa/9C5YCkdS6MaLtW4LxnJS+cHu16mmGzrrGPQO9uwOOsz/1wwqvMbLwZdFRQ1bcDXeamBS7GaBJsfUS2Yykox0sIf0dW7oBaOcGJimmM7AJvPq5jvon0N2jk/0VkgHOBoKlDXD/cTbS3gce1qo4dcajwjm7z+NVCIHY= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706753387; c=relaxed/simple; bh=DaLQL33bZo8BxiKgzY0Y/aGVLvGOPzL5C2g9jxoJz00=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=ZvFt5FkQ6f4H/l86Fi8n4zLxwmymALoMRjLaH2zadHUzHYbnO9GSZqPH8a9hpc4GOkr1Ls93ZIlJrzdiffKaNCgG/3DG3sx1CqCNZoVrLk3nAM5XvudIK7F4gKu4QsVXhBIOnW8cuLeYpFztOrzTXaOEjJHgnYSGTvCAU/H3uTw= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-qt1-x82f.google.com with SMTP id d75a77b69052e-429f53f0b0bso2383271cf.2 for ; Wed, 31 Jan 2024 18:09:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1706753372; x=1707358172; darn=gcc.gnu.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=VxpY36p0IMUOVJF0uL4BSdmW3gdCT41c12wUpDtnhTo=; b=lrKP171m9fcqzhwzEdbXY5ZOTASMz9LEbY3KLJn9jHX1u3vhLA6OmIJYPK29wKVCyY fIX8eVJXEGF16qV2Qq+KLe9BDTl82nV2DNudSUzpfnkoIy5f4w3zqgThVPQJPuAxplna Q1ceBmcli8HsUQp2aQzeZHbFmmbqkJyn2VJLj4aAWATDjLyNa66LN//uTLNBFprMqekU 7m0o7ehT4Ub/vd2C+k7PKzOVUvifiItDAR1HjMgm7xtRXuEJsmyrfaTAM8/R577fGqah uETjnTvm86imh1qUpFEaMgZ+yaM00YXYVWAk/bCfJi3LMnepMHk+Xn1PQGrtlFg0ZnB9 b62A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706753373; x=1707358173; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=VxpY36p0IMUOVJF0uL4BSdmW3gdCT41c12wUpDtnhTo=; b=MCXzVVhGHTc/+YD5Rk7B/33eAW8TVH6mXMbO6stFseSc74jSYc34QL0KG6oKJ/fiVR 7rpapmh53TiUj/yHGOAVZtDAtyZrZSy2BEAIzi5cWtSTkqbsTno9009eOnrfQ4b+DOsc EFjivxV5YfWsdxUBydCgt2x0fiEkkLJJ6cbGXEsHzWYB+zpasJBwlxpNjwZMWdI+JpSb /2WBxbf8wj9En6Vb8M4nrTTq/GTlCXTOCekS8YkPvz5hfLb6bV5aIg2DnuJcinS6NrG0 IpxGV/bPSjQnPjpyO8sND8Z9hW21lNeDaDis/bhG73gwwGK8F19XV6BGGP1p55RCEZ59 zbxw== X-Gm-Message-State: AOJu0Yx9+WqXy7UViljADEwZqTdQQfNDEmEDU0MRPmCrL3pXUZKckojk Qo5YV5pj6RRet0IPiWPTkQEYdcBWl48nC7Dk68Mp713PvNQPjP/X X-Google-Smtp-Source: AGHT+IHeW5d8J2p7taHVwY3JLKvq5vLPXdearxLYAmbxDsG9x/EWva4VYmEto2FrTDWMRFbhms6SIg== X-Received: by 2002:ad4:5945:0:b0:68c:75e8:7d7d with SMTP id eo5-20020ad45945000000b0068c75e87d7dmr1445674qvb.60.1706753372622; Wed, 31 Jan 2024 18:09:32 -0800 (PST) Received: from blueshift.am ([96.67.140.173]) by smtp.gmail.com with ESMTPSA id mw2-20020a05621433c200b006891e1f098csm6105915qvb.78.2024.01.31.18.09.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 31 Jan 2024 18:09:31 -0800 (PST) Date: Wed, 31 Jan 2024 21:09:30 -0500 From: Lewis Hyatt To: Jason Merrill Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] c-family: Fix ICE with large column number after restoring a PCH [PR105608] Message-ID: References: <20231206015211.682650-1-lhyatt@gmail.com> <17e2d4b7-1dfe-4e0c-9fea-c5c115ffc66c@redhat.com> <1020fac4-663e-49bb-b4e3-0c6b7a7c0f59@redhat.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="u5CHFxKYSZnr/ZDT" Content-Disposition: inline In-Reply-To: <1020fac4-663e-49bb-b4e3-0c6b7a7c0f59@redhat.com> X-Spam-Status: No, score=-3038.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,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: --u5CHFxKYSZnr/ZDT Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 --u5CHFxKYSZnr/ZDT Content-Type: text/plain; charset=us-ascii Content-Disposition: inline; filename="PR105608-part2-v2.txt" [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); } --u5CHFxKYSZnr/ZDT--