* [PATCH] Fix missing range information for "%q+D" format code
@ 2015-12-03 20:13 David Malcolm
2015-12-04 11:09 ` Bernd Schmidt
0 siblings, 1 reply; 5+ messages in thread
From: David Malcolm @ 2015-12-03 20:13 UTC (permalink / raw)
To: gcc-patches; +Cc: David Malcolm
There are about 220 or so diagnostics in trunk that use "%q+D" in
their format string, which, as well as printing a quoted decl,
overwrites any location_t supplied to the diagnostic, instead using
the location of the associated decl.
During development of the location range patch kit I adjusted
things to use both location&range of the decl for this case, but it
looks I broke it at some point; in the version in trunk the code is
currently discarding range information, so that just the caret is
printed.
For example:
diagnostic-ranges-1.c:6:7: warning: unused variable 'redundant' [-Wunused-variable]
int redundant;
^
The attached patch updates the handling of %q+D, simplifying
the implementation, and ensuring that it retains the range
information of the decl, giving:
diagnostic-ranges-1.c:6:7: warning: unused variable âredundantâ [-Wunused-variable]
int redundant;
^~~~~~~~~
As well as the above fix, the patch adds test coverage, both
- for the specific case above, and
- as a unit test for %q+D via one of the existing test plugins
Successfully bootstrapped®rtested on x86_64-pc-linux-gnu; adds
5 PASS results to gcc.sum.
OK for trunk?
gcc/c-family/ChangeLog:
* c-common.c (c_cpp_error): Update for change to
rich_location::set_range.
gcc/fortran/ChangeLog:
* error.c (gfc_format_decoder): Update for change of
text_info::set_range to text_info::set_location.
gcc/ChangeLog:
* pretty-print.c (text_info::set_range): Rename to...
(text_info::set_location): ...this, converting 2nd param
from source_range to a location_t.
* pretty-print.h (text_info::set_location): Convert
from inline function to external definition.
(text_info::set_range): Delete.
gcc/testsuite/ChangeLog:
* gcc.dg/diagnostic-ranges-1.c: New test file.
* gcc.dg/plugin/diagnostic-test-show-locus-bw.c
(test_percent_q_plus_d): New test function.
* gcc.dg/plugin/diagnostic_plugin_test_show_locus.c
(test_show_locus): Rewrite test code using
rich_location::set_range. Add code to unit-test the "%q+D"
format code.
libcpp/ChangeLog:
* include/line-map.h (rich_location::set_range): Add line_maps *
param; convert param from source_range to source_location. Drop
"overwrite_loc_p" param.
* line-map.c (rich_location::set_range): Likewise, acting as if
"overwrite_loc_p" were true, and getting range from the location.
---
gcc/c-family/c-common.c | 4 +---
gcc/fortran/error.c | 11 ++++-----
gcc/pretty-print.c | 6 ++---
gcc/pretty-print.h | 9 +-------
gcc/testsuite/gcc.dg/diagnostic-ranges-1.c | 11 +++++++++
.../gcc.dg/plugin/diagnostic-test-show-locus-bw.c | 12 ++++++++++
.../plugin/diagnostic_plugin_test_show_locus.c | 27 +++++++++++++++++-----
libcpp/include/line-map.h | 4 ++--
libcpp/line-map.c | 14 ++++++-----
9 files changed, 64 insertions(+), 34 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/diagnostic-ranges-1.c
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index a8122b3..59cfc19 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -10129,9 +10129,7 @@ c_cpp_error (cpp_reader *pfile ATTRIBUTE_UNUSED, int level, int reason,
gcc_unreachable ();
}
if (done_lexing)
- richloc->set_range (0,
- source_range::from_location (input_location),
- true, true);
+ richloc->set_range (line_table, 0, input_location, true);
diagnostic_set_info_translated (&diagnostic, msg, ap,
richloc, dlevel);
diagnostic_override_option_index (&diagnostic,
diff --git a/gcc/fortran/error.c b/gcc/fortran/error.c
index b4f7020..8f57aff 100644
--- a/gcc/fortran/error.c
+++ b/gcc/fortran/error.c
@@ -939,12 +939,11 @@ gfc_format_decoder (pretty_printer *pp,
/* If location[0] != UNKNOWN_LOCATION means that we already
processed one of %C/%L. */
int loc_num = text->get_location (0) == UNKNOWN_LOCATION ? 0 : 1;
- source_range range
- = source_range::from_location (
- linemap_position_for_loc_and_offset (line_table,
- loc->lb->location,
- offset));
- text->set_range (loc_num, range, true);
+ location_t src_loc
+ = linemap_position_for_loc_and_offset (line_table,
+ loc->lb->location,
+ offset);
+ text->set_location (loc_num, src_loc, true);
pp_string (pp, result[loc_num]);
return true;
}
diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c
index 4a28d3c..3365074 100644
--- a/gcc/pretty-print.c
+++ b/gcc/pretty-print.c
@@ -31,14 +31,14 @@ along with GCC; see the file COPYING3. If not see
#include <iconv.h>
#endif
-/* Overwrite the range within this text_info's rich_location.
+/* Overwrite the given location/range within this text_info's rich_location.
For use e.g. when implementing "+" in client format decoders. */
void
-text_info::set_range (unsigned int idx, source_range range, bool caret_p)
+text_info::set_location (unsigned int idx, location_t loc, bool show_caret_p)
{
gcc_checking_assert (m_richloc);
- m_richloc->set_range (idx, range, caret_p, true);
+ m_richloc->set_range (line_table, idx, loc, show_caret_p);
}
location_t
diff --git a/gcc/pretty-print.h b/gcc/pretty-print.h
index cdee253..3dc1f6b 100644
--- a/gcc/pretty-print.h
+++ b/gcc/pretty-print.h
@@ -37,14 +37,7 @@ struct text_info
void **x_data;
rich_location *m_richloc;
- inline void set_location (unsigned int idx, location_t loc, bool caret_p)
- {
- source_range src_range;
- src_range.m_start = loc;
- src_range.m_finish = loc;
- set_range (idx, src_range, caret_p);
- }
- void set_range (unsigned int idx, source_range range, bool caret_p);
+ void set_location (unsigned int idx, location_t loc, bool caret_p);
location_t get_location (unsigned int index_of_location) const;
};
diff --git a/gcc/testsuite/gcc.dg/diagnostic-ranges-1.c b/gcc/testsuite/gcc.dg/diagnostic-ranges-1.c
new file mode 100644
index 0000000..369c0b3
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/diagnostic-ranges-1.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-fdiagnostics-show-caret -Wall" } */
+
+void test_range_of_unused_variable (void)
+{
+ int redundant; /* { dg-warning "unused variable" } */
+/* { dg-begin-multiline-output "" }
+ int redundant;
+ ^~~~~~~~~
+ { dg-end-multiline-output "" } */
+}
diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw.c b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw.c
index 44b47e0..8d44078 100644
--- a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw.c
+++ b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw.c
@@ -190,3 +190,15 @@ void test_fixit_replace (void)
{ dg-end-multiline-output "" } */
#endif
}
+
+/* Test of "%q+D" format code. */
+
+int test_percent_q_plus_d (void)
+{
+ int local = 0; /* { dg-warning "example of plus in format code" } */
+/* { dg-begin-multiline-output "" }
+ int local = 0;
+ ^~~~~
+ { dg-end-multiline-output "" } */
+ return local;
+}
diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_test_show_locus.c b/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_test_show_locus.c
index 7ff2cff..02a2aef 100644
--- a/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_test_show_locus.c
+++ b/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_test_show_locus.c
@@ -224,9 +224,11 @@ test_show_locus (function *fun)
source_range src_range;
src_range.m_start = get_loc (line, 12);
src_range.m_finish = get_loc (line, 20);
- rich_location richloc (line_table, caret);
- richloc.set_range (0, src_range, true, false);
- warning_at_rich_loc (&richloc, 0, "test");
+ location_t combined_loc = COMBINE_LOCATION_DATA (line_table,
+ caret,
+ src_range,
+ NULL);
+ warning_at (combined_loc, 0, "test");
}
/* Example of a very wide line, where the information of interest
@@ -238,9 +240,11 @@ test_show_locus (function *fun)
source_range src_range;
src_range.m_start = get_loc (line, 90);
src_range.m_finish = get_loc (line, 98);
- rich_location richloc (line_table, caret);
- richloc.set_range (0, src_range, true, false);
- warning_at_rich_loc (&richloc, 0, "test");
+ location_t combined_loc = COMBINE_LOCATION_DATA (line_table,
+ caret,
+ src_range,
+ NULL);
+ warning_at (combined_loc, 0, "test");
}
/* Example of multiple carets. */
@@ -313,6 +317,17 @@ test_show_locus (function *fun)
global_dc->caret_chars[0] = '^';
global_dc->caret_chars[1] = '^';
}
+
+ /* Example of using the "%q+D" format code, which as well as printing
+ a quoted decl, overrides the given location to use the location of
+ the decl. */
+ if (0 == strcmp (fnname, "test_percent_q_plus_d"))
+ {
+ const int line = fnstart_line + 3;
+ tree local = (*fun->local_decls)[0];
+ warning_at (input_location, 0,
+ "example of plus in format code for %q+D", local);
+ }
}
unsigned int
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 4f440fa..73c583e 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1376,8 +1376,8 @@ class rich_location
add_range (location_range *src_range);
void
- set_range (unsigned int idx, source_range src_range,
- bool show_caret_p, bool overwrite_loc_p);
+ set_range (line_maps *set, unsigned int idx, source_location loc,
+ bool show_caret_p);
unsigned int get_num_locations () const { return m_num_ranges; }
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 4284303..333e835 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -2079,8 +2079,8 @@ rich_location::add_range (location_range *src_range)
"%C" and "%L" format codes. */
void
-rich_location::set_range (unsigned int idx, source_range src_range,
- bool show_caret_p, bool overwrite_loc_p)
+rich_location::set_range (line_maps *set, unsigned int idx,
+ source_location loc, bool show_caret_p)
{
linemap_assert (idx < MAX_RANGES);
@@ -2088,6 +2088,8 @@ rich_location::set_range (unsigned int idx, source_range src_range,
on the end of the array. */
linemap_assert (idx <= m_num_ranges);
+ source_range src_range = get_range_from_loc (set, loc);
+
location_range *locrange = &m_ranges[idx];
locrange->m_start
= linemap_client_expand_location_to_spelling_point (src_range.m_start);
@@ -2095,16 +2097,16 @@ rich_location::set_range (unsigned int idx, source_range src_range,
= linemap_client_expand_location_to_spelling_point (src_range.m_finish);
locrange->m_show_caret_p = show_caret_p;
- if (overwrite_loc_p)
- locrange->m_caret = locrange->m_start;
+ locrange->m_caret
+ = linemap_client_expand_location_to_spelling_point (loc);
/* Are we adding a range onto the end? */
if (idx == m_num_ranges)
m_num_ranges = idx + 1;
- if (idx == 0 && overwrite_loc_p)
+ if (idx == 0)
{
- m_loc = src_range.m_start;
+ m_loc = loc;
/* Mark any cached value here as dirty. */
m_have_expanded_location = false;
}
--
1.8.5.3
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix missing range information for "%q+D" format code
2015-12-03 20:13 [PATCH] Fix missing range information for "%q+D" format code David Malcolm
@ 2015-12-04 11:09 ` Bernd Schmidt
2015-12-04 20:45 ` David Malcolm
0 siblings, 1 reply; 5+ messages in thread
From: Bernd Schmidt @ 2015-12-04 11:09 UTC (permalink / raw)
To: David Malcolm, gcc-patches
On 12/03/2015 09:33 PM, David Malcolm wrote:
> The attached patch updates the handling of %q+D, simplifying
> the implementation, and ensuring that it retains the range
> information of the decl, giving:
>
> diagnostic-ranges-1.c:6:7: warning: unused variable âredundantâ [-Wunused-variable]
> int redundant;
> ^~~~~~~~~
For most of it I've convinced myself that it looks OK.
>
> void
> -rich_location::set_range (unsigned int idx, source_range src_range,
> - bool show_caret_p, bool overwrite_loc_p)
> +rich_location::set_range (line_maps *set, unsigned int idx,
> + source_location loc, bool show_caret_p)
> {
Here you need to update the function comment.
This is a bit of a strange function. As far as I can tell, it's called
from only two places, one in c-common.c (which overwrites idx 0, i.e.
the entire range, and therefore is uninteresting), and one in
text_info::set_location. I wonder about the use of "idx" in the latter.
As far as I can tell, that's used by the Fortran frontend to keep track
of two separate ranges for their diagnostics - correct? Is that really
related to the normal function of rich_location and how it keeps track
of multiple ranges, or would that be better expressed by keeping two
rich_locations in text_info?
Bernd
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix missing range information for "%q+D" format code
2015-12-04 11:09 ` Bernd Schmidt
@ 2015-12-04 20:45 ` David Malcolm
2015-12-04 21:09 ` David Malcolm
0 siblings, 1 reply; 5+ messages in thread
From: David Malcolm @ 2015-12-04 20:45 UTC (permalink / raw)
To: Bernd Schmidt; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2645 bytes --]
On Fri, 2015-12-04 at 12:09 +0100, Bernd Schmidt wrote:
> On 12/03/2015 09:33 PM, David Malcolm wrote:
> > The attached patch updates the handling of %q+D, simplifying
> > the implementation, and ensuring that it retains the range
> > information of the decl, giving:
> >
> > diagnostic-ranges-1.c:6:7: warning: unused variable âredundantâ [-Wunused-variable]
> > int redundant;
> > ^~~~~~~~~
>
> For most of it I've convinced myself that it looks OK.
Thanks.
> > void
> > -rich_location::set_range (unsigned int idx, source_range src_range,
> > - bool show_caret_p, bool overwrite_loc_p)
> > +rich_location::set_range (line_maps *set, unsigned int idx,
> > + source_location loc, bool show_caret_p)
> > {
>
> Here you need to update the function comment.
Is the attached suitable?
> This is a bit of a strange function. As far as I can tell, it's called
> from only two places, one in c-common.c (which overwrites idx 0, i.e.
> the entire range, and therefore is uninteresting),
idx 0 is the primary location associated with the rich_location; but I'm
not sure it's strictly correct to say "the entire range". In theory
there could be other locations/ranges within the rich_location e.g. if
someone did something like:
rich_location richloc (loc_A); /* primary loc is "loc_A" */
/* ...then add a couple of other ranges of interest
e.g. to print a bogus infix binary operator,
or somesuch. */
richloc.add_range (range_B);
richloc.add_range (range_C);
/* richloc now has:
idx == 0 (primary) at loc_A
idx == 1 at range_B
idx == 2 at range_C. */
/* emit it... */
error_at_richloc (&richloc,
"something bad involving %q+D", decl);
where the %q+D leads to the idx == 0 location being overwritten by
DECL_SOURCE_LOCATION (decl) instead of loc_A.
> and one in
> text_info::set_location. I wonder about the use of "idx" in the latter.
> As far as I can tell, that's used by the Fortran frontend to keep track
> of two separate ranges for their diagnostics - correct?
Yes. These format codes that manipulate the location of the
diagnostic were one of the big surprises to me when I was writing this.
This function exists to support them.
> Is that really
> related to the normal function of rich_location and how it keeps track
> of multiple ranges,
Yes.
> or would that be better expressed by keeping two
> rich_locations in text_info?
No; keeping track of multiple locations/ranges, potentially with
multiple carets is rich_location's job.
Hope this makes sense; does it look OK with the proposed comment fix?
Dave
[-- Attachment #2: comment.patch --]
[-- Type: text/x-patch, Size: 955 bytes --]
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 333e835..0c8f328 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -2064,14 +2064,14 @@ rich_location::add_range (location_range *src_range)
m_ranges[m_num_ranges++] = *src_range;
}
-/* Add or overwrite the range given by IDX. It must either
- overwrite an existing range, or add one *exactly* on the end of
+/* Add or overwrite the location given by IDX. It must either
+ overwrite an existing location, or add one *exactly* on the end of
the array.
This is primarily for use by gcc when implementing diagnostic
format decoders e.g. the "+" in the C/C++ frontends, for handling
format codes like "%q+D" (which writes the source location of a
- tree back into range 0 of the rich_location).
+ tree back into location 0 of the rich_location).
If SHOW_CARET_P is true, then the range should be rendered with
a caret at its starting location. This
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix missing range information for "%q+D" format code
2015-12-04 20:45 ` David Malcolm
@ 2015-12-04 21:09 ` David Malcolm
2015-12-07 10:31 ` Bernd Schmidt
0 siblings, 1 reply; 5+ messages in thread
From: David Malcolm @ 2015-12-04 21:09 UTC (permalink / raw)
To: Bernd Schmidt; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2251 bytes --]
On Fri, 2015-12-04 at 15:45 -0500, David Malcolm wrote:
> On Fri, 2015-12-04 at 12:09 +0100, Bernd Schmidt wrote:
> > On 12/03/2015 09:33 PM, David Malcolm wrote:
> > > The attached patch updates the handling of %q+D, simplifying
> > > the implementation, and ensuring that it retains the range
> > > information of the decl, giving:
> > >
> > > diagnostic-ranges-1.c:6:7: warning: unused variable
> âredundantâ [-Wunused-variable]
> > > int redundant;
> > > ^~~~~~~~~
> >
> > For most of it I've convinced myself that it looks OK.
>
> Thanks.
>
> > > void
> > > -rich_location::set_range (unsigned int idx, source_range
> src_range,
> > > - bool show_caret_p, bool overwrite_loc_p)
> > > +rich_location::set_range (line_maps *set, unsigned int idx,
> > > + source_location loc, bool show_caret_p)
> > > {
> >
> > Here you need to update the function comment.
>
> Is the attached suitable?
> diff --git a/libcpp/line-map.c b/libcpp/line-map.c
> index 333e835..0c8f328 100644
> --- a/libcpp/line-map.c
> +++ b/libcpp/line-map.c
> @@ -2064,14 +2064,14 @@ rich_location::add_range (location_range *src_range)
> m_ranges[m_num_ranges++] = *src_range;
> }
>
> -/* Add or overwrite the range given by IDX. It must either
> - overwrite an existing range, or add one *exactly* on the end of
> +/* Add or overwrite the location given by IDX. It must either
> + overwrite an existing location, or add one *exactly* on the end of
> the array.
>
> This is primarily for use by gcc when implementing diagnostic
> format decoders e.g. the "+" in the C/C++ frontends, for handling
> format codes like "%q+D" (which writes the source location of a
> - tree back into range 0 of the rich_location).
> + tree back into location 0 of the rich_location).
Sorry; I realize now that the patch makes the final paragraph
inaccurate:
> If SHOW_CARET_P is true, then the range should be rendered with
> a caret at its starting location. This
...in that the range/location is actually printed with the caret at its
*caret* location, not its start location.
Updated patch to comment attached, which rewrites things to clarify the
meaning of SHOW_CARET_P.
[-- Attachment #2: comment-2.patch --]
[-- Type: text/x-patch, Size: 1523 bytes --]
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 333e835..209d0fb 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -2064,19 +2064,18 @@ rich_location::add_range (location_range *src_range)
m_ranges[m_num_ranges++] = *src_range;
}
-/* Add or overwrite the range given by IDX. It must either
- overwrite an existing range, or add one *exactly* on the end of
- the array.
-
- This is primarily for use by gcc when implementing diagnostic
- format decoders e.g. the "+" in the C/C++ frontends, for handling
- format codes like "%q+D" (which writes the source location of a
- tree back into range 0 of the rich_location).
-
- If SHOW_CARET_P is true, then the range should be rendered with
- a caret at its starting location. This
- is for use by the Fortran frontend, for implementing the
- "%C" and "%L" format codes. */
+/* Add or overwrite the location given by IDX, setting its location to LOC,
+ and setting its "should my caret be printed" flag to SHOW_CARET_P.
+
+ It must either overwrite an existing location, or add one *exactly* on
+ the end of the array.
+
+ This is primarily for use by gcc when implementing diagnostic format
+ decoders e.g.
+ - the "+" in the C/C++ frontends, for handling format codes like "%q+D"
+ (which writes the source location of a tree back into location 0 of
+ the rich_location), and
+ - the "%C" and "%L" format codes in the Fortran frontend. */
void
rich_location::set_range (line_maps *set, unsigned int idx,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix missing range information for "%q+D" format code
2015-12-04 21:09 ` David Malcolm
@ 2015-12-07 10:31 ` Bernd Schmidt
0 siblings, 0 replies; 5+ messages in thread
From: Bernd Schmidt @ 2015-12-07 10:31 UTC (permalink / raw)
To: David Malcolm; +Cc: gcc-patches
On 12/04/2015 10:09 PM, David Malcolm wrote:
> Updated patch to comment attached, which rewrites things to clarify the
> meaning of SHOW_CARET_P.
I guess this is OK for now. I think I'll go play with the Fortran
frontend a bit to see what exactly is going on with its use of set_range
there, I still think that function is a little clunky.
Bernd
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-12-07 10:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-03 20:13 [PATCH] Fix missing range information for "%q+D" format code David Malcolm
2015-12-04 11:09 ` Bernd Schmidt
2015-12-04 20:45 ` David Malcolm
2015-12-04 21:09 ` David Malcolm
2015-12-07 10:31 ` Bernd Schmidt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).