[PATCH] PR preprocessor/69664: fix rich_location::override_column

[PATCH] PR preprocessor/69664: fix rich_location::override_column

[David Malcolm]

In gcc 5 and earlier, struct diagnostic_info had a field:
  unsigned int override_column;
which could be set by the macro:
  diagnostic_override_column

This was only used by the frontends' callbacks for handling errors
from libcpp: c_cpp_error for the c-family, and cb_cpp_error for Fortran:
  if (column_override)
    diagnostic_override_column (&diagnostic, column_override);
based on a column_override value passed by libcpp to the callback.

I removed the field when introducing rich_location (in r229884),
instead adding a rich_location::override_column method, called from
libcpp immediately before calling the frontend's error-handling
callback (the callback takes a rich_location *).

I got the implementation of rich_location::override_column wrong
(sorry), and PR preprocessor/69664 is a symptom of this.

Specifically, I was only overriding the column within
m_expanded_location, which affects some parts of
diagnostic-show-locus.c, but I was not overriding the column within
the various expanded_location instances within the rich_location's
m_ranges[0].

Hence the wrong column information is printed for diagnostics
emitted by libcpp where the column is overridden.
This happens for any of the "_with_line" diagnostics calls in
libcpp that are passed a non-zero column override.
I believe these are all confined to libcpp/lex.c.

The attached patch fixes this, by ensuring that all relevant
columns are updated by rich_location::override_column, and
also adding the missing conditional to only call it for non-zero
column_override values (this time in libcpp before calling the
error-handling callback, rather than from within the error-handling
callbacks).

It also adds expected column numbers to some pre-existing tests,
giving us test coverage for this.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu;
OK for trunk?

gcc/testsuite/ChangeLog:
	PR preprocessor/69664
	* gcc.dg/cpp/trad/comment-2.c: Add expected column number.
	* gcc.dg/cpp/warn-comments.c: Likewise.

libcpp/ChangeLog:
	PR preprocessor/69664
	* errors.c (cpp_diagnostic_with_line): Only call
	rich_location::override_column if the column is non-zero.
	* line-map.c (rich_location::override_column): Update columns
	within m_ranges[0].  Add assertions to verify that doing so is
	sane.
---
 gcc/testsuite/gcc.dg/cpp/trad/comment-2.c |  2 +-
 gcc/testsuite/gcc.dg/cpp/warn-comments.c  |  4 ++--
 libcpp/errors.c                           |  3 ++-
 libcpp/line-map.c                         | 11 ++++++++++-
 4 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/gcc/testsuite/gcc.dg/cpp/trad/comment-2.c b/gcc/testsuite/gcc.dg/cpp/trad/comment-2.c
index 8d54e3a..310f569 100644
--- a/gcc/testsuite/gcc.dg/cpp/trad/comment-2.c
+++ b/gcc/testsuite/gcc.dg/cpp/trad/comment-2.c
@@ -8,4 +8,4 @@
 
 /*
 
- /* { dg-warning "within comment" } */
+ /* { dg-warning "2: within comment" } */
diff --git a/gcc/testsuite/gcc.dg/cpp/warn-comments.c b/gcc/testsuite/gcc.dg/cpp/warn-comments.c
index 1cdf75c..bbe2821 100644
--- a/gcc/testsuite/gcc.dg/cpp/warn-comments.c
+++ b/gcc/testsuite/gcc.dg/cpp/warn-comments.c
@@ -1,7 +1,7 @@
 // { dg-do preprocess }
 // { dg-options "-std=gnu99 -fdiagnostics-show-option -Wcomments" }
 
-/* /* */  // { dg-warning "\"\.\*\" within comment .-Wcomment." }
+/* /* */  // { dg-warning "4: \"\.\*\" within comment .-Wcomment." }
 
 // \
-          // { dg-warning "multi-line comment .-Wcomment." "multi-line" { target *-*-* } 6 }
+          // { dg-warning "1: multi-line comment .-Wcomment." "multi-line" { target *-*-* } 6 }
diff --git a/libcpp/errors.c b/libcpp/errors.c
index d92b386..9847378 100644
--- a/libcpp/errors.c
+++ b/libcpp/errors.c
@@ -141,7 +141,8 @@ cpp_diagnostic_with_line (cpp_reader * pfile, int level, int reason,
   if (!pfile->cb.error)
     abort ();
   rich_location richloc (pfile->line_table, src_loc);
-  richloc.override_column (column);
+  if (column)
+    richloc.override_column (column);
   ret = pfile->cb.error (pfile, level, reason, &richloc, _(msgid), ap);
 
   return ret;
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index fcf0259..e9175df 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -2036,13 +2036,22 @@ rich_location::lazily_expand_location ()
   return m_expanded_location;
 }
 
-/* Set the column of the primary location.  */
+/* Set the column of the primary location.  This can only be called for
+   rich_location instances for which the primary location has
+   caret==start==finish.  */
 
 void
 rich_location::override_column (int column)
 {
   lazily_expand_location ();
+  gcc_assert (m_ranges[0].m_show_caret_p);
+  gcc_assert (m_ranges[0].m_caret.column == m_expanded_location.column);
+  gcc_assert (m_ranges[0].m_start.column == m_expanded_location.column);
+  gcc_assert (m_ranges[0].m_finish.column == m_expanded_location.column);
   m_expanded_location.column = column;
+  m_ranges[0].m_caret.column = column;
+  m_ranges[0].m_start.column = column;
+  m_ranges[0].m_finish.column = column;
 }
 
 /* Add the given range.  */
-- 
1.8.5.3

Re: [PATCH] PR preprocessor/69664: fix rich_location::override_column

[Jeff Law]

On 02/04/2016 03:51 PM, David Malcolm wrote:
> In gcc 5 and earlier, struct diagnostic_info had a field:
>    unsigned int override_column;
> which could be set by the macro:
>    diagnostic_override_column
>
> This was only used by the frontends' callbacks for handling errors
> from libcpp: c_cpp_error for the c-family, and cb_cpp_error for Fortran:
>    if (column_override)
>      diagnostic_override_column (&diagnostic, column_override);
> based on a column_override value passed by libcpp to the callback.
>
> I removed the field when introducing rich_location (in r229884),
> instead adding a rich_location::override_column method, called from
> libcpp immediately before calling the frontend's error-handling
> callback (the callback takes a rich_location *).
>
> I got the implementation of rich_location::override_column wrong
> (sorry), and PR preprocessor/69664 is a symptom of this.
>
> Specifically, I was only overriding the column within
> m_expanded_location, which affects some parts of
> diagnostic-show-locus.c, but I was not overriding the column within
> the various expanded_location instances within the rich_location's
> m_ranges[0].
>
> Hence the wrong column information is printed for diagnostics
> emitted by libcpp where the column is overridden.
> This happens for any of the "_with_line" diagnostics calls in
> libcpp that are passed a non-zero column override.
> I believe these are all confined to libcpp/lex.c.
>
> The attached patch fixes this, by ensuring that all relevant
> columns are updated by rich_location::override_column, and
> also adding the missing conditional to only call it for non-zero
> column_override values (this time in libcpp before calling the
> error-handling callback, rather than from within the error-handling
> callbacks).
>
> It also adds expected column numbers to some pre-existing tests,
> giving us test coverage for this.
>
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu;
> OK for trunk?
>
> gcc/testsuite/ChangeLog:
> 	PR preprocessor/69664
> 	* gcc.dg/cpp/trad/comment-2.c: Add expected column number.
> 	* gcc.dg/cpp/warn-comments.c: Likewise.
>
> libcpp/ChangeLog:
> 	PR preprocessor/69664
> 	* errors.c (cpp_diagnostic_with_line): Only call
> 	rich_location::override_column if the column is non-zero.
> 	* line-map.c (rich_location::override_column): Update columns
> 	within m_ranges[0].  Add assertions to verify that doing so is
> 	sane.
OK

Thanks,
Jeff