On 01/12/2018 06:16 PM, David Malcolm wrote:
> 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?
Hi Dave. I don't have any gcc contributor paperwork in place, however I just
finished that process for a gdb patch so I think it could be done pretty
quickly.
>> 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?
>
Sorry, yes that is right.
>> + not invoked during the preprocessor stage. */
>
> This new test plugin seems almost identical to the existing
> location_overflow_plugin.c.
Yes, I was originally hoping to use location_overflow_plugin.c until I realized
that PLUGIN_START_UNIT event was not triggered during the preprocessor.
Additionally, location_overflow_plugin.c has some restrictions that allow only a
couple of specific initial offsets to be accepted, although I don't see any
reason why that couldn't be changed.
>
> 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?
>
I imagine it would work, although it does seem like PLUGIN_PRAGMAS is being used
for something other than its intended purpose here, which I was trying to keep
to a minimum. Its been a while since I wrote this patch, and I can't recall
whether I noticed any other side-effects of using PLUGIN_PRAGMAS that could
impact the existing location overflow tests. I'll take a look at it again and
update the patch or report back any concerns.
> [...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.
>
If I'm going to modify location_overflow_plugin.c and reuse it for this PR, then
it would make sense to rename the test and its accompanying helper files to your
suggested naming as well. The dg-final regexes will likely continue to work.
>> +/*
>> + 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 haven't really poked too hard to try to find if there is any corruption of the
line map occurring when loc <= LINE_MAP_MAX_LOCATION_WITH_COLS. It is just a
suspicion given the fact that this code is still decrementing
line_table->highest_location in that case. I would imagine this may result in
corruption of the column number or range rather than the file name and line
number.
> 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
>
Thanks. A number of developers at my company have been using gcc 6.3 internally
with the first proposed patch backported to it, and haven't reported any issues.
When I was testing the second version of the patch, I had also suspected some of
the bounds in the assertions in linemap_add could be tightened, but I did see
some assertion failures after doing that when running the test suite. This
simply convinced me that I didn't really understand the nuances of the line map,
so I really appreciate feedback from someone who is able to grok that code.
Thanks,
Mike
