On Mon, 2016-08-29 at 13:43 -0400, David Malcolm wrote:
> 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.
Currently the fix-it validator rejects ad-hoc locations.
Fix this by calling get_pure_location on the input locations to
add_fixit_insert/replace. Doing so requires moving get_pure_location
from gcc to libcpp.
Successfully bootstrapped®rtested on x86_64-pc-linux-gnu.
Committed to trunk as r239843.
gcc/ChangeLog:
* diagnostic-show-locus.c
(selftest::test_one_liner_fixit_validation_adhoc_locations): New
function.
(selftest::test_diagnostic_show_locus_one_liner): Call it.
* input.c (get_pure_location): Move to libcpp/line-map.c.
* input.h (get_pure_location): Convert decl to an inline function
calling implementation in libcpp.
libcpp/ChangeLog:
* include/line-map.h (get_pure_location): New decl.
* line-map.c (get_pure_location): Move here, from gcc/input.c, adding
a line_maps * param.
(rich_location::add_fixit_insert): Call get_pure_location on "where".
(rich_location::add_fixit_replace): Call get_pure_location on the
end-points.
---
gcc/diagnostic-show-locus.c | 70 +++++++++++++++++++++++++++++++++++++++++++++
gcc/input.c | 22 --------------
gcc/input.h | 6 +++-
libcpp/include/line-map.h | 6 ++++
libcpp/line-map.c | 27 +++++++++++++++++
5 files changed, 108 insertions(+), 23 deletions(-)
diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index f3f661e..ba52f24 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -1594,6 +1594,75 @@ test_one_liner_fixit_replace_equal_secondary_range ()
pp_formatted_text (dc.printer));
}
+/* Verify that we can use ad-hoc locations when adding fixits to a
+ rich_location. */
+
+static void
+test_one_liner_fixit_validation_adhoc_locations ()
+{
+ /* Generate a range that's too long to be packed, so must
+ be stored as an ad-hoc location (given the defaults
+ of 5 bits or 0 bits of packed range); 41 columns > 2**5. */
+ const location_t c7 = linemap_position_for_column (line_table, 7);
+ const location_t c47 = linemap_position_for_column (line_table, 47);
+ const location_t loc = make_location (c7, c7, c47);
+
+ if (c47 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+ return;
+
+ ASSERT_TRUE (IS_ADHOC_LOC (loc));
+
+ /* Insert. */
+ {
+ rich_location richloc (line_table, loc);
+ richloc.add_fixit_insert (loc, "test");
+ /* It should not have been discarded by the validator. */
+ ASSERT_EQ (1, richloc.get_num_fixit_hints ());
+
+ test_diagnostic_context dc;
+ diagnostic_show_locus (&dc, &richloc, DK_ERROR);
+ ASSERT_STREQ ("\n"
+ " foo = bar.field;\n"
+ " ^~~~~~~~~~ \n"
+ " test\n",
+ pp_formatted_text (dc.printer));
+ }
+
+ /* Remove. */
+ {
+ rich_location richloc (line_table, loc);
+ source_range range = source_range::from_locations (loc, c47);
+ richloc.add_fixit_remove (range);
+ /* It should not have been discarded by the validator. */
+ ASSERT_EQ (1, richloc.get_num_fixit_hints ());
+
+ test_diagnostic_context dc;
+ diagnostic_show_locus (&dc, &richloc, DK_ERROR);
+ ASSERT_STREQ ("\n"
+ " foo = bar.field;\n"
+ " ^~~~~~~~~~ \n"
+ " -----------------------------------------\n",
+ pp_formatted_text (dc.printer));
+ }
+
+ /* Replace. */
+ {
+ rich_location richloc (line_table, loc);
+ source_range range = source_range::from_locations (loc, c47);
+ richloc.add_fixit_replace (range, "test");
+ /* It should not have been discarded by the validator. */
+ ASSERT_EQ (1, richloc.get_num_fixit_hints ());
+
+ test_diagnostic_context dc;
+ diagnostic_show_locus (&dc, &richloc, DK_ERROR);
+ ASSERT_STREQ ("\n"
+ " foo = bar.field;\n"
+ " ^~~~~~~~~~ \n"
+ " test\n",
+ pp_formatted_text (dc.printer));
+ }
+}
+
/* Run the various one-liner tests. */
static void
@@ -1626,6 +1695,7 @@ test_diagnostic_show_locus_one_liner (const
line_table_case &case_)
test_one_liner_fixit_replace ();
test_one_liner_fixit_replace_non_equal_range ();
test_one_liner_fixit_replace_equal_secondary_range ();
+ test_one_liner_fixit_validation_adhoc_locations ();
}
/* Verify that fix-it hints are appropriately consolidated.
diff --git a/gcc/input.c b/gcc/input.c
index 4ec218d..a3fe542 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -838,28 +838,6 @@ expansion_point_location (source_location location)
LRK_MACRO_EXPANSION_POINT, NULL);
}
-/* Given location LOC, strip away any packed range information
- or ad-hoc information. */
-
-location_t
-get_pure_location (location_t loc)
-{
- if (IS_ADHOC_LOC (loc))
- loc
- = line_table->location_adhoc_data_map.data[loc &
MAX_SOURCE_LOCATION].locus;
-
- if (loc >= LINEMAPS_MACRO_LOWEST_LOCATION (line_table))
- return loc;
-
- if (loc < RESERVED_LOCATION_COUNT)
- return loc;
-
- const line_map *map = linemap_lookup (line_table, loc);
- const line_map_ordinary *ordmap = linemap_check_ordinary (map);
-
- return loc & ~((1 << ordmap->m_range_bits) - 1);
-}
-
/* Construct a location with caret at CARET, ranging from START to
finish e.g.
diff --git a/gcc/input.h b/gcc/input.h
index ecf8db3..fd21f34 100644
--- a/gcc/input.h
+++ b/gcc/input.h
@@ -77,7 +77,11 @@ extern location_t input_location;
#define from_macro_expansion_at(LOC) \
((linemap_location_from_macro_expansion_p (line_table, LOC)))
-extern location_t get_pure_location (location_t loc);
+static inline location_t
+get_pure_location (location_t loc)
+{
+ return get_pure_location (line_table, loc);
+}
/* Get the start of any range encoded within location LOC. */
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 0fc4848..d9c31de 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1002,6 +1002,12 @@ IS_ADHOC_LOC (source_location loc)
bool
pure_location_p (line_maps *set, source_location loc);
+/* Given location LOC within SET, strip away any packed range information
+ or ad-hoc information. */
+
+extern source_location get_pure_location (line_maps *set,
+ source_location loc);
+
/* Combine LOC and BLOCK, giving a combined adhoc location. */
inline source_location
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 8fe48bd..f5b1586 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -311,6 +311,28 @@ pure_location_p (line_maps *set, source_location loc)
return true;
}
+/* Given location LOC within SET, strip away any packed range information
+ or ad-hoc information. */
+
+source_location
+get_pure_location (line_maps *set, source_location loc)
+{
+ if (IS_ADHOC_LOC (loc))
+ loc
+ = set->location_adhoc_data_map.data[loc & MAX_SOURCE_LOCATION].locus;
+
+ if (loc >= LINEMAPS_MACRO_LOWEST_LOCATION (set))
+ return loc;
+
+ if (loc < RESERVED_LOCATION_COUNT)
+ return loc;
+
+ const line_map *map = linemap_lookup (set, loc);
+ const line_map_ordinary *ordmap = linemap_check_ordinary (map);
+
+ return loc & ~((1 << ordmap->m_range_bits) - 1);
+}
+
/* Finalize the location_adhoc_data structure. */
void
location_adhoc_data_fini (struct line_maps *set)
@@ -2077,6 +2099,8 @@ void
rich_location::add_fixit_insert (source_location where,
const char *new_content)
{
+ where = get_pure_location (m_line_table, where);
+
if (reject_impossible_fixit (where))
return;
@@ -2141,6 +2165,9 @@ rich_location::add_fixit_replace (source_range src_range,
{
linemap_assert (m_num_fixit_hints < MAX_FIXIT_HINTS);
+ src_range.m_start = get_pure_location (m_line_table, src_range.m_start);
+ src_range.m_finish = get_pure_location (m_line_table, src_range.m_finish);
+
if (reject_impossible_fixit (src_range.m_start))
return;
if (reject_impossible_fixit (src_range.m_finish))
--
1.8.5.3
