On Mon, 2016-08-29 at 08:50 -0400, David Malcolm wrote:
> On Mon, 2016-08-29 at 13:44 +0200, Marek Polacek wrote:
> > On Mon, Aug 29, 2016 at 12:35:38PM +0200, Marek Polacek wrote:
> > > On Mon, Aug 29, 2016 at 11:16:25AM +0200, Andreas Schwab wrote:
> > > > On Aug 25 2016, Marek Polacek <pola...@redhat.com> wrote:
> > > >
> > > > > * c-c++-common/Wlogical-not-parentheses-2.c: New test.
> > > >
> > > > FAIL: c-c++-common/Wlogical-not-parentheses-2.c -std=gnu++11
> > > > expected multiline pattern lines 13-17 not found: "\s*r \+=
> > > > !aaa
> > > > == bbb;.*\n \^~\n r \+= !aaa == bbb;.*\n
> > > > \^~~~\n \( \).*\n"
> > > > FAIL: c-c++-common/Wlogical-not-parentheses-2.c -std=gnu++11
> > > > (test for excess errors)
> > > > Excess errors:
> > > > r += !aaa == bbb; /* { dg-warning "logical not is only
> > > > applied" } */
> > > > ^~
> > > > r += !aaa == bbb; /* { dg-warning "logical not is only
> > > > applied" } */
> > > > ^~~~
> > >
> > > This has regressed with David's
> > >
> > > commit 367964faf71a99ebd511dffb81075b58bff345a1
> > > Author: dmalcolm <dmalcolm@138bc75d-0d04-0410-961f-82ee72b054a4>
> > > Date: Fri Aug 26 21:25:41 2016 +0000
> > >
> > > Add validation and consolidation of fix-it hints
> > >
> > > I don't know yet what exactly went wrong here.
> >
> > So we reject printing fix-it hint because in
> > reject_impossible_fixit:
> >
> > 2187 if (where <= LINE_MAP_MAX_LOCATION_WITH_COLS)
> > 2188 /* WHERE is a reasonable location for a fix-it; don't
> > reject
> > it. */
> > 2189 return false;
> >
> > (gdb) p where
> > $1 = 2147483652
> > (gdb) p LINE_MAP_MAX_LOCATION_WITH_COLS
> > $2 = 1610612736
> >
> > so we set m_seen_impossible_fixit.
> >
> > David, why is that happening?
>
> Sorry about this; I believe I did my testing of my patch against a
> tree
> that didn't yet have your change.
>
> (gdb) p /x 2147483652
> $2 = 0x80000004
>
> so "where", the insertion-point, is an ad-hoc location (describing a
> range), and it looks like I somehow forgot to deal with that case in
> the validation code.
>
> I'm working on a fix. Sorry again.
The issue affected the C++ frontend, but not the C frontend.
The root cause was that a location expressing a packed range was
passed to a make_location call as the "start" parameter.
make_location was converting the caret to a pure location (stripping
away the packed range), but wasn't doing this for the "start" parameter.
This meant that caret != start, despite them being the same
file/line/col, due to the latter containing packed range information.
This was handled by make_location by creating an ad-hoc location, and
fixit-validation doesn't yet handle ad-hoc locations for insertion
fixits, only pure locations.
The following patch fixes the issue seen in this specific case by
fixing make_location to avoid storing ranges for the endpoints of a
range, and thus avoid generating ad-hoc locations. If a location
containing a range is passed as start or finish to make_location, it
will use the start/finish of that range respectively.
I'm working on a followup fix to make the fixit-validation code
better handle any ad-hoc locations that do make it through.
Successfully bootstrapped®rtested on x86_64-pc-linux-gnu; converts
the failures in g++.sum into passes.
Committed to trunk as r239831.
gcc/ChangeLog:
* input.c (make_location): Call get_start and get_finish
on the endpoints to avoid storing packed ranges or ad-hoc
ranges in them.
(selftest::test_make_location_nonpure_range_endpoints): New function.
(selftest::input_c_tests): Call it.
* input.h (get_start): New inline function.
---
gcc/input.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
gcc/input.h | 8 ++++++++
2 files changed, 72 insertions(+), 2 deletions(-)
diff --git a/gcc/input.c b/gcc/input.c
index 2dcecfb..4ec218d 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -879,8 +879,8 @@ make_location (location_t caret, location_t start,
location_t finish)
{
location_t pure_loc = get_pure_location (caret);
source_range src_range;
- src_range.m_start = start;
- src_range.m_finish = finish;
+ src_range.m_start = get_start (start);
+ src_range.m_finish = get_finish (finish);
location_t combined_loc = COMBINE_LOCATION_DATA (line_table,
pure_loc,
src_range,
@@ -1741,6 +1741,67 @@ test_builtins ()
ASSERT_PRED1 (is_location_from_builtin_token, BUILTINS_LOCATION);
}
+/* Regression test for make_location.
+ Ensure that we use the caret locations of the start/finish, rather
+ than storing a packed or ad-hoc range as the start/finish. */
+
+static void
+test_make_location_nonpure_range_endpoints (const line_table_case &case_)
+{
+ /* Issue seen with testsuite/c-c++-common/Wlogical-not-parentheses-2.c
+ with C++ frontend.
+ ....................0000000001111111111222.
+ ....................1234567890123456789012. */
+ const char *content = " r += !aaa == bbb;\n";
+ temp_source_file tmp (SELFTEST_LOCATION, ".C", content);
+ line_table_test ltt (case_);
+ linemap_add (line_table, LC_ENTER, false, tmp.get_filename (), 1);
+
+ const location_t c11 = linemap_position_for_column (line_table, 11);
+ const location_t c12 = linemap_position_for_column (line_table, 12);
+ const location_t c13 = linemap_position_for_column (line_table, 13);
+ const location_t c14 = linemap_position_for_column (line_table, 14);
+ const location_t c21 = linemap_position_for_column (line_table, 21);
+
+ if (c21 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+ return;
+
+ /* Use column 13 for the caret location, arbitrarily, to verify that we
+ handle start != caret. */
+ const location_t aaa = make_location (c13, c12, c14);
+ ASSERT_EQ (c13, get_pure_location (aaa));
+ ASSERT_EQ (c12, get_start (aaa));
+ ASSERT_FALSE (IS_ADHOC_LOC (get_start (aaa)));
+ ASSERT_EQ (c14, get_finish (aaa));
+ ASSERT_FALSE (IS_ADHOC_LOC (get_finish (aaa)));
+
+ /* Make a location using a location with a range as the start-point. */
+ const location_t not_aaa = make_location (c11, aaa, c14);
+ ASSERT_EQ (c11, get_pure_location (not_aaa));
+ /* It should use the start location of the range, not store the range
+ itself. */
+ ASSERT_EQ (c12, get_start (not_aaa));
+ ASSERT_FALSE (IS_ADHOC_LOC (get_start (not_aaa)));
+ ASSERT_EQ (c14, get_finish (not_aaa));
+ ASSERT_FALSE (IS_ADHOC_LOC (get_finish (not_aaa)));
+
+ /* Similarly, make a location with a range as the end-point. */
+ const location_t aaa_eq_bbb = make_location (c12, c12, c21);
+ ASSERT_EQ (c12, get_pure_location (aaa_eq_bbb));
+ ASSERT_EQ (c12, get_start (aaa_eq_bbb));
+ ASSERT_FALSE (IS_ADHOC_LOC (get_start (aaa_eq_bbb)));
+ ASSERT_EQ (c21, get_finish (aaa_eq_bbb));
+ ASSERT_FALSE (IS_ADHOC_LOC (get_finish (aaa_eq_bbb)));
+ const location_t not_aaa_eq_bbb = make_location (c11, c12, aaa_eq_bbb);
+ /* It should use the finish location of the range, not store the range
+ itself. */
+ ASSERT_EQ (c11, get_pure_location (not_aaa_eq_bbb));
+ ASSERT_EQ (c12, get_start (not_aaa_eq_bbb));
+ ASSERT_FALSE (IS_ADHOC_LOC (get_start (not_aaa_eq_bbb)));
+ ASSERT_EQ (c21, get_finish (not_aaa_eq_bbb));
+ ASSERT_FALSE (IS_ADHOC_LOC (get_finish (not_aaa_eq_bbb)));
+}
+
/* Verify reading of input files (e.g. for caret-based diagnostics). */
static void
@@ -3187,6 +3248,7 @@ input_c_tests ()
test_should_have_column_data_p ();
test_unknown_location ();
test_builtins ();
+ for_each_line_table_case (test_make_location_nonpure_range_endpoints);
for_each_line_table_case (test_accessing_ordinary_linemaps);
for_each_line_table_case (test_lexer);
diff --git a/gcc/input.h b/gcc/input.h
index c99abfd..ecf8db3 100644
--- a/gcc/input.h
+++ b/gcc/input.h
@@ -79,6 +79,14 @@ extern location_t input_location;
extern location_t get_pure_location (location_t loc);
+/* Get the start of any range encoded within location LOC. */
+
+static inline location_t
+get_start (location_t loc)
+{
+ return get_range_from_loc (line_table, loc).m_start;
+}
+
/* Get the endpoint of any range encoded within location LOC. */
static inline location_t
--
1.8.5.3
