From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 92491 invoked by alias); 12 Jan 2018 23:16:22 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 92456 invoked by uid 89); 12 Jan 2018 23:16:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 12 Jan 2018 23:16:20 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1F3D27485B; Fri, 12 Jan 2018 23:16:19 +0000 (UTC) Received: from ovpn-116-33.phx2.redhat.com (ovpn-116-33.phx2.redhat.com [10.3.116.33]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9FA3F6A838; Fri, 12 Jan 2018 23:16:18 +0000 (UTC) Message-ID: <1515798977.24844.99.camel@redhat.com> Subject: Re: [RFC][PATCH] PR preprocessor/83173: Additional check before decrementing highest_location From: David Malcolm To: Mike Gulick , gcc-patches@gcc.gnu.org Date: Fri, 12 Jan 2018 23:33:00 -0000 In-Reply-To: <7d281f10-a4e0-d81e-c405-d77ceda86f5b@mathworks.com> References: <7d281f10-a4e0-d81e-c405-d77ceda86f5b@mathworks.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-01/txt/msg01143.txt.bz2 On Fri, 2017-12-01 at 17:57 -0500, Mike Gulick wrote: > I've come up with some patches that fix PR preprocessor/83173, which > I reported > a couple of weeks ago. > > The first patch is a test case. The second and third patches are two > versions > of the fix. The first version is simpler, but it may still leave in > place some > subtle incorrect behavior that happens when the current source > location is less > than LINE_MAP_MAX_COLUMN_NUMBER. The second version tries to handle > that case > as well, however I'm less comfortable with it as I don't know whether > I'm > computing the source_location of the *end* of the current line > correctly in all > cases. Both of these pass the gcc/g++ test suites with no > regressions. > > Thanks in advance for the review/feedback! > > -Mike Hi Mike; sorry about the delay in reviewing this. Do you have the gcc contributor paperwork in place? > From 6ff0068284c346c8db08c4b6b4d9a66d8464aeac Mon Sep 17 00:00:00 2001 > From: Mike Gulick > Date: Thu, 30 Nov 2017 18:35:48 -0500 > Subject: [PATCH 1/2] PR preprocessor/83173: New test > > 2017-12-01 Mike Gulick > > PR preprocessor/83173 > * gcc.dg/plugin/pr83173.c: New test. > * gcc.dg/plugin/pr83173.h: Header for pr83173.c > * gcc.dg/plugin/pr83173-1.h: Header for pr83173.c > * gcc.dg/plugin/pr83173-2.h: Header for pr83173.c > * gcc.dg/plugin/location_overflow_pp_plugin.c: New plugin to > override line_table->highest_location for preprocessor. > --- > .../gcc.dg/plugin/location_overflow_pp_plugin.c | 44 ++++++++++++++++++++++ > gcc/testsuite/gcc.dg/plugin/plugin.exp | 1 + > gcc/testsuite/gcc.dg/plugin/pr83173-1.h | 2 + > gcc/testsuite/gcc.dg/plugin/pr83173-2.h | 2 + > gcc/testsuite/gcc.dg/plugin/pr83173.c | 21 +++++++++++ > gcc/testsuite/gcc.dg/plugin/pr83173.h | 2 + > 6 files changed, 72 insertions(+) > create mode 100644 gcc/testsuite/gcc.dg/plugin/location_overflow_pp_plugin.c > create mode 100644 gcc/testsuite/gcc.dg/plugin/pr83173-1.h > create mode 100644 gcc/testsuite/gcc.dg/plugin/pr83173-2.h > create mode 100644 gcc/testsuite/gcc.dg/plugin/pr83173.c > create mode 100644 gcc/testsuite/gcc.dg/plugin/pr83173.h > > diff --git a/gcc/testsuite/gcc.dg/plugin/location_overflow_pp_plugin.c b/gcc/testsuite/gcc.dg/plugin/location_overflow_pp_plugin.c > new file mode 100644 > index 00000000000..ba5a795b937 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/plugin/location_overflow_pp_plugin.c > @@ -0,0 +1,44 @@ > +/* Plugin for testing how gracefully we degrade in the face of very > + large source files. */ > + > +#include "config.h" > +#include "gcc-plugin.h" > +#include "system.h" > +#include "coretypes.h" > +#include "diagnostic.h" > + > +int plugin_is_GPL_compatible; > + > +static location_t base_location; > + > +/* Callback handler for the PLUGIN_PRAGMAS event. This is used to set the > + initial line table offset for the preprocessor, to make it appear as if we > + had parsed a very large file. PRAGMA_START_UNIT is not suitable here as is PLUGIN_START_UNIT, presumably? > + not invoked during the preprocessor stage. */ This new test plugin seems almost identical to the existing location_overflow_plugin.c. I tested changing the existing plugin to use PLUGIN_PRAGMAS rather than PLUGIN_START_UNIT, and it works fine for that event, so if that's the only difference, then maybe we don't need this new plugin? [...snip...] > diff --git a/gcc/testsuite/gcc.dg/plugin/pr83173.c b/gcc/testsuite/gcc.dg/plugin/pr83173.c > new file mode 100644 > index 00000000000..ff1858a2b33 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/plugin/pr83173.c > @@ -0,0 +1,21 @@ > +/* > + { dg-options "-fplugin-arg-location_overflow_pp_plugin-value=0x60000001" } This hardcodes a location_t overflow value. There are a pair of comments in libcpp/include/line-map.h that read: /* Do not pack ranges if locations get higher than this. If you change this, update: gcc.dg/plugin/location-overflow-test-*.c. */ I was going to suggest renaming your new test to e.g. location-overflow-test-pr83173.c so that it matches the glob in those comments, but given that you refer to the testname in dg-final directives, please update the line-map.h comments to refer to e.g. "all of testcases in gcc.dg/plugin that use location_overflow_plugin.c.", or somesuch wording. > +/* > + The preprocessor output should contain '# 1 "path/to/pr83173-1.h" 1', but > + should not contain any other references to pr83183-1.h. > + > + { dg-final { scan-file pr83173.i "# 1 \[^\r\n\]+pr83173-1\.h\" 1" } } > + { dg-final { scan-file-not pr83173.i "# (?!1 \[^\r\n\]+pr83173-1\.h\" 1)\[0-9\]+ \[^\r\n\]+pr83173-1\.h\"" } } [...snip...] If I'm reading your description in the PR right, this test case covers the loc > LINE_MAP_MAX_LOCATION_WITH_COLS case, but not the: loc <= LINE_MAP_MAX_LOCATION_WITH_COLS case. Can the latter be done by re-using the testcase, but with a different (or no) plugin argument? I'm still mulling over the two proposed fixes (it's been a while since I poked at the innards of the linemap impl); sorry. Dave