public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] PR c/68473: sanitize source range-printing within certain macro expansions
@ 2015-11-23 17:59 David Malcolm
  2015-11-23 18:13 ` Bernd Schmidt
  2015-12-10 16:24 ` Martin Sebor
  0 siblings, 2 replies; 10+ messages in thread
From: David Malcolm @ 2015-11-23 17:59 UTC (permalink / raw)
  To: gcc-patches; +Cc: David Malcolm

This patch fixes PR c/68473 by bulletproofing the new
diagnostic_show_locus implementation against ranges that finish before
they start (which can happen when using the C preprocessor), falling
back to simply printing a caret.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu; adds 7 new
PASS results to gcc.sum.

OK for trunk?

gcc/ChangeLog:
	PR c/68473
	* diagnostic-show-locus.c (layout::layout): Make loc_range const.
	Sanitize the layout_range against ranges that finish before they
	start.

gcc/testsuite/ChangeLog:
	PR c/68473
	* gcc.dg/plugin/diagnostic-test-expressions-1.c (fminl): New decl.
	(TEST_EQ): New macro.
	(test_macro): New function.
	* gcc.target/i386/pr68473-1.c: New test case.
---
 gcc/diagnostic-show-locus.c                        | 34 ++++++++++++++++++----
 .../gcc.dg/plugin/diagnostic-test-expressions-1.c  | 17 +++++++++++
 gcc/testsuite/gcc.target/i386/pr68473-1.c          | 24 +++++++++++++++
 3 files changed, 69 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr68473-1.c

diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index 9e51b95..968b3cb 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -444,7 +444,7 @@ layout::layout (diagnostic_context * context,
     {
       /* This diagnostic printer can only cope with "sufficiently sane" ranges.
 	 Ignore any ranges that are awkward to handle.  */
-      location_range *loc_range = richloc->get_range (idx);
+      const location_range *loc_range = richloc->get_range (idx);
 
       /* If any part of the range isn't in the same file as the primary
 	 location of this diagnostic, ignore the range.  */
@@ -456,16 +456,38 @@ layout::layout (diagnostic_context * context,
 	if (loc_range->m_caret.file != m_exploc.file)
 	  continue;
 
+      /* Everything is now known to be in the correct source file,
+	 but it may require further sanitization.  */
+      layout_range ri (loc_range);
+
+      /* If we have a range that finishes before it starts (perhaps
+	 from something built via macro expansion), printing the
+	 range is likely to be nonsensical.  Also, attempting to do so
+	 breaks assumptions within the printing code  (PR c/68473).  */
+      if (loc_range->m_start.line > loc_range->m_finish.line)
+	{
+	  /* Is this the primary location?  */
+	  if (m_layout_ranges.length () == 0)
+	    {
+	      /* We want to print the caret for the primary location, but
+		 we must sanitize away m_start and m_finish.  */
+	      ri.m_start = ri.m_caret;
+	      ri.m_finish = ri.m_caret;
+	    }
+	  else
+	    /* This is a non-primary range; ignore it.  */
+	    continue;
+	}
+
       /* Passed all the tests; add the range to m_layout_ranges so that
 	 it will be printed.  */
-      layout_range ri (loc_range);
       m_layout_ranges.safe_push (ri);
 
       /* Update m_first_line/m_last_line if necessary.  */
-      if (loc_range->m_start.line < m_first_line)
-	m_first_line = loc_range->m_start.line;
-      if (loc_range->m_finish.line > m_last_line)
-	m_last_line = loc_range->m_finish.line;
+      if (ri.m_start.m_line < m_first_line)
+	m_first_line = ri.m_start.m_line;
+      if (ri.m_finish.m_line > m_last_line)
+	m_last_line = ri.m_finish.m_line;
     }
 
   /* Adjust m_x_offset.
diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-expressions-1.c b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-expressions-1.c
index 0d8c7c5..3e38035 100644
--- a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-expressions-1.c
+++ b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-expressions-1.c
@@ -541,3 +541,20 @@ void test_quadratic (double a, double b, double c)
    { dg-end-multiline-output "" } */
 
 }
+
+/* Reproducer for PR c/68473.  */
+
+extern long double fminl (long double __x, long double __y);
+#define TEST_EQ(FUNC) FUNC##l(xl,xl)
+void test_macro (long double xl)
+{
+  __emit_expression_range (0, TEST_EQ (fmin) ); /* { dg-warning "range" } */
+/* { dg-begin-multiline-output "" }
+   __emit_expression_range (0, TEST_EQ (fmin) );
+                                        ^
+   { dg-end-multiline-output "" } */
+/* { dg-begin-multiline-output "" }
+ #define TEST_EQ(FUNC) FUNC##l(xl,xl)
+                       ^~~~
+   { dg-end-multiline-output "" } */
+}
diff --git a/gcc/testsuite/gcc.target/i386/pr68473-1.c b/gcc/testsuite/gcc.target/i386/pr68473-1.c
new file mode 100644
index 0000000..ffffaa7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr68473-1.c
@@ -0,0 +1,24 @@
+/* { dg-options "-fdiagnostics-show-caret -mno-fp-ret-in-387" } */
+
+extern long double fminl (long double __x, long double __y);
+
+#define TEST_EQ(FUNC) do { \
+  if ((long)FUNC##l(xl,xl) != (long)xl) \
+    return; \
+  } while (0)
+
+void
+foo (long double xl)
+{
+  TEST_EQ (fmin); /* { dg-error "x87 register return with x87 disabled" } */
+}
+
+/* { dg-begin-multiline-output "" }
+   TEST_EQ (fmin);
+            ^
+   { dg-end-multiline-output "" } */
+
+/* { dg-begin-multiline-output "" }
+   if ((long)FUNC##l(xl,xl) != (long)xl) \
+             ^~~~
+   { dg-end-multiline-output "" } */
-- 
1.8.5.3

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PR c/68473: sanitize source range-printing within certain macro expansions
  2015-11-23 17:59 [PATCH] PR c/68473: sanitize source range-printing within certain macro expansions David Malcolm
@ 2015-11-23 18:13 ` Bernd Schmidt
  2015-11-23 18:37   ` David Malcolm
  2015-12-10 16:24 ` Martin Sebor
  1 sibling, 1 reply; 10+ messages in thread
From: Bernd Schmidt @ 2015-11-23 18:13 UTC (permalink / raw)
  To: David Malcolm, gcc-patches

On 11/23/2015 06:52 PM, David Malcolm wrote:
> This patch fixes PR c/68473 by bulletproofing the new
> diagnostic_show_locus implementation against ranges that finish before
> they start (which can happen when using the C preprocessor), falling
> back to simply printing a caret.

Hmm, wouldn't it be better to avoid such a situation? Can you describe a 
bit more how exactly the macro expansion caused such a situation?


Bernd

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PR c/68473: sanitize source range-printing within certain macro expansions
  2015-11-23 18:13 ` Bernd Schmidt
@ 2015-11-23 18:37   ` David Malcolm
  2015-11-24 12:44     ` Bernd Schmidt
  0 siblings, 1 reply; 10+ messages in thread
From: David Malcolm @ 2015-11-23 18:37 UTC (permalink / raw)
  To: Bernd Schmidt; +Cc: gcc-patches

On Mon, 2015-11-23 at 18:59 +0100, Bernd Schmidt wrote:
> On 11/23/2015 06:52 PM, David Malcolm wrote:
> > This patch fixes PR c/68473 by bulletproofing the new
> > diagnostic_show_locus implementation against ranges that finish before
> > they start (which can happen when using the C preprocessor), falling
> > back to simply printing a caret.
> 
> Hmm, wouldn't it be better to avoid such a situation? Can you describe a 
> bit more how exactly the macro expansion caused such a situation?

The issue is here:

     1	/* { dg-options "-fdiagnostics-show-caret -mno-fp-ret-in-387" } */
     2	
     3	extern long double fminl (long double __x, long double __y);
     4	
     5	#define TEST_EQ(FUNC) do { \
     6	  if ((long)FUNC##l(xl,xl) != (long)xl) \
     7	    return; \
     8	  } while (0)
     9	
    10	void
    11	foo (long double xl)
    12	{
    13	  TEST_EQ (fmin); /* { dg-error "x87 register return with x87 disabled" } */
    14	}


    16	/* { dg-begin-multiline-output "" }
    17	   TEST_EQ (fmin);
    18	            ^
    19	   { dg-end-multiline-output "" } */
    20	
    21	/* { dg-begin-multiline-output "" }
    22	   if ((long)FUNC##l(xl,xl) != (long)xl) \
    23	             ^~~~
    24	   { dg-end-multiline-output "" } */

An error is emitted whilst expanding the macro at line 13, at
input_location.

This is at the expansion of this function call:

   fminl (xl, xl)

Normally we'd emit a source range like this for a function call:

   fminl (xl, xl)
   ^~~~~~~~~~~~~~

However, once we fully resolve locations, the "fmin" part of "fminl"
appears at line 13 here:

    13	  TEST_EQ (fmin);
                   ^~~~

giving the location of the caret, and start of the range, whereas the
rest of the the call is spelled here:

     6	  if ((long)FUNC##l(xl,xl) != (long)xl) \
                           ~~~~~~~

where the close paren gives the end of the range.

It would be wrong to try to print the whole range (anything might be
between lines 6 and 13).

In theory we could attempt to try to handle this kind of thing by
looking at the macro expansions, and to print something like:

    13	  TEST_EQ (fmin);
                   ^~~~
     6	  if ((long)FUNC##l(xl,xl) != (long)xl) \
                          ~~~~~~~~

or whatnot, but that strikes me as error-prone at this stage.


The patch instead detects such a situation, and tries to handle things
gracefully by falling back to simply printing a caret, without any
underlines:

pr68473-1.c: In function ‘foo’:
pr68473-1.c:13:12: error: x87 register return with x87 disabled
   TEST_EQ (fmin);
            ^

pr68473-1.c:6:13: note: in definition of macro ‘TEST_EQ’
   if ((long)FUNC##l(xl,xl) != (long)xl) \
             ^~~~


Dave

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PR c/68473: sanitize source range-printing within certain macro expansions
  2015-11-23 18:37   ` David Malcolm
@ 2015-11-24 12:44     ` Bernd Schmidt
  2015-12-11 18:25       ` [PATCH 0/3] " David Malcolm
  0 siblings, 1 reply; 10+ messages in thread
From: Bernd Schmidt @ 2015-11-24 12:44 UTC (permalink / raw)
  To: David Malcolm; +Cc: gcc-patches

On 11/23/2015 07:26 PM, David Malcolm wrote:
>
> In theory we could attempt to try to handle this kind of thing by
> looking at the macro expansions, and to print something like:
>
>      13	  TEST_EQ (fmin);
>                     ^~~~
>       6	  if ((long)FUNC##l(xl,xl) != (long)xl) \
>                            ~~~~~~~~
>
> or whatnot, but that strikes me as error-prone at this stage.

Could I ask you to spend some time looking at what would be involved at 
fixing it this way? In the end (assuming it doesn't prove to be a simple 
fix) we will probably go with your original patch for gcc-6, but it 
really goes against the grain to paper over a bug like this.


Bernd

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PR c/68473: sanitize source range-printing within certain macro expansions
  2015-11-23 17:59 [PATCH] PR c/68473: sanitize source range-printing within certain macro expansions David Malcolm
  2015-11-23 18:13 ` Bernd Schmidt
@ 2015-12-10 16:24 ` Martin Sebor
  1 sibling, 0 replies; 10+ messages in thread
From: Martin Sebor @ 2015-12-10 16:24 UTC (permalink / raw)
  To: David Malcolm, gcc-patches

On 11/23/2015 10:52 AM, David Malcolm wrote:
> This patch fixes PR c/68473 by bulletproofing the new
> diagnostic_show_locus implementation against ranges that finish before
> they start (which can happen when using the C preprocessor), falling
> back to simply printing a caret.
>
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu; adds 7 new
> PASS results to gcc.sum.

I just ran into the same ICE with a different (simpler) example
in bug 68839 and confirm that the patch fixes that one as well.

Martin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/3] Delay location expansion within rich_location until printing
  2015-12-11 18:25       ` [PATCH 0/3] " David Malcolm
  2015-12-11 18:25         ` [PATCH 2/3] Fix range/location terminology David Malcolm
  2015-12-11 18:25         ` [PATCH 3/3] PR c/68473: sanitize source range-printing within certain macro expansions (v2) David Malcolm
@ 2015-12-11 18:25         ` David Malcolm
  2015-12-14 12:39         ` [PATCH 0/3] Re: [PATCH] PR c/68473: sanitize source range-printing within certain macro expansions Bernd Schmidt
  3 siblings, 0 replies; 10+ messages in thread
From: David Malcolm @ 2015-12-11 18:25 UTC (permalink / raw)
  To: Bernd Schmidt; +Cc: gcc-patches, David Malcolm

Previously, source_location/location_t values passed to
rich_location were immediately expanded (to expanded_location
instances stored inside the rich_location).

This patch updates the insides of class rich_location to delay
this expansion until the insides of diagnostic_show_locus.

This simplifies the implementation, and allows for future fixes
to the printing code for handling PR c/68473 where we could do
something smarter about printing ranges within macro expansions
(since it gives us access to the relevant virtual locs from within
diagnostic_show_locus).  That idea isn't implemented in this patch
kit though; it merely has a patch for the PR to gracefully degrade
the diagnostic to showing a caret without ranges.

Delaying the expansion means that rich_location's ctor no longer
needs access to the line_table, so this param is dropped.

The change in internal representation means that it's simplest
to drop support for constructing rich_location instances from
source_range values: this was only used in plugins in the testsuite,
so I've updated those accordingly.

With the delayed expansion, source_range::debug becomes impossible
to implement without having it make a new location_t.  I've found that
I'm no longer using this debug function, so I've dropped it - these days
I generally debug this stuff by injecting calls to "inform" from
within gdb.

gcc/c-family/ChangeLog:
	* c-common.c (c_cpp_error): Remove line_table param from call to
	rich_location::set_range.

gcc/c/ChangeLog:
	* c-decl.c (warn_defaults_to): Remove line_table param from call to
	rich_location ctor.
	* c-errors.c (pedwarn_c99): Likewise.
	(pedwarn_c90): Likewise.
	* c-typeck.c (build_component_ref): Likewise.

gcc/cp/ChangeLog:
	* error.c (pedwarn_cxx98): Remove line_table param from call to
	rich_location ctor.

gcc/ChangeLog:
	* diagnostic-show-locus.c (layout_range::layout_range): Replace
	location_range param with three const expanded_locations * and a
	bool.
	(layout::layout): Replace call to
	rich_location::lazily_expand_location with get_expanded_location.
	Make loc_range const.  Extract the range and perform location
	expansion here, passing the results to the layout_range ctor.
	* diagnostic.c (diagnostic_append_note): Remove line_table param
	from call to rich_location ctor.
	(emit_diagnostic): Likewise.
	(inform): Likewise.
	(inform_n): Likewise.
	(warning): Likewise.
	(warning_at): Likewise.
	(warning_n): Likewise.
	(pedwarn): Likewise.
	(permerror): Likewise.
	(error): Likewise.
	(error_n): Likewise.
	(error_at): Likewise.
	(sorry): Likewise.
	(fatal_error): Likewise.
	(internal_error): Likewise.
	(internal_error_no_backtrace): Likewise.
	(source_range::debug): Delete.
	* diagnostic.h (diagnostic_expand_location): Reimplement in terms
	of rich_location::get_expanded_location.

gcc/fortran/ChangeLog:
	* error.c (gfc_warning): Remove line_table param from call to
	rich_location ctor.
	(gfc_warning_now_at): Likewise.
	(gfc_warning_now): Likewise.
	(gfc_error_now): Likewise.
	(gfc_fatal_error): Likewise.
	(gfc_error): Likewise.
	(gfc_internal_error): Likewise.

gcc/ChangeLog:
	* gcc-rich-location.c (get_range_for_expr): Delete.
	(gcc_rich_location::add_expr): Reimplement to avoid the
	rich_location::add_range overload that took a location_range,
	passing a location_t instead.
	* gcc-rich-location.h (gcc_rich_location::gcc_rich_location):
	Eliminate the overloaded ctor taking a source_range.
	Remove line_table param from call to rich_location ctor.
	* genmatch.c (fatal_at): Remove line_table param from call to
	rich_location ctor.
	(fatal_at): Likewise.
	(warning_at): Likewise.
	* pretty-print.c (text_info::set_location): Likewise for call to
	rich_location::set_range.
	* rtl-error.c (diagnostic_for_asm): Likewise for call to
	rich_location ctor.

gcc/testsuite/ChangeLog:
	* gcc.dg/plugin/diagnostic_plugin_show_trees.c
	(get_range_for_expr): Delete, as per gcc-rich-location.c
	(gcc_rich_location::add_expr): Update as per gcc-rich-location.c.
	(show_tree): Drop range information from call to
	inform_at_rich_loc.
	* gcc.dg/plugin/diagnostic_plugin_test_show_locus.c (add_range):
	New.
	(test_show_locus): Replace calls to rich_location::add_range with
	calls to add_range.  Rewrite the tests that used the now-defunct
	rich_location ctor taking a source_range.  Simplify other tests
	by replacing calls to COMBINE_LOCATION_DATA with calls to
	make_location.

libcpp/ChangeLog:
	* errors.c (cpp_diagnostic): Remove line_table param from call to
	rich_location ctor.
	(cpp_diagnostic_with_line): Likewise.
	* include/line-map.h (source_range::debug): Delete.
	(struct location_range): Update comment.  Replace
	expanded_location fields "m_start", "m_finish", and "m_caret" with
	a source_location field: "m_loc".
	(class rich_location): Reword comment.
	(rich_location::rich_location): Drop line_maps * param from ctor;
	delete ctor taking a source_range.
	(rich_location::get_loc): Reimplement in terms of a new overloaded
	variant which takes an unsigned int.
	(rich_location::get_loc_addr): Delete.
	(rich_location::add_range): Drop params "start" and "finish" in
	favor of param "loc".  Drop overloaded variants taking a
	source_range or location_range *.
	(rich_location::set_range): Drop line_maps * param.
	(rich_location::lazily_expand_location): Delete in favor of...
	(rich_location::get_expanded_location): New decl.
	(rich_location::m_loc): Delete field.
	(rich_location::m_column_override): New field.
	* line-map.c (rich_location::rich_location):  Drop line_maps *
	param, initialization of deleted fields.  Reimplement body
	as a call to add_range.  Delete overloaded variant taking a
	source_range.
	(rich_location::get_loc): New function.
	(rich_location::lazily_expand_location): Delete in favor of...
	(rich_location::get_expanded_location): New function.
	(rich_location::override_column): Reimplement.
	(rich_location::add_range): Drop params "start" and "finish" in
	favor of param "loc".  Eliminate location expansion in favor of
	simply storing loc.  Drop overloaded variants taking a
	source_range or location_range *.
	(rich_location::set_range): Drop line_maps * param.  Eliminate
	location expansion.
---
 gcc/c-family/c-common.c                            |   2 +-
 gcc/c/c-decl.c                                     |   2 +-
 gcc/c/c-errors.c                                   |   4 +-
 gcc/c/c-typeck.c                                   |   2 +-
 gcc/cp/error.c                                     |   2 +-
 gcc/diagnostic-show-locus.c                        |  49 ++++++---
 gcc/diagnostic.c                                   |  46 +++------
 gcc/diagnostic.h                                   |   2 +-
 gcc/fortran/error.c                                |  14 +--
 gcc/gcc-rich-location.c                            |  28 +-----
 gcc/gcc-rich-location.h                            |  11 +--
 gcc/genmatch.c                                     |   8 +-
 gcc/pretty-print.c                                 |   2 +-
 gcc/rtl-error.c                                    |   2 +-
 .../gcc.dg/plugin/diagnostic_plugin_show_trees.c   |  33 +------
 .../plugin/diagnostic_plugin_test_show_locus.c     | 109 ++++++++++-----------
 libcpp/errors.c                                    |   4 +-
 libcpp/include/line-map.h                          |  63 ++++--------
 libcpp/line-map.c                                  | 109 ++++++++-------------
 19 files changed, 186 insertions(+), 306 deletions(-)

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 9bc02fc..509a0ca 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -10161,7 +10161,7 @@ c_cpp_error (cpp_reader *pfile ATTRIBUTE_UNUSED, int level, int reason,
       gcc_unreachable ();
     }
   if (done_lexing)
-    richloc->set_range (line_table, 0, input_location, true);
+    richloc->set_range (0, input_location, true);
   diagnostic_set_info_translated (&diagnostic, msg, ap,
 				  richloc, dlevel);
   diagnostic_override_option_index (&diagnostic,
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index adac2b6..2d56f34 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -5320,7 +5320,7 @@ warn_defaults_to (location_t location, int opt, const char *gmsgid, ...)
 {
   diagnostic_info diagnostic;
   va_list ap;
-  rich_location richloc (line_table, location);
+  rich_location richloc (location);
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, &richloc,
diff --git a/gcc/c/c-errors.c b/gcc/c/c-errors.c
index ee9c2b5..ef0f9a2 100644
--- a/gcc/c/c-errors.c
+++ b/gcc/c/c-errors.c
@@ -37,7 +37,7 @@ pedwarn_c99 (location_t location, int opt, const char *gmsgid, ...)
   diagnostic_info diagnostic;
   va_list ap;
   bool warned = false;
-  rich_location richloc (line_table, location);
+  rich_location richloc (location);
 
   va_start (ap, gmsgid);
   /* If desired, issue the C99/C11 compat warning, which is more specific
@@ -76,7 +76,7 @@ pedwarn_c90 (location_t location, int opt, const char *gmsgid, ...)
 {
   diagnostic_info diagnostic;
   va_list ap;
-  rich_location richloc (line_table, location);
+  rich_location richloc (location);
 
   va_start (ap, gmsgid);
   /* Warnings such as -Wvla are the most specific ones.  */
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index e45f2d2..252d023 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -2399,7 +2399,7 @@ build_component_ref (location_t loc, tree datum, tree component)
     {
       /* Special-case the error message for "ptr.field" for the case
 	 where the user has confused "." vs "->".  */
-      rich_location richloc (line_table, loc);
+      rich_location richloc (loc);
       /* "loc" should be the "." token.  */
       richloc.add_fixit_replace (source_range::from_location (loc), "->");
       error_at_rich_loc (&richloc,
diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index e0ba806..62dc228 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -3673,7 +3673,7 @@ pedwarn_cxx98 (location_t location, int opt, const char *gmsgid, ...)
   diagnostic_info diagnostic;
   va_list ap;
   bool ret;
-  rich_location richloc (line_table, location);
+  rich_location richloc (location);
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, &richloc,
diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index 9e51b95..16004d8 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -117,7 +117,10 @@ class layout_point
 class layout_range
 {
  public:
-  layout_range (const location_range *loc_range);
+  layout_range (const expanded_location *start_exploc,
+		const expanded_location *finish_exploc,
+		bool show_caret_p,
+		const expanded_location *caret_exploc);
 
   bool contains_point (int row, int column) const;
 
@@ -287,11 +290,14 @@ colorizer::finish_state (int state)
    Initialize various layout_point fields from expanded_location
    equivalents; we've already filtered on file.  */
 
-layout_range::layout_range (const location_range *loc_range)
-: m_start (loc_range->m_start),
-  m_finish (loc_range->m_finish),
-  m_show_caret_p (loc_range->m_show_caret_p),
-  m_caret (loc_range->m_caret)
+layout_range::layout_range (const expanded_location *start_exploc,
+			    const expanded_location *finish_exploc,
+			    bool show_caret_p,
+			    const expanded_location *caret_exploc)
+: m_start (*start_exploc),
+  m_finish (*finish_exploc),
+  m_show_caret_p (show_caret_p),
+  m_caret (*caret_exploc)
 {
 }
 
@@ -431,7 +437,7 @@ layout::layout (diagnostic_context * context,
 : m_context (context),
   m_pp (context->printer),
   m_diagnostic_kind (diagnostic->kind),
-  m_exploc (diagnostic->richloc->lazily_expand_location ()),
+  m_exploc (diagnostic->richloc->get_expanded_location (0)),
   m_colorizer (context, diagnostic),
   m_colorize_source_p (context->colorize_source_p),
   m_layout_ranges (rich_location::MAX_RANGES),
@@ -444,28 +450,39 @@ layout::layout (diagnostic_context * context,
     {
       /* This diagnostic printer can only cope with "sufficiently sane" ranges.
 	 Ignore any ranges that are awkward to handle.  */
-      location_range *loc_range = richloc->get_range (idx);
+      const location_range *loc_range = richloc->get_range (idx);
+
+      /* Split the "range" into caret and range information.  */
+      source_range src_range = get_range_from_loc (line_table, loc_range->m_loc);
+
+      /* Expand the various locations.  */
+      expanded_location start
+	= linemap_client_expand_location_to_spelling_point (src_range.m_start);
+      expanded_location finish
+	= linemap_client_expand_location_to_spelling_point (src_range.m_finish);
+      expanded_location caret
+	= linemap_client_expand_location_to_spelling_point (loc_range->m_loc);
 
       /* If any part of the range isn't in the same file as the primary
 	 location of this diagnostic, ignore the range.  */
-      if (loc_range->m_start.file != m_exploc.file)
+      if (start.file != m_exploc.file)
 	continue;
-      if (loc_range->m_finish.file != m_exploc.file)
+      if (finish.file != m_exploc.file)
 	continue;
       if (loc_range->m_show_caret_p)
-	if (loc_range->m_caret.file != m_exploc.file)
+	if (caret.file != m_exploc.file)
 	  continue;
 
       /* Passed all the tests; add the range to m_layout_ranges so that
 	 it will be printed.  */
-      layout_range ri (loc_range);
+      layout_range ri (&start, &finish, loc_range->m_show_caret_p, &caret);
       m_layout_ranges.safe_push (ri);
 
       /* Update m_first_line/m_last_line if necessary.  */
-      if (loc_range->m_start.line < m_first_line)
-	m_first_line = loc_range->m_start.line;
-      if (loc_range->m_finish.line > m_last_line)
-	m_last_line = loc_range->m_finish.line;
+      if (ri.m_start.m_line < m_first_line)
+	m_first_line = ri.m_start.m_line;
+      if (ri.m_finish.m_line > m_last_line)
+	m_last_line = ri.m_finish.m_line;
     }
 
   /* Adjust m_x_offset.
diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index b4d3a7d..b6dbc6a 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -867,7 +867,7 @@ diagnostic_append_note (diagnostic_context *context,
   diagnostic_info diagnostic;
   va_list ap;
   const char *saved_prefix;
-  rich_location richloc (line_table, location);
+  rich_location richloc (location);
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, &richloc, DK_NOTE);
@@ -925,7 +925,7 @@ emit_diagnostic (diagnostic_t kind, location_t location, int opt,
   diagnostic_info diagnostic;
   va_list ap;
   bool ret;
-  rich_location richloc (line_table, location);
+  rich_location richloc (location);
 
   va_start (ap, gmsgid);
   if (kind == DK_PERMERROR)
@@ -952,7 +952,7 @@ inform (location_t location, const char *gmsgid, ...)
 {
   diagnostic_info diagnostic;
   va_list ap;
-  rich_location richloc (line_table, location);
+  rich_location richloc (location);
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, &richloc, DK_NOTE);
@@ -981,7 +981,7 @@ inform_n (location_t location, int n, const char *singular_gmsgid,
 {
   diagnostic_info diagnostic;
   va_list ap;
-  rich_location richloc (line_table, location);
+  rich_location richloc (location);
 
   va_start (ap, plural_gmsgid);
   diagnostic_set_info_translated (&diagnostic,
@@ -1000,7 +1000,7 @@ warning (int opt, const char *gmsgid, ...)
   diagnostic_info diagnostic;
   va_list ap;
   bool ret;
-  rich_location richloc (line_table, input_location);
+  rich_location richloc (input_location);
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, &richloc, DK_WARNING);
@@ -1021,7 +1021,7 @@ warning_at (location_t location, int opt, const char *gmsgid, ...)
   diagnostic_info diagnostic;
   va_list ap;
   bool ret;
-  rich_location richloc (line_table, location);
+  rich_location richloc (location);
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, &richloc, DK_WARNING);
@@ -1059,7 +1059,7 @@ warning_n (location_t location, int opt, int n, const char *singular_gmsgid,
   diagnostic_info diagnostic;
   va_list ap;
   bool ret;
-  rich_location richloc (line_table, location);
+  rich_location richloc (location);
 
   va_start (ap, plural_gmsgid);
   diagnostic_set_info_translated (&diagnostic,
@@ -1091,7 +1091,7 @@ pedwarn (location_t location, int opt, const char *gmsgid, ...)
   diagnostic_info diagnostic;
   va_list ap;
   bool ret;
-  rich_location richloc (line_table, location);
+  rich_location richloc (location);
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, &richloc,  DK_PEDWARN);
@@ -1114,7 +1114,7 @@ permerror (location_t location, const char *gmsgid, ...)
   diagnostic_info diagnostic;
   va_list ap;
   bool ret;
-  rich_location richloc (line_table, location);
+  rich_location richloc (location);
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, &richloc,
@@ -1150,7 +1150,7 @@ error (const char *gmsgid, ...)
 {
   diagnostic_info diagnostic;
   va_list ap;
-  rich_location richloc (line_table, input_location);
+  rich_location richloc (input_location);
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, &richloc, DK_ERROR);
@@ -1166,7 +1166,7 @@ error_n (location_t location, int n, const char *singular_gmsgid,
 {
   diagnostic_info diagnostic;
   va_list ap;
-  rich_location richloc (line_table, location);
+  rich_location richloc (location);
 
   va_start (ap, plural_gmsgid);
   diagnostic_set_info_translated (&diagnostic,
@@ -1182,7 +1182,7 @@ error_at (location_t loc, const char *gmsgid, ...)
 {
   diagnostic_info diagnostic;
   va_list ap;
-  rich_location richloc (line_table, loc);
+  rich_location richloc (loc);
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, &richloc, DK_ERROR);
@@ -1213,7 +1213,7 @@ sorry (const char *gmsgid, ...)
 {
   diagnostic_info diagnostic;
   va_list ap;
-  rich_location richloc (line_table, input_location);
+  rich_location richloc (input_location);
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, &richloc, DK_SORRY);
@@ -1237,7 +1237,7 @@ fatal_error (location_t loc, const char *gmsgid, ...)
 {
   diagnostic_info diagnostic;
   va_list ap;
-  rich_location richloc (line_table, loc);
+  rich_location richloc (loc);
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, &richloc, DK_FATAL);
@@ -1256,7 +1256,7 @@ internal_error (const char *gmsgid, ...)
 {
   diagnostic_info diagnostic;
   va_list ap;
-  rich_location richloc (line_table, input_location);
+  rich_location richloc (input_location);
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, &richloc, DK_ICE);
@@ -1274,7 +1274,7 @@ internal_error_no_backtrace (const char *gmsgid, ...)
 {
   diagnostic_info diagnostic;
   va_list ap;
-  rich_location richloc (line_table, input_location);
+  rich_location richloc (input_location);
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, &richloc, DK_ICE_NOBT);
@@ -1341,17 +1341,3 @@ real_abort (void)
 {
   abort ();
 }
-
-/* Display the given source_range instance, with MSG as a descriptive
-   comment.  This issues a "note" diagnostic at the range.
-
-   This is declared within libcpp, but implemented here, since it
-   makes use of the diagnostic-printing machinery.  */
-
-DEBUG_FUNCTION void
-source_range::debug (const char *msg) const
-{
-  rich_location richloc (line_table, m_start);
-  richloc.add_range (m_start, m_finish, false);
-  inform_at_rich_loc (&richloc, "%s", msg);
-}
diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
index 9096e16..6794262 100644
--- a/gcc/diagnostic.h
+++ b/gcc/diagnostic.h
@@ -332,7 +332,7 @@ diagnostic_num_locations (const diagnostic_info * diagnostic)
 static inline expanded_location
 diagnostic_expand_location (const diagnostic_info * diagnostic, int which = 0)
 {
-  return diagnostic->richloc->get_range (which)->m_caret;
+  return diagnostic->richloc->get_expanded_location (which);
 }
 
 /* This is somehow the right-side margin of a caret line, that is, we
diff --git a/gcc/fortran/error.c b/gcc/fortran/error.c
index 8f57aff..3312441 100644
--- a/gcc/fortran/error.c
+++ b/gcc/fortran/error.c
@@ -773,7 +773,7 @@ gfc_warning (int opt, const char *gmsgid, va_list ap)
   va_copy (argp, ap);
 
   diagnostic_info diagnostic;
-  rich_location rich_loc (line_table, UNKNOWN_LOCATION);
+  rich_location rich_loc (UNKNOWN_LOCATION);
   bool fatal_errors = global_dc->fatal_errors;
   pretty_printer *pp = global_dc->printer;
   output_buffer *tmp_buffer = pp->buffer;
@@ -1119,7 +1119,7 @@ gfc_warning_now_at (location_t loc, int opt, const char *gmsgid, ...)
 {
   va_list argp;
   diagnostic_info diagnostic;
-  rich_location rich_loc (line_table, loc);
+  rich_location rich_loc (loc);
   bool ret;
 
   va_start (argp, gmsgid);
@@ -1137,7 +1137,7 @@ gfc_warning_now (int opt, const char *gmsgid, ...)
 {
   va_list argp;
   diagnostic_info diagnostic;
-  rich_location rich_loc (line_table, UNKNOWN_LOCATION);
+  rich_location rich_loc (UNKNOWN_LOCATION);
   bool ret;
 
   va_start (argp, gmsgid);
@@ -1157,7 +1157,7 @@ gfc_error_now (const char *gmsgid, ...)
 {
   va_list argp;
   diagnostic_info diagnostic;
-  rich_location rich_loc (line_table, UNKNOWN_LOCATION);
+  rich_location rich_loc (UNKNOWN_LOCATION);
 
   error_buffer.flag = true;
 
@@ -1175,7 +1175,7 @@ gfc_fatal_error (const char *gmsgid, ...)
 {
   va_list argp;
   diagnostic_info diagnostic;
-  rich_location rich_loc (line_table, UNKNOWN_LOCATION);
+  rich_location rich_loc (UNKNOWN_LOCATION);
 
   va_start (argp, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &argp, &rich_loc, DK_FATAL);
@@ -1241,7 +1241,7 @@ gfc_error (const char *gmsgid, va_list ap)
     }
 
   diagnostic_info diagnostic;
-  rich_location richloc (line_table, UNKNOWN_LOCATION);
+  rich_location richloc (UNKNOWN_LOCATION);
   bool fatal_errors = global_dc->fatal_errors;
   pretty_printer *pp = global_dc->printer;
   output_buffer *tmp_buffer = pp->buffer;
@@ -1287,7 +1287,7 @@ gfc_internal_error (const char *gmsgid, ...)
 {
   va_list argp;
   diagnostic_info diagnostic;
-  rich_location rich_loc (line_table, UNKNOWN_LOCATION);
+  rich_location rich_loc (UNKNOWN_LOCATION);
 
   va_start (argp, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &argp, &rich_loc, DK_ICE);
diff --git a/gcc/gcc-rich-location.c b/gcc/gcc-rich-location.c
index b0ec47b..88bd82d 100644
--- a/gcc/gcc-rich-location.c
+++ b/gcc/gcc-rich-location.c
@@ -41,28 +41,6 @@ along with GCC; see the file COPYING3.  If not see
 #include "cpplib.h"
 #include "diagnostic.h"
 
-/* Extract any source range information from EXPR and write it
-   to *R.  */
-
-static bool
-get_range_for_expr (tree expr, location_range *r)
-{
-  if (EXPR_HAS_RANGE (expr))
-    {
-      source_range sr = EXPR_LOCATION_RANGE (expr);
-
-      /* Do we have meaningful data?  */
-      if (sr.m_start && sr.m_finish)
-	{
-	  r->m_start = expand_location (sr.m_start);
-	  r->m_finish = expand_location (sr.m_finish);
-	  return true;
-	}
-    }
-
-  return false;
-}
-
 /* Add a range to the rich_location, covering expression EXPR. */
 
 void
@@ -70,10 +48,8 @@ gcc_rich_location::add_expr (tree expr)
 {
   gcc_assert (expr);
 
-  location_range r;
-  r.m_show_caret_p = false;
-  if (get_range_for_expr (expr, &r))
-    add_range (&r);
+  if (CAN_HAVE_RANGE_P (expr))
+    add_range (EXPR_LOCATION (expr), false);
 }
 
 /* If T is an expression, add a range for it to the rich_location.  */
diff --git a/gcc/gcc-rich-location.h b/gcc/gcc-rich-location.h
index 2f9291d..60e9e88 100644
--- a/gcc/gcc-rich-location.h
+++ b/gcc/gcc-rich-location.h
@@ -25,16 +25,9 @@ along with GCC; see the file COPYING3.  If not see
 class gcc_rich_location : public rich_location
 {
  public:
-  /* Constructors.  */
-
-  /* Constructing from a location.  */
+  /* Constructor.  */
   gcc_rich_location (source_location loc) :
-    rich_location (line_table, loc) {}
-
-  /* Constructing from a source_range.  */
-  gcc_rich_location (source_range src_range) :
-    rich_location (src_range) {}
-
+    rich_location (loc) {}
 
   /* Methods for adding ranges via gcc entities.  */
   void
diff --git a/gcc/genmatch.c b/gcc/genmatch.c
index ef39cb0..c06279a 100644
--- a/gcc/genmatch.c
+++ b/gcc/genmatch.c
@@ -119,7 +119,7 @@ __attribute__((format (printf, 2, 3)))
 #endif
 fatal_at (const cpp_token *tk, const char *msg, ...)
 {
-  rich_location richloc (line_table, tk->src_loc);
+  rich_location richloc (tk->src_loc);
   va_list ap;
   va_start (ap, msg);
   error_cb (NULL, CPP_DL_FATAL, 0, &richloc, msg, &ap);
@@ -132,7 +132,7 @@ __attribute__((format (printf, 2, 3)))
 #endif
 fatal_at (source_location loc, const char *msg, ...)
 {
-  rich_location richloc (line_table, loc);
+  rich_location richloc (loc);
   va_list ap;
   va_start (ap, msg);
   error_cb (NULL, CPP_DL_FATAL, 0, &richloc, msg, &ap);
@@ -145,7 +145,7 @@ __attribute__((format (printf, 2, 3)))
 #endif
 warning_at (const cpp_token *tk, const char *msg, ...)
 {
-  rich_location richloc (line_table, tk->src_loc);
+  rich_location richloc (tk->src_loc);
   va_list ap;
   va_start (ap, msg);
   error_cb (NULL, CPP_DL_WARNING, 0, &richloc, msg, &ap);
@@ -158,7 +158,7 @@ __attribute__((format (printf, 2, 3)))
 #endif
 warning_at (source_location loc, const char *msg, ...)
 {
-  rich_location richloc (line_table, loc);
+  rich_location richloc (loc);
   va_list ap;
   va_start (ap, msg);
   error_cb (NULL, CPP_DL_WARNING, 0, &richloc, msg, &ap);
diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c
index 3365074..cd35792 100644
--- a/gcc/pretty-print.c
+++ b/gcc/pretty-print.c
@@ -38,7 +38,7 @@ void
 text_info::set_location (unsigned int idx, location_t loc, bool show_caret_p)
 {
   gcc_checking_assert (m_richloc);
-  m_richloc->set_range (line_table, idx, loc, show_caret_p);
+  m_richloc->set_range (idx, loc, show_caret_p);
 }
 
 location_t
diff --git a/gcc/rtl-error.c b/gcc/rtl-error.c
index 088bb8a..96da2bd 100644
--- a/gcc/rtl-error.c
+++ b/gcc/rtl-error.c
@@ -67,7 +67,7 @@ diagnostic_for_asm (const rtx_insn *insn, const char *msg, va_list *args_ptr,
 		    diagnostic_t kind)
 {
   diagnostic_info diagnostic;
-  rich_location richloc (line_table, location_for_asm (insn));
+  rich_location richloc (location_for_asm (insn));
 
   diagnostic_set_info (&diagnostic, msg, args_ptr,
 		       &richloc, kind);
diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_show_trees.c b/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_show_trees.c
index 5a911c1..8b1d1b7 100644
--- a/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_show_trees.c
+++ b/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_show_trees.c
@@ -42,25 +42,6 @@
   So as a nasty workaround, the following material is copied&pasted
   from gcc-rich-location.c: */
 
-static bool
-get_range_for_expr (tree expr, location_range *r)
-{
-  if (EXPR_HAS_RANGE (expr))
-    {
-      source_range sr = EXPR_LOCATION_RANGE (expr);
-
-      /* Do we have meaningful data?  */
-      if (sr.m_start && sr.m_finish)
-	{
-	  r->m_start = expand_location (sr.m_start);
-	  r->m_finish = expand_location (sr.m_finish);
-	  return true;
-	}
-    }
-
-  return false;
-}
-
 /* Add a range to the rich_location, covering expression EXPR. */
 
 void
@@ -68,10 +49,8 @@ gcc_rich_location::add_expr (tree expr)
 {
   gcc_assert (expr);
 
-  location_range r;
-  r.m_show_caret_p = false;
-  if (get_range_for_expr (expr, &r))
-    add_range (&r);
+  if (CAN_HAVE_RANGE_P (expr))
+    add_range (EXPR_LOCATION (expr), false);
 }
 
 /* FIXME: end of material taken from gcc-rich-location.c */
@@ -96,13 +75,7 @@ show_tree (tree node)
   enum tree_code code = TREE_CODE (node);
 
   location_range *range = richloc.get_range (1);
-  inform_at_rich_loc (&richloc,
-		      "%s at range %i:%i-%i:%i",
-		      get_tree_code_name (code),
-		      range->m_start.line,
-		      range->m_start.column,
-		      range->m_finish.line,
-		      range->m_finish.column);
+  inform_at_rich_loc (&richloc, "%s", get_tree_code_name (code));
 
   /* Recurse.  */
   int min_idx = 0;
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 02a2aef..3f9d139 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
@@ -140,6 +140,15 @@ custom_diagnostic_finalizer (diagnostic_context *context,
   pp_newline_and_flush (context->printer);
 }
 
+/* Add a location to RICHLOC with caret==start at START, ranging to FINISH.  */
+
+static void
+add_range (rich_location *richloc, location_t start, location_t finish,
+	   bool show_caret_p)
+{
+  richloc->add_range (make_location (start, start, finish), show_caret_p);
+}
+
 /* Exercise the diagnostic machinery to emit various warnings,
    for use by diagnostic-test-show-locus-*.c.
 
@@ -164,54 +173,49 @@ test_show_locus (function *fun)
   if (0 == strcmp (fnname, "test_simple"))
     {
       const int line = fnstart_line + 2;
-      rich_location richloc (line_table, get_loc (line, 15));
-      richloc.add_range (get_loc (line, 10), get_loc (line, 14), false);
-      richloc.add_range (get_loc (line, 16), get_loc (line, 16), false);
+      rich_location richloc (get_loc (line, 15));
+      add_range (&richloc, get_loc (line, 10), get_loc (line, 14), false);
+      add_range (&richloc, get_loc (line, 16), get_loc (line, 16), false);
       warning_at_rich_loc (&richloc, 0, "test");
     }
 
   if (0 == strcmp (fnname, "test_simple_2"))
     {
       const int line = fnstart_line + 2;
-      rich_location richloc (line_table, get_loc (line, 24));
-      richloc.add_range (get_loc (line, 6),
-			 get_loc (line, 22), false);
-      richloc.add_range (get_loc (line, 26),
-			 get_loc (line, 43), false);
+      rich_location richloc (get_loc (line, 24));
+      add_range (&richloc, get_loc (line, 6), get_loc (line, 22), false);
+      add_range (&richloc, get_loc (line, 26), get_loc (line, 43), false);
       warning_at_rich_loc (&richloc, 0, "test");
     }
 
   if (0 == strcmp (fnname, "test_multiline"))
     {
       const int line = fnstart_line + 2;
-      rich_location richloc (line_table, get_loc (line + 1, 7));
-      richloc.add_range (get_loc (line, 7),
-			 get_loc (line, 23), false);
-      richloc.add_range (get_loc (line + 1, 9),
-			 get_loc (line + 1, 26), false);
+      rich_location richloc (get_loc (line + 1, 7));
+      add_range (&richloc, get_loc (line, 7), get_loc (line, 23), false);
+      add_range (&richloc, get_loc (line + 1, 9), get_loc (line + 1, 26),
+		 false);
       warning_at_rich_loc (&richloc, 0, "test");
     }
 
   if (0 == strcmp (fnname, "test_many_lines"))
     {
       const int line = fnstart_line + 2;
-      rich_location richloc (line_table, get_loc (line + 5, 7));
-      richloc.add_range (get_loc (line, 7),
-			 get_loc (line + 4, 65), false);
-      richloc.add_range (get_loc (line + 5, 9),
-			 get_loc (line + 10, 61), false);
+      rich_location richloc (get_loc (line + 5, 7));
+      add_range (&richloc, get_loc (line, 7), get_loc (line + 4, 65), false);
+      add_range (&richloc, get_loc (line + 5, 9), get_loc (line + 10, 61),
+		 false);
       warning_at_rich_loc (&richloc, 0, "test");
     }
 
-  /* Example of a rich_location constructed directly from a
-     source_range where the range is larger than one character.  */
+  /* Example of a rich_location where the range is larger than
+     one character.  */
   if (0 == strcmp (fnname, "test_richloc_from_proper_range"))
     {
       const int line = fnstart_line + 2;
-      source_range src_range;
-      src_range.m_start = get_loc (line, 12);
-      src_range.m_finish = get_loc (line, 16);
-      rich_location richloc (src_range);
+      location_t start = get_loc (line, 12);
+      location_t finish = get_loc (line, 16);
+      rich_location richloc (make_location (start, start, finish));
       warning_at_rich_loc (&richloc, 0, "test");
     }
 
@@ -220,15 +224,9 @@ test_show_locus (function *fun)
   if (0 == strcmp (fnname, "test_caret_within_proper_range"))
     {
       const int line = fnstart_line + 2;
-      location_t caret = get_loc (line, 16);
-      source_range src_range;
-      src_range.m_start = get_loc (line, 12);
-      src_range.m_finish = get_loc (line, 20);
-      location_t combined_loc = COMBINE_LOCATION_DATA (line_table,
-						       caret,
-						       src_range,
-						       NULL);
-      warning_at (combined_loc, 0, "test");
+      warning_at (make_location (get_loc (line, 16), get_loc (line, 12),
+				 get_loc (line, 20)),
+		  0, "test");
     }
 
   /* Example of a very wide line, where the information of interest
@@ -236,15 +234,9 @@ test_show_locus (function *fun)
   if (0 == strcmp (fnname, "test_very_wide_line"))
     {
       const int line = fnstart_line + 2;
-      location_t caret = get_loc (line, 94);
-      source_range src_range;
-      src_range.m_start = get_loc (line, 90);
-      src_range.m_finish = get_loc (line, 98);
-      location_t combined_loc = COMBINE_LOCATION_DATA (line_table,
-						       caret,
-						       src_range,
-						       NULL);
-      warning_at (combined_loc, 0, "test");
+      warning_at (make_location (get_loc (line, 94), get_loc (line, 90),
+				 get_loc (line, 98)),
+		  0, "test");
     }
 
   /* Example of multiple carets.  */
@@ -253,8 +245,8 @@ test_show_locus (function *fun)
       const int line = fnstart_line + 2;
       location_t caret_a = get_loc (line, 7);
       location_t caret_b = get_loc (line, 11);
-      rich_location richloc (line_table, caret_a);
-      richloc.add_range (caret_b, caret_b, true);
+      rich_location richloc (caret_a);
+      add_range (&richloc, caret_b, caret_b, true);
       global_dc->caret_chars[0] = 'A';
       global_dc->caret_chars[1] = 'B';
       warning_at_rich_loc (&richloc, 0, "test");
@@ -266,11 +258,10 @@ test_show_locus (function *fun)
   if (0 == strcmp (fnname, "test_fixit_insert"))
     {
       const int line = fnstart_line + 2;
-      source_range src_range;
-      src_range.m_start = get_loc (line, 19);
-      src_range.m_finish = get_loc (line, 22);
-      rich_location richloc (src_range);
-      richloc.add_fixit_insert (src_range.m_start, "{");
+      location_t start = get_loc (line, 19);
+      location_t finish = get_loc (line, 22);
+      rich_location richloc (make_location (start, start, finish));
+      richloc.add_fixit_insert (start, "{");
       richloc.add_fixit_insert (get_loc (line, 23), "}");
       warning_at_rich_loc (&richloc, 0, "example of insertion hints");
     }
@@ -278,10 +269,12 @@ test_show_locus (function *fun)
   if (0 == strcmp (fnname, "test_fixit_remove"))
     {
       const int line = fnstart_line + 2;
+      location_t start = get_loc (line, 8);
+      location_t finish = get_loc (line, 8);
+      rich_location richloc (make_location (start, start, finish));
       source_range src_range;
-      src_range.m_start = get_loc (line, 8);
-      src_range.m_finish = get_loc (line, 8);
-      rich_location richloc (src_range);
+      src_range.m_start = start;
+      src_range.m_finish = finish;
       richloc.add_fixit_remove (src_range);
       warning_at_rich_loc (&richloc, 0, "example of a removal hint");
     }
@@ -289,10 +282,12 @@ test_show_locus (function *fun)
   if (0 == strcmp (fnname, "test_fixit_replace"))
     {
       const int line = fnstart_line + 2;
+      location_t start = get_loc (line, 2);
+      location_t finish = get_loc (line, 19);
+      rich_location richloc (make_location (start, start, finish));
       source_range src_range;
-      src_range.m_start = get_loc (line, 2);
-      src_range.m_finish = get_loc (line, 19);
-      rich_location richloc (src_range);
+      src_range.m_start = start;
+      src_range.m_finish = finish;
       richloc.add_fixit_replace (src_range, "gtk_widget_show_all");
       warning_at_rich_loc (&richloc, 0, "example of a replacement hint");
     }
@@ -309,8 +304,8 @@ test_show_locus (function *fun)
       const int line = fnstart_line + 3;
       location_t caret_a = get_loc (line, 5);
       location_t caret_b = get_loc (line - 1, 19);
-      rich_location richloc (line_table, caret_a);
-      richloc.add_range (caret_b, caret_b, true);
+      rich_location richloc (caret_a);
+      richloc.add_range (caret_b, true);
       global_dc->caret_chars[0] = '1';
       global_dc->caret_chars[1] = '2';
       warning_at_rich_loc (&richloc, 0, "test");
diff --git a/libcpp/errors.c b/libcpp/errors.c
index c27a417..32c01e6 100644
--- a/libcpp/errors.c
+++ b/libcpp/errors.c
@@ -57,7 +57,7 @@ cpp_diagnostic (cpp_reader * pfile, int level, int reason,
 
   if (!pfile->cb.error)
     abort ();
-  rich_location richloc (pfile->line_table, src_loc);
+  rich_location richloc (src_loc);
   ret = pfile->cb.error (pfile, level, reason, &richloc, _(msgid), ap);
 
   return ret;
@@ -140,7 +140,7 @@ cpp_diagnostic_with_line (cpp_reader * pfile, int level, int reason,
   
   if (!pfile->cb.error)
     abort ();
-  rich_location richloc (pfile->line_table, src_loc);
+  rich_location richloc (src_loc);
   richloc.override_column (column);
   ret = pfile->cb.error (pfile, level, reason, &richloc, _(msgid), ap);
 
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 73c583e..0cd3c1d 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -273,20 +273,6 @@ struct GTY(()) source_range
   source_location m_start;
   source_location m_finish;
 
-  /* Display this source_range instance, with MSG as a descriptive
-     comment.  This issues a "note" diagnostic at the range, using
-     gcc's diagnostic machinery.
-
-     This is declared here, but is implemented within gcc/diagnostic.c,
-     since it makes use of gcc's diagnostic-printing machinery.  This
-     is a slight layering violation, but this is sufficiently useful
-     for debugging that it's worth it.
-
-     This declaration would have a DEBUG_FUNCTION annotation, but that
-     is implemented in gcc/system.h and thus is not available here in
-     libcpp.  */
-  void debug (const char *msg) const;
-
   /* We avoid using constructors, since various structs that
      don't yet have constructors will embed instances of
      source_range.  */
@@ -1249,13 +1235,12 @@ typedef struct
 
    i.e. "3:1:" in GCC corresponds to "(3, 0)" in Emacs.  */
 
-/* Ranges are closed
-   m_start is the first location within the range, and
-   m_finish is the last location within the range.  */
+/* A location within a rich_location: a caret&range, with
+   the caret potentially flagged for display.  */
+
 struct location_range
 {
-  expanded_location m_start;
-  expanded_location m_finish;
+  source_location m_loc;
 
   /* Should a caret be drawn for this range?  Typically this is
      true for the 0th range, and false for subsequent ranges,
@@ -1267,7 +1252,6 @@ struct location_range
 
      where "1" and "2" are notionally carets.  */
   bool m_show_caret_p;
-  expanded_location m_caret;
 };
 
 class fixit_hint;
@@ -1276,9 +1260,10 @@ class fixit_hint;
   class fixit_replace;
 
 /* A "rich" source code location, for use when printing diagnostics.
-   A rich_location has one or more ranges, each optionally with
-   a caret.   Typically the zeroth range has a caret; other ranges
-   sometimes have carets.
+   A rich_location has one or more carets&ranges, where the carets
+   are optional.  These are referred to as "ranges" from here.
+   Typically the zeroth range has a caret; other ranges sometimes
+   have carets.
 
    The "primary" location of a rich_location is the caret of range 0,
    used for determining the line/column when printing diagnostic
@@ -1349,35 +1334,21 @@ class fixit_hint;
 class rich_location
 {
  public:
-  /* Constructors.  */
-
-  /* Constructing from a location.  */
-  rich_location (line_maps *set, source_location loc);
-
-  /* Constructing from a source_range.  */
-  rich_location (source_range src_range);
+  /* Constructor.  */
+  rich_location (source_location loc);
 
   /* Destructor.  */
   ~rich_location ();
 
   /* Accessors.  */
-  source_location get_loc () const { return m_loc; }
-
-  source_location *get_loc_addr () { return &m_loc; }
+  source_location get_loc () const { return get_loc (0); }
+  source_location get_loc (unsigned int idx) const;
 
   void
-  add_range (source_location start, source_location finish,
-	     bool show_caret_p);
+  add_range (source_location loc,  bool show_caret_p);
 
   void
-  add_range (source_range src_range, bool show_caret_p);
-
-  void
-  add_range (location_range *src_range);
-
-  void
-  set_range (line_maps *set, unsigned int idx, source_location loc,
-	     bool show_caret_p);
+  set_range (unsigned int idx, source_location loc, bool show_caret_p);
 
   unsigned int get_num_locations () const { return m_num_ranges; }
 
@@ -1387,7 +1358,7 @@ class rich_location
     return &m_ranges[idx];
   }
 
-  expanded_location lazily_expand_location ();
+  expanded_location get_expanded_location (unsigned int idx);
 
   void
   override_column (int column);
@@ -1412,11 +1383,11 @@ public:
   static const int MAX_FIXIT_HINTS = 2;
 
 protected:
-  source_location m_loc;
-
   unsigned int m_num_ranges;
   location_range m_ranges[MAX_RANGES];
 
+  int m_column_override;
+
   bool m_have_expanded_location;
   expanded_location m_expanded_location;
 
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 209d0fb..6bdf841 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -1973,29 +1973,13 @@ source_range::intersects_line_p (const char *file, int line) const
 
 /* Construct a rich_location with location LOC as its initial range.  */
 
-rich_location::rich_location (line_maps *set, source_location loc) :
-  m_loc (loc),
+rich_location::rich_location (source_location loc) :
   m_num_ranges (0),
+  m_column_override (0),
   m_have_expanded_location (false),
   m_num_fixit_hints (0)
 {
-  /* Set up the 0th range, extracting any range from LOC.  */
-  source_range src_range = get_range_from_loc (set, loc);
-  add_range (src_range, true);
-  m_ranges[0].m_caret = lazily_expand_location ();
-}
-
-/* Construct a rich_location with source_range SRC_RANGE as its
-   initial range.  */
-
-rich_location::rich_location (source_range src_range)
-: m_loc (src_range.m_start),
-  m_num_ranges (0),
-  m_have_expanded_location (false),
-  m_num_fixit_hints (0)
-{
-  /* Set up the 0th range: */
-  add_range (src_range, true);
+  add_range (loc, true);
 }
 
 /* The destructor for class rich_location.  */
@@ -2006,64 +1990,60 @@ rich_location::~rich_location ()
     delete m_fixit_hints[i];
 }
 
-/* Get an expanded_location for this rich_location's primary
-   location.  */
+/* Get location IDX within this rich_location.  */
+
+source_location
+rich_location::get_loc (unsigned int idx) const
+{
+  linemap_assert (idx < m_num_ranges);
+  return m_ranges[idx].m_loc;
+}
+
+/* Expand location IDX within this rich_location.  */
 
 expanded_location
-rich_location::lazily_expand_location ()
+rich_location::get_expanded_location (unsigned int idx)
 {
-  if (!m_have_expanded_location)
+  if (idx == 0)
     {
-      m_expanded_location
-	= linemap_client_expand_location_to_spelling_point (m_loc);
-      m_have_expanded_location = true;
-    }
+      /* Cache the expansion of the primary location.  */
+      if (!m_have_expanded_location)
+	{
+	  m_expanded_location
+	    = linemap_client_expand_location_to_spelling_point (get_loc (0));
+	  if (m_column_override)
+	    m_expanded_location.column = m_column_override;
+	  m_have_expanded_location = true;
+	}
 
-  return m_expanded_location;
+      return m_expanded_location;
+    }
+  else
+    return linemap_client_expand_location_to_spelling_point (get_loc (idx));
 }
 
-/* Set the column of the primary location.  */
+/* Potentially set the column of the primary location, with 0 meaning
+   "don't override it".  */
 
 void
 rich_location::override_column (int column)
 {
-  lazily_expand_location ();
-  m_expanded_location.column = column;
+  m_column_override = column;
+  m_have_expanded_location = false;
 }
 
 /* Add the given range.  */
 
 void
-rich_location::add_range (source_location start, source_location finish,
-			  bool show_caret_p)
+rich_location::add_range (source_location loc, bool show_caret_p)
 {
   linemap_assert (m_num_ranges < MAX_RANGES);
 
   location_range *range = &m_ranges[m_num_ranges++];
-  range->m_start = linemap_client_expand_location_to_spelling_point (start);
-  range->m_finish = linemap_client_expand_location_to_spelling_point (finish);
-  range->m_caret = range->m_start;
+  range->m_loc = loc;
   range->m_show_caret_p = show_caret_p;
 }
 
-/* Add the given range.  */
-
-void
-rich_location::add_range (source_range src_range, bool show_caret_p)
-{
-  linemap_assert (m_num_ranges < MAX_RANGES);
-
-  add_range (src_range.m_start, src_range.m_finish, show_caret_p);
-}
-
-void
-rich_location::add_range (location_range *src_range)
-{
-  linemap_assert (m_num_ranges < MAX_RANGES);
-
-  m_ranges[m_num_ranges++] = *src_range;
-}
-
 /* 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.
 
@@ -2078,8 +2058,8 @@ rich_location::add_range (location_range *src_range)
    - the "%C" and "%L" format codes in the Fortran frontend.  */
 
 void
-rich_location::set_range (line_maps *set, unsigned int idx,
-			  source_location loc, bool show_caret_p)
+rich_location::set_range (unsigned int idx, source_location loc,
+			  bool show_caret_p)
 {
   linemap_assert (idx < MAX_RANGES);
 
@@ -2087,28 +2067,17 @@ rich_location::set_range (line_maps *set, unsigned int idx,
      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);
-  locrange->m_finish
-    = linemap_client_expand_location_to_spelling_point (src_range.m_finish);
-
+  locrange->m_loc = loc;
   locrange->m_show_caret_p = show_caret_p;
-  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)
-    {
-      m_loc = loc;
-      /* Mark any cached value here as dirty.  */
-      m_have_expanded_location = false;
-    }
+    /* Mark any cached value here as dirty.  */
+    m_have_expanded_location = false;
 }
 
 /* Add a fixit-hint, suggesting insertion of NEW_CONTENT
-- 
1.8.5.3

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/3] Fix range/location terminology
  2015-12-11 18:25       ` [PATCH 0/3] " David Malcolm
@ 2015-12-11 18:25         ` David Malcolm
  2015-12-11 18:25         ` [PATCH 3/3] PR c/68473: sanitize source range-printing within certain macro expansions (v2) David Malcolm
                           ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: David Malcolm @ 2015-12-11 18:25 UTC (permalink / raw)
  To: Bernd Schmidt; +Cc: gcc-patches, David Malcolm

The terminology within rich_location has become muddled, and
with the simplifications of the previous patch, I'd like to
rename things to better reflect what's going on.

A rich_location can contain one more more nested locations, each
of which can have a start, a finish, and optionally a caret.
These nested locations are essentially a location_t plus a flag,
but for historical reasons the current code confusingly refers to
them as "ranges".

This patch consolidates the terminology throughout rich_location
and diagnostic_show_locus; I believe it's a significant
clarification.

"struct location_range" becomes "struct nested_location"
Various "range" within rich_location become "location" e.g.
"add_range" becomes "add_location".

I didn't rename class layout_range within diagnostic-show-locus.c
as this is relatively localized, and I can't think of a better name
for it.

gcc/c-family/ChangeLog:
	* c-common.c (c_cpp_error): Update for renaming of
	rich_location::set_range to set_location.

gcc/ChangeLog:
	* diagnostic-show-locus.c (class layout_range): Update
	for "range" to "location" renamings.
	(layout::layout): Likewise, updating wording of comments.
	* diagnostic.c (diagnostic_initialize): Likewise.
	* diagnostic.h (struct diagnostic_context): Likewise.
	* gcc-rich-location.c (gcc_rich_location::add_expr): Likewise.
	(gcc_rich_location::maybe_add_expr): Likewise.
	* pretty-print.c (text_info::set_location): Likewise.

gcc/testsuite/ChangeLog:
	* gcc.dg/plugin/diagnostic_plugin_show_trees.c
	(gcc_rich_location::add_expr): Update for "range" to "location"
	renamings.
	(show_tree): Drop redundant local "range".
	* gcc.dg/plugin/diagnostic_plugin_test_show_locus.c (add_range):
	Rename to...
	(add_location): ...this.  Update for corresponding renaming of
	rich_location method.
	(test_show_locus): Update for renamings.

libcpp/ChangeLog:
	* include/line-map.h (struct location_range): Rename to...
	(struct nested_location): ...this, and update wording of comment.
	(class rich_location): Update comment.
	(rich_location::add_range): Rename to...
	(rich_location::add_location): ...this.
	(rich_location::set_range): Rename to...
	(rich_location::set_location): ...this.
	(rich_location::get_num_locations): Update for renamings.
	(rich_location::get_range): Rename to...
	(rich_location::get_location): ...this.
	(rich_location::MAX_RANGES): Rename to...
	(rich_location::MAX_LOCATIONS): ...this.
	(rich_location::m_num_ranges): Rename to...
	(rich_location::m_num_locations): ...this.
	(rich_location::m_locations): Update for renaming of
	location_range to nested_location.
	* line-map.c (rich_location::rich_location): Update for renamings.
	(rich_location::get_loc): Update for renamings.
	(rich_location::get_expanded_location): Update comment.
	(rich_location::override_column): Likewise.
	(rich_location::add_range): Rename to...
	(rich_location::add_location): ...this, updating for renamings,
	and renaming local "range" to "nested_loc".
	(rich_location::set_range): Rename to...
	(rich_location::set_location): ...this, updating for renamings,
	renaming local "locrange" to "nested_loc".
---
 gcc/c-family/c-common.c                            |  2 +-
 gcc/diagnostic-show-locus.c                        | 24 ++++-----
 gcc/diagnostic.c                                   |  2 +-
 gcc/diagnostic.h                                   |  2 +-
 gcc/gcc-rich-location.c                            |  6 +--
 gcc/pretty-print.c                                 |  4 +-
 .../gcc.dg/plugin/diagnostic_plugin_show_trees.c   |  3 +-
 .../plugin/diagnostic_plugin_test_show_locus.c     | 24 ++++-----
 libcpp/include/line-map.h                          | 61 +++++++++++-----------
 libcpp/line-map.c                                  | 47 +++++++++--------
 10 files changed, 88 insertions(+), 87 deletions(-)

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 509a0ca..ab61031 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -10161,7 +10161,7 @@ c_cpp_error (cpp_reader *pfile ATTRIBUTE_UNUSED, int level, int reason,
       gcc_unreachable ();
     }
   if (done_lexing)
-    richloc->set_range (0, input_location, true);
+    richloc->set_location (0, input_location, true);
   diagnostic_set_info_translated (&diagnostic, msg, ap,
 				  richloc, dlevel);
   diagnostic_override_option_index (&diagnostic,
diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index 16004d8..f279019 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -112,7 +112,7 @@ class layout_point
   int m_column;
 };
 
-/* A class for use by "class layout" below: a filtered location_range.  */
+/* A class for use by "class layout" below: a filtered nested_location.  */
 
 class layout_range
 {
@@ -440,7 +440,7 @@ layout::layout (diagnostic_context * context,
   m_exploc (diagnostic->richloc->get_expanded_location (0)),
   m_colorizer (context, diagnostic),
   m_colorize_source_p (context->colorize_source_p),
-  m_layout_ranges (rich_location::MAX_RANGES),
+  m_layout_ranges (rich_location::MAX_LOCATIONS),
   m_first_line (m_exploc.line),
   m_last_line  (m_exploc.line),
   m_x_offset (0)
@@ -448,12 +448,12 @@ layout::layout (diagnostic_context * context,
   rich_location *richloc = diagnostic->richloc;
   for (unsigned int idx = 0; idx < richloc->get_num_locations (); idx++)
     {
-      /* This diagnostic printer can only cope with "sufficiently sane" ranges.
-	 Ignore any ranges that are awkward to handle.  */
-      const location_range *loc_range = richloc->get_range (idx);
+      /* This diagnostic printer can only cope with "sufficiently sane"
+	 locations. Ignore any that are awkward to handle.  */
+      const nested_location *nested_loc = richloc->get_location (idx);
 
-      /* Split the "range" into caret and range information.  */
-      source_range src_range = get_range_from_loc (line_table, loc_range->m_loc);
+      /* Split the location into caret and range information.  */
+      source_range src_range = get_range_from_loc (line_table, nested_loc->m_loc);
 
       /* Expand the various locations.  */
       expanded_location start
@@ -461,21 +461,21 @@ layout::layout (diagnostic_context * context,
       expanded_location finish
 	= linemap_client_expand_location_to_spelling_point (src_range.m_finish);
       expanded_location caret
-	= linemap_client_expand_location_to_spelling_point (loc_range->m_loc);
+	= linemap_client_expand_location_to_spelling_point (nested_loc->m_loc);
 
-      /* If any part of the range isn't in the same file as the primary
+      /* If any part of the location isn't in the same file as the primary
 	 location of this diagnostic, ignore the range.  */
       if (start.file != m_exploc.file)
 	continue;
       if (finish.file != m_exploc.file)
 	continue;
-      if (loc_range->m_show_caret_p)
+      if (nested_loc->m_show_caret_p)
 	if (caret.file != m_exploc.file)
 	  continue;
 
-      /* Passed all the tests; add the range to m_layout_ranges so that
+      /* Passed all the tests; add the location to m_layout_ranges so that
 	 it will be printed.  */
-      layout_range ri (&start, &finish, loc_range->m_show_caret_p, &caret);
+      layout_range ri (&start, &finish, nested_loc->m_show_caret_p, &caret);
       m_layout_ranges.safe_push (ri);
 
       /* Update m_first_line/m_last_line if necessary.  */
diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index b6dbc6a..d14afce 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -144,7 +144,7 @@ diagnostic_initialize (diagnostic_context *context, int n_opts)
     context->classify_diagnostic[i] = DK_UNSPECIFIED;
   context->show_caret = false;
   diagnostic_set_caret_max_width (context, pp_line_cutoff (context->printer));
-  for (i = 0; i < rich_location::MAX_RANGES; i++)
+  for (i = 0; i < rich_location::MAX_LOCATIONS; i++)
     context->caret_chars[i] = '^';
   context->show_option_requested = false;
   context->abort_on_error = false;
diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
index 6794262..0e2297e 100644
--- a/gcc/diagnostic.h
+++ b/gcc/diagnostic.h
@@ -105,7 +105,7 @@ struct diagnostic_context
   int caret_max_width;
 
   /* Character used for caret diagnostics.  */
-  char caret_chars[rich_location::MAX_RANGES];
+  char caret_chars[rich_location::MAX_LOCATIONS];
 
   /* True if we should print the command line option which controls
      each diagnostic, if known.  */
diff --git a/gcc/gcc-rich-location.c b/gcc/gcc-rich-location.c
index 88bd82d..15c6376 100644
--- a/gcc/gcc-rich-location.c
+++ b/gcc/gcc-rich-location.c
@@ -41,7 +41,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "cpplib.h"
 #include "diagnostic.h"
 
-/* Add a range to the rich_location, covering expression EXPR. */
+/* Add a location to the rich_location, covering expression EXPR. */
 
 void
 gcc_rich_location::add_expr (tree expr)
@@ -49,10 +49,10 @@ gcc_rich_location::add_expr (tree expr)
   gcc_assert (expr);
 
   if (CAN_HAVE_RANGE_P (expr))
-    add_range (EXPR_LOCATION (expr), false);
+    add_location (EXPR_LOCATION (expr), false);
 }
 
-/* If T is an expression, add a range for it to the rich_location.  */
+/* If T is an expression, add a location for it to the rich_location.  */
 
 void
 gcc_rich_location::maybe_add_expr (tree t)
diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c
index cd35792..d972d9b 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 given location/range within this text_info's rich_location.
+/* Overwrite the given location within this text_info's rich_location.
    For use e.g. when implementing "+" in client format decoders.  */
 
 void
 text_info::set_location (unsigned int idx, location_t loc, bool show_caret_p)
 {
   gcc_checking_assert (m_richloc);
-  m_richloc->set_range (idx, loc, show_caret_p);
+  m_richloc->set_location (idx, loc, show_caret_p);
 }
 
 location_t
diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_show_trees.c b/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_show_trees.c
index 8b1d1b7..5457d01 100644
--- a/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_show_trees.c
+++ b/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_show_trees.c
@@ -50,7 +50,7 @@ gcc_rich_location::add_expr (tree expr)
   gcc_assert (expr);
 
   if (CAN_HAVE_RANGE_P (expr))
-    add_range (EXPR_LOCATION (expr), false);
+    add_location (EXPR_LOCATION (expr), false);
 }
 
 /* FIXME: end of material taken from gcc-rich-location.c */
@@ -74,7 +74,6 @@ show_tree (tree node)
 
   enum tree_code code = TREE_CODE (node);
 
-  location_range *range = richloc.get_range (1);
   inform_at_rich_loc (&richloc, "%s", get_tree_code_name (code));
 
   /* Recurse.  */
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 3f9d139..cb8b4f6 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
@@ -143,10 +143,10 @@ custom_diagnostic_finalizer (diagnostic_context *context,
 /* Add a location to RICHLOC with caret==start at START, ranging to FINISH.  */
 
 static void
-add_range (rich_location *richloc, location_t start, location_t finish,
+add_location (rich_location *richloc, location_t start, location_t finish,
 	   bool show_caret_p)
 {
-  richloc->add_range (make_location (start, start, finish), show_caret_p);
+  richloc->add_location (make_location (start, start, finish), show_caret_p);
 }
 
 /* Exercise the diagnostic machinery to emit various warnings,
@@ -174,8 +174,8 @@ test_show_locus (function *fun)
     {
       const int line = fnstart_line + 2;
       rich_location richloc (get_loc (line, 15));
-      add_range (&richloc, get_loc (line, 10), get_loc (line, 14), false);
-      add_range (&richloc, get_loc (line, 16), get_loc (line, 16), false);
+      add_location (&richloc, get_loc (line, 10), get_loc (line, 14), false);
+      add_location (&richloc, get_loc (line, 16), get_loc (line, 16), false);
       warning_at_rich_loc (&richloc, 0, "test");
     }
 
@@ -183,8 +183,8 @@ test_show_locus (function *fun)
     {
       const int line = fnstart_line + 2;
       rich_location richloc (get_loc (line, 24));
-      add_range (&richloc, get_loc (line, 6), get_loc (line, 22), false);
-      add_range (&richloc, get_loc (line, 26), get_loc (line, 43), false);
+      add_location (&richloc, get_loc (line, 6), get_loc (line, 22), false);
+      add_location (&richloc, get_loc (line, 26), get_loc (line, 43), false);
       warning_at_rich_loc (&richloc, 0, "test");
     }
 
@@ -192,8 +192,8 @@ test_show_locus (function *fun)
     {
       const int line = fnstart_line + 2;
       rich_location richloc (get_loc (line + 1, 7));
-      add_range (&richloc, get_loc (line, 7), get_loc (line, 23), false);
-      add_range (&richloc, get_loc (line + 1, 9), get_loc (line + 1, 26),
+      add_location (&richloc, get_loc (line, 7), get_loc (line, 23), false);
+      add_location (&richloc, get_loc (line + 1, 9), get_loc (line + 1, 26),
 		 false);
       warning_at_rich_loc (&richloc, 0, "test");
     }
@@ -202,8 +202,8 @@ test_show_locus (function *fun)
     {
       const int line = fnstart_line + 2;
       rich_location richloc (get_loc (line + 5, 7));
-      add_range (&richloc, get_loc (line, 7), get_loc (line + 4, 65), false);
-      add_range (&richloc, get_loc (line + 5, 9), get_loc (line + 10, 61),
+      add_location (&richloc, get_loc (line, 7), get_loc (line + 4, 65), false);
+      add_location (&richloc, get_loc (line + 5, 9), get_loc (line + 10, 61),
 		 false);
       warning_at_rich_loc (&richloc, 0, "test");
     }
@@ -246,7 +246,7 @@ test_show_locus (function *fun)
       location_t caret_a = get_loc (line, 7);
       location_t caret_b = get_loc (line, 11);
       rich_location richloc (caret_a);
-      add_range (&richloc, caret_b, caret_b, true);
+      add_location (&richloc, caret_b, caret_b, true);
       global_dc->caret_chars[0] = 'A';
       global_dc->caret_chars[1] = 'B';
       warning_at_rich_loc (&richloc, 0, "test");
@@ -305,7 +305,7 @@ test_show_locus (function *fun)
       location_t caret_a = get_loc (line, 5);
       location_t caret_b = get_loc (line - 1, 19);
       rich_location richloc (caret_a);
-      richloc.add_range (caret_b, true);
+      richloc.add_location (caret_b, true);
       global_dc->caret_chars[0] = '1';
       global_dc->caret_chars[1] = '2';
       warning_at_rich_loc (&richloc, 0, "test");
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 0cd3c1d..498e43c 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1238,12 +1238,12 @@ typedef struct
 /* A location within a rich_location: a caret&range, with
    the caret potentially flagged for display.  */
 
-struct location_range
+struct nested_location
 {
   source_location m_loc;
 
-  /* Should a caret be drawn for this range?  Typically this is
-     true for the 0th range, and false for subsequent ranges,
+  /* Should a caret be drawn for this location?  Typically this is
+     true for the 0th location, and false for subsequent locations,
      but the Fortran frontend overrides this for rendering things like:
 
        x = x + y
@@ -1260,18 +1260,19 @@ class fixit_hint;
   class fixit_replace;
 
 /* A "rich" source code location, for use when printing diagnostics.
-   A rich_location has one or more carets&ranges, where the carets
-   are optional.  These are referred to as "ranges" from here.
-   Typically the zeroth range has a caret; other ranges sometimes
-   have carets.
+   A rich_location has one or more nested_location instances within it.
+   Each nested_location has a caret and a ranges, where the carets are
+   optional.  These nested locations are referred to as "locations" from
+   here.  Typically the zeroth location has a caret; other locations
+   sometimes have carets.
 
-   The "primary" location of a rich_location is the caret of range 0,
+   The "primary" location of a rich_location is the caret of location 0,
    used for determining the line/column when printing diagnostic
    text, such as:
 
       some-file.c:3:1: error: ...etc...
 
-   Additional ranges may be added to help the user identify other
+   Additional locations may be added to help the user identify other
    pertinent clauses in a diagnostic.
 
    rich_location instances are intended to be allocated on the stack
@@ -1284,14 +1285,14 @@ class fixit_hint;
    *********
       int i = "foo";
               ^
-   This "rich" location is simply a single range (range 0), with
+   This "rich" location is simply a single location (location 0), with
    caret = start = finish at the given point.
 
    Example B
    *********
       a = (foo && bar)
           ~~~~~^~~~~~~
-   This rich location has a single range (range 0), with the caret
+   This rich location has a single location (location 0), with the caret
    at the first "&", and the start/finish at the parentheses.
    Compare with example C below.
 
@@ -1299,13 +1300,13 @@ class fixit_hint;
    *********
       a = (foo && bar)
            ~~~ ^~ ~~~
-   This rich location has three ranges:
-   - Range 0 has its caret and start location at the first "&" and
+   This rich location has three locations:
+   - Location 0 has its caret and start location at the first "&" and
      end at the second "&.
-   - Range 1 has its start and finish at the "f" and "o" of "foo";
+   - Location 1 has its start and finish at the "f" and "o" of "foo";
      the caret is not flagged for display, but is perhaps at the "f"
      of "foo".
-   - Similarly, range 2 has its start and finish at the "b" and "r" of
+   - Similarly, location 2 has its start and finish at the "b" and "r" of
      "bar"; the caret is not flagged for display, but is perhaps at the
      "b" of "bar".
    Compare with example B above.
@@ -1314,10 +1315,10 @@ class fixit_hint;
    ****************************
        x = x + y
            1   2
-   This rich location has range 0 at "1", and range 1 at "2".
-   Both are flagged for caret display.  Both ranges have start/finish
+   This rich location has location 0 at "1", and location 1 at "2".
+   Both are flagged for caret display.  Both locations have start/finish
    equal to their caret point.  The frontend overrides the diagnostic
-   context's default caret character for these ranges.
+   context's default caret character for these locations.
 
    Example E
    *********
@@ -1325,10 +1326,10 @@ class fixit_hint;
                                ^~
               100, 101, 102);
                    ~~~
-   This rich location has two ranges:
-   - range 0 is at the "%s" with start = caret = "%" and finish at
+   This rich location has two locations:
+   - location 0 is at the "%s" with start = caret = "%" and finish at
      the "s".
-   - range 1 has start/finish covering the "101" and is not flagged for
+   - location 1 has start/finish covering the "101" and is not flagged for
      caret printing; it is perhaps at the start of "101".  */
 
 class rich_location
@@ -1345,17 +1346,17 @@ class rich_location
   source_location get_loc (unsigned int idx) const;
 
   void
-  add_range (source_location loc,  bool show_caret_p);
+  add_location (source_location loc,  bool show_caret_p);
 
   void
-  set_range (unsigned int idx, source_location loc, bool show_caret_p);
+  set_location (unsigned int idx, source_location loc, bool show_caret_p);
 
-  unsigned int get_num_locations () const { return m_num_ranges; }
+  unsigned int get_num_locations () const { return m_num_locations; }
 
-  location_range *get_range (unsigned int idx)
+  const nested_location *get_location (unsigned int idx)
   {
-    linemap_assert (idx < m_num_ranges);
-    return &m_ranges[idx];
+    linemap_assert (idx < m_num_locations);
+    return &m_locations[idx];
   }
 
   expanded_location get_expanded_location (unsigned int idx);
@@ -1379,12 +1380,12 @@ class rich_location
   fixit_hint *get_fixit_hint (int idx) const { return m_fixit_hints[idx]; }
 
 public:
-  static const int MAX_RANGES = 3;
+  static const int MAX_LOCATIONS = 3;
   static const int MAX_FIXIT_HINTS = 2;
 
 protected:
-  unsigned int m_num_ranges;
-  location_range m_ranges[MAX_RANGES];
+  unsigned int m_num_locations;
+  nested_location m_locations[MAX_LOCATIONS];
 
   int m_column_override;
 
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 6bdf841..5ca4440 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -1974,12 +1974,12 @@ source_range::intersects_line_p (const char *file, int line) const
 /* Construct a rich_location with location LOC as its initial range.  */
 
 rich_location::rich_location (source_location loc) :
-  m_num_ranges (0),
+  m_num_locations (0),
   m_column_override (0),
   m_have_expanded_location (false),
   m_num_fixit_hints (0)
 {
-  add_range (loc, true);
+  add_location (loc, true);
 }
 
 /* The destructor for class rich_location.  */
@@ -1995,11 +1995,12 @@ rich_location::~rich_location ()
 source_location
 rich_location::get_loc (unsigned int idx) const
 {
-  linemap_assert (idx < m_num_ranges);
-  return m_ranges[idx].m_loc;
+  linemap_assert (idx < m_num_locations);
+  return m_locations[idx].m_loc;
 }
 
-/* Expand location IDX within this rich_location.  */
+/* Expand location IDX within this rich_location,
+   potentially overriding the column.   */
 
 expanded_location
 rich_location::get_expanded_location (unsigned int idx)
@@ -2022,8 +2023,8 @@ rich_location::get_expanded_location (unsigned int idx)
     return linemap_client_expand_location_to_spelling_point (get_loc (idx));
 }
 
-/* Potentially set the column of the primary location, with 0 meaning
-   "don't override it".  */
+/*  Set the column of the primary location (as reported by
+    get_expanded_location) with 0 meaning "don't override it".  */
 
 void
 rich_location::override_column (int column)
@@ -2032,16 +2033,16 @@ rich_location::override_column (int column)
   m_have_expanded_location = false;
 }
 
-/* Add the given range.  */
+/* Add the given location.  */
 
 void
-rich_location::add_range (source_location loc, bool show_caret_p)
+rich_location::add_location (source_location loc, bool show_caret_p)
 {
-  linemap_assert (m_num_ranges < MAX_RANGES);
+  linemap_assert (m_num_locations < MAX_LOCATIONS);
 
-  location_range *range = &m_ranges[m_num_ranges++];
-  range->m_loc = loc;
-  range->m_show_caret_p = show_caret_p;
+  nested_location *nested_loc = &m_locations[m_num_locations++];
+  nested_loc->m_loc = loc;
+  nested_loc->m_show_caret_p = show_caret_p;
 }
 
 /* Add or overwrite the location given by IDX, setting its location to LOC,
@@ -2058,22 +2059,22 @@ rich_location::add_range (source_location loc, bool show_caret_p)
    - the "%C" and "%L" format codes in the Fortran frontend.  */
 
 void
-rich_location::set_range (unsigned int idx, source_location loc,
-			  bool show_caret_p)
+rich_location::set_location (unsigned int idx, source_location loc,
+			     bool show_caret_p)
 {
-  linemap_assert (idx < MAX_RANGES);
+  linemap_assert (idx < MAX_LOCATIONS);
 
   /* We can either overwrite an existing range, or add one exactly
      on the end of the array.  */
-  linemap_assert (idx <= m_num_ranges);
+  linemap_assert (idx <= m_num_locations);
 
-  location_range *locrange = &m_ranges[idx];
-  locrange->m_loc = loc;
-  locrange->m_show_caret_p = show_caret_p;
+  nested_location *nested_loc = &m_locations[idx];
+  nested_loc->m_loc = loc;
+  nested_loc->m_show_caret_p = show_caret_p;
 
-  /* Are we adding a range onto the end?  */
-  if (idx == m_num_ranges)
-    m_num_ranges = idx + 1;
+  /* Are we adding a location onto the end?  */
+  if (idx == m_num_locations)
+    m_num_locations = idx + 1;
 
   if (idx == 0)
     /* Mark any cached value here as dirty.  */
-- 
1.8.5.3

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 3/3] PR c/68473: sanitize source range-printing within certain macro expansions (v2)
  2015-12-11 18:25       ` [PATCH 0/3] " David Malcolm
  2015-12-11 18:25         ` [PATCH 2/3] Fix range/location terminology David Malcolm
@ 2015-12-11 18:25         ` David Malcolm
  2015-12-11 18:25         ` [PATCH 1/3] Delay location expansion within rich_location until printing David Malcolm
  2015-12-14 12:39         ` [PATCH 0/3] Re: [PATCH] PR c/68473: sanitize source range-printing within certain macro expansions Bernd Schmidt
  3 siblings, 0 replies; 10+ messages in thread
From: David Malcolm @ 2015-12-11 18:25 UTC (permalink / raw)
  To: Bernd Schmidt; +Cc: gcc-patches, David Malcolm

Changes in v2:
  - add two more testcases, based on additional reproducers
    reported (one to the PR, another to a dup); now adds 17 PASS
    results to gcc.sum.
  - update for delayed expansion of locations within rich_location
    and the terminology fixes from the other patches in this kit.

As before this patch fixes PR c/68473 by bulletproofing the new
diagnostic_show_locus implementation against ranges that finish before
they start (which can happen when using the C preprocessor), falling
back to simply printing a caret.

gcc/ChangeLog:
	PR c/68473
	* diagnostic-show-locus.c (layout::layout): Sanitize the
	layout against ranges that finish before they start.

gcc/testsuite/ChangeLog:
	PR c/68473
	* gcc.dg/plugin/diagnostic-test-expressions-1.c (fminl): New decl.
	(TEST_EQ): New macro.
	(test_macro): New function.
	* gcc.dg/pr68473-2.c: New test case.
	* gcc.dg/pr68473-3.c: New test case.
	* gcc.target/i386/pr68473-1.c: New test case.
---
 gcc/diagnostic-show-locus.c                        | 24 +++++++++++++++-
 .../gcc.dg/plugin/diagnostic-test-expressions-1.c  | 17 +++++++++++
 gcc/testsuite/gcc.dg/pr68473-2.c                   | 23 +++++++++++++++
 gcc/testsuite/gcc.dg/pr68473-3.c                   | 33 ++++++++++++++++++++++
 gcc/testsuite/gcc.target/i386/pr68473-1.c          | 24 ++++++++++++++++
 5 files changed, 120 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr68473-2.c
 create mode 100644 gcc/testsuite/gcc.dg/pr68473-3.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr68473-1.c

diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index f279019..0bcd333 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -473,9 +473,31 @@ layout::layout (diagnostic_context * context,
 	if (caret.file != m_exploc.file)
 	  continue;
 
+      /* Everything is now known to be in the correct source file,
+	 but it may require further sanitization.  */
+      layout_range ri (&start, &finish, nested_loc->m_show_caret_p, &caret);
+
+      /* If we have a range that finishes before it starts (perhaps
+	 from something built via macro expansion), printing the
+	 range is likely to be nonsensical.  Also, attempting to do so
+	 breaks assumptions within the printing code  (PR c/68473).  */
+      if (ri.m_start.m_line > ri.m_finish.m_line)
+	{
+	  /* Is this the primary location?  */
+	  if (m_layout_ranges.length () == 0)
+	    {
+	      /* We want to print the caret for the primary location, but
+		 we must sanitize away m_start and m_finish.  */
+	      ri.m_start = ri.m_caret;
+	      ri.m_finish = ri.m_caret;
+	    }
+	  else
+	    /* This is a non-primary location; ignore it.  */
+	    continue;
+	}
+
       /* Passed all the tests; add the location to m_layout_ranges so that
 	 it will be printed.  */
-      layout_range ri (&start, &finish, nested_loc->m_show_caret_p, &caret);
       m_layout_ranges.safe_push (ri);
 
       /* Update m_first_line/m_last_line if necessary.  */
diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-expressions-1.c b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-expressions-1.c
index 175b2ea..97426f6 100644
--- a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-expressions-1.c
+++ b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-expressions-1.c
@@ -618,3 +618,20 @@ void test_quadratic (double a, double b, double c)
    { dg-end-multiline-output "" } */
 
 }
+
+/* Reproducer for PR c/68473.  */
+
+extern long double fminl (long double __x, long double __y);
+#define TEST_EQ(FUNC) FUNC##l(xl,xl)
+void test_macro (long double xl)
+{
+  __emit_expression_range (0, TEST_EQ (fmin) ); /* { dg-warning "range" } */
+/* { dg-begin-multiline-output "" }
+   __emit_expression_range (0, TEST_EQ (fmin) );
+                                        ^
+   { dg-end-multiline-output "" } */
+/* { dg-begin-multiline-output "" }
+ #define TEST_EQ(FUNC) FUNC##l(xl,xl)
+                       ^~~~
+   { dg-end-multiline-output "" } */
+}
diff --git a/gcc/testsuite/gcc.dg/pr68473-2.c b/gcc/testsuite/gcc.dg/pr68473-2.c
new file mode 100644
index 0000000..5a10f1c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr68473-2.c
@@ -0,0 +1,23 @@
+/* { dg-options "-fdiagnostics-show-caret -Wall" } */
+#define FOO(bar) bar && 42  /* { dg-warning "suggest parentheses" } */
+int param;
+int lhs;
+int f() { return lhs || FOO(param); }  /* { dg-message "in expansion of" } */
+
+/* { dg-begin-multiline-output "" }
+ #define FOO(bar) bar && 42
+                      ^
+   { dg-end-multiline-output "" } */
+
+/* { dg-begin-multiline-output "" }
+ int f() { return lhs || FOO(param); }
+                         ^~~
+   { dg-end-multiline-output "" } */
+
+/* TODO: ideally we'd print the full range of the expression as it
+   pertains to the primary caret:
+
+ #define FOO(bar) bar && 42
+                  ~~~~^~~~~
+
+*/
diff --git a/gcc/testsuite/gcc.dg/pr68473-3.c b/gcc/testsuite/gcc.dg/pr68473-3.c
new file mode 100644
index 0000000..d1f3107
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr68473-3.c
@@ -0,0 +1,33 @@
+/* From PR c/68839 (dup of 68473). */
+/* { dg-options "-fdiagnostics-show-caret -Wduplicated-cond" } */
+
+struct S { void *q; };
+
+#define NULL (void*)0
+
+void f (struct S *p)
+{
+  if (p->q != NULL) { } /* { dg-message "previously used here" } */
+    else if (p->q != NULL) { } /* { dg-warning "duplicated" } */
+}
+
+/* { dg-begin-multiline-output "" }
+     else if (p->q != NULL) { }
+                   ^
+   { dg-end-multiline-output "" } */
+
+/* { dg-begin-multiline-output "" }
+   if (p->q != NULL) { }
+            ^
+   { dg-end-multiline-output "" } */
+
+/* TODO: Ideally we'd print some form of the ranges as they pertain to the
+   primary caret within the location:
+
+     else if (p->q != NULL) { }
+              ~~~~~^~~~~~~
+
+     if (p->q != NULL) { }
+         ~~~~~^~~~~~~
+
+*/
diff --git a/gcc/testsuite/gcc.target/i386/pr68473-1.c b/gcc/testsuite/gcc.target/i386/pr68473-1.c
new file mode 100644
index 0000000..ffffaa7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr68473-1.c
@@ -0,0 +1,24 @@
+/* { dg-options "-fdiagnostics-show-caret -mno-fp-ret-in-387" } */
+
+extern long double fminl (long double __x, long double __y);
+
+#define TEST_EQ(FUNC) do { \
+  if ((long)FUNC##l(xl,xl) != (long)xl) \
+    return; \
+  } while (0)
+
+void
+foo (long double xl)
+{
+  TEST_EQ (fmin); /* { dg-error "x87 register return with x87 disabled" } */
+}
+
+/* { dg-begin-multiline-output "" }
+   TEST_EQ (fmin);
+            ^
+   { dg-end-multiline-output "" } */
+
+/* { dg-begin-multiline-output "" }
+   if ((long)FUNC##l(xl,xl) != (long)xl) \
+             ^~~~
+   { dg-end-multiline-output "" } */
-- 
1.8.5.3

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 0/3] Re: [PATCH] PR c/68473: sanitize source range-printing within certain macro expansions
  2015-11-24 12:44     ` Bernd Schmidt
@ 2015-12-11 18:25       ` David Malcolm
  2015-12-11 18:25         ` [PATCH 2/3] Fix range/location terminology David Malcolm
                           ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: David Malcolm @ 2015-12-11 18:25 UTC (permalink / raw)
  To: Bernd Schmidt; +Cc: gcc-patches, David Malcolm

On Tue, 2015-11-24 at 13:43 +0100, Bernd Schmidt wrote:
> On 11/23/2015 07:26 PM, David Malcolm wrote:
> >
> > In theory we could attempt to try to handle this kind of thing by
> > looking at the macro expansions, and to print something like:
> >
> >      13	  TEST_EQ (fmin);
> >                     ^~~~
> >       6	  if ((long)FUNC##l(xl,xl) != (long)xl) \
> >                            ~~~~~~~~
> >
> > or whatnot, but that strikes me as error-prone at this stage.
> 
> Could I ask you to spend some time looking at what would be involved at 
> fixing it this way? In the end (assuming it doesn't prove to be a simple 
> fix) we will probably go with your original patch for gcc-6, but it 
> really goes against the grain to paper over a bug like this.

I've been looking into this.

The first issue we run into is that, currently, when we pass location_t
values into rich_location, it expands them immediately and stores the
result, which is thus throwing away any information on virtual macro
locations.

The first patch in the attached kit fixes that, storing location_t
values (aka source_location) within rich_location, delaying their
expansion until inside diagnostic_show_locus.  Doing so also
simplifies the code.

In writing the above patch I noticed now muddled the terminology has
become (range vs location), so the second patch in the kit rewords
things; I believe it makes the code much clearer.

The third patch in the kit is the earlier workaround for the bug; as
before it degrades diagnostic-printing to just print a caret for the
awkward cases, rather than attempting to print a range.  This should
mean that we fallback to the gcc 5 behavior for these cases.  I've
also added some new test cases based on duplicates filed against the
bug; they show a variety of interesting patterns that would have to
be handled if we were to use the 1st patch to fix things "properly".
I've added some TODO comments to the testcases to describe my
thoughts on how a proper fix might print them.

I've successfully bootstrapped&regrtested the combination of the patch
kit on x86_64-pc-linux-gnu; adds 17 PASS results to gcc.sum.

OK for trunk for gcc 6? (as 3 separate commits, assuming they
individually bootstrap&regrtest)

Thanks
Dave

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/3] Re: [PATCH] PR c/68473: sanitize source range-printing within certain macro expansions
  2015-12-11 18:25       ` [PATCH 0/3] " David Malcolm
                           ` (2 preceding siblings ...)
  2015-12-11 18:25         ` [PATCH 1/3] Delay location expansion within rich_location until printing David Malcolm
@ 2015-12-14 12:39         ` Bernd Schmidt
  3 siblings, 0 replies; 10+ messages in thread
From: Bernd Schmidt @ 2015-12-14 12:39 UTC (permalink / raw)
  To: David Malcolm; +Cc: gcc-patches

On 12/11/2015 07:45 PM, David Malcolm wrote:
> The third patch in the kit is the earlier workaround for the bug; as
> before it degrades diagnostic-printing to just print a caret for the
> awkward cases, rather than attempting to print a range.

I'm a little confused now, do the first two patches actually help or are 
they just cleanups? If they have no immediate effect, let's postpone 
them to stage 1 and just do your initial workaround for now.


Bernd

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-12-14 12:39 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-23 17:59 [PATCH] PR c/68473: sanitize source range-printing within certain macro expansions David Malcolm
2015-11-23 18:13 ` Bernd Schmidt
2015-11-23 18:37   ` David Malcolm
2015-11-24 12:44     ` Bernd Schmidt
2015-12-11 18:25       ` [PATCH 0/3] " David Malcolm
2015-12-11 18:25         ` [PATCH 2/3] Fix range/location terminology David Malcolm
2015-12-11 18:25         ` [PATCH 3/3] PR c/68473: sanitize source range-printing within certain macro expansions (v2) David Malcolm
2015-12-11 18:25         ` [PATCH 1/3] Delay location expansion within rich_location until printing David Malcolm
2015-12-14 12:39         ` [PATCH 0/3] Re: [PATCH] PR c/68473: sanitize source range-printing within certain macro expansions Bernd Schmidt
2015-12-10 16:24 ` Martin Sebor

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).