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 <mgul...@mathworks.com>
> Date: Thu, 30 Nov 2017 18:35:48 -0500
> Subject: [PATCH 1/2] PR preprocessor/83173: New test
>
> 2017-12-01 Mike Gulick <mgul...@mathworks.com>
>
> 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
