diff mbox

PR other/69006: fix extra newlines after diagnostics

Message ID 1452646670-40297-1-git-send-email-dmalcolm@redhat.com
State New
Headers show

Commit Message

David Malcolm Jan. 13, 2016, 12:57 a.m. UTC 
Before r229884 (aka f047900000107a32653aa7adbd24967b908c8a08),
diagnostic_show_locus did not emit a newline for the line
containing the caret, whereas the new implementation in r229884
does emit a newline for the final line it emits.

PR other/69006 notes that this leads to undesired extra newlines in
diagnostic output.

There are two ways we could fix the extra newline:
(A) preserve the old behavior of diagnostic_show_locus, and not emit
    a newline for the final line
(B) update everything that uses diagnostic_show_locus to expect it to
    have emitted a final newline

diagnostic_show_locus can emit multiple lines, for source, underlines
and fixits, so approach (A) seems awkward, so this patch takes
approach (B).

There are five places in trunk that can call diagnostic_show_locus.

The patch updates 3 of them to remove a newline immediately after a call
to diagnostic_show_locus:
  default_diagnostic_finalizer
  c_diagnostic_finalizer
  gfc_diagnostic_starter

Another caller of diagnostic_show_locus:
  diagnostic_append_note
is called zero or more times by:
  maybe_unwind_expanded_macro_loc
which is called by:
  virt_loc_aware_diagnostic_finalizer
The patch updates it to remove a call to pp_newline, which used to
occur before printing the note.

The final caller of diagnostic_show_locus:
  diagnostic_append_note_at_rich_loc
is unused, so the patch deletes it.

The patch required removing this assertion from
pp_output_formatted_text:
    gcc_assert (buffer->line_length == 0);
It's possible for this to fail to hold in the Fortran frontend when
printing e.g.:
  ../../src/gcc/testsuite/gfortran.dg/PR19754_1.f90:7:7-12:

      x = x + y ! { dg-error "Shapes for operands at" }
         1    2
  Shapes for operands at (1) and (2) are not conformable
with colorization - after printing the source code, the colorizer
prints color codes to disable colorization *after* the newline, and
hence there are 6 bytes of disable-colorization codes in the buffer
before the formatted text
  Shapes for operands at (1) and (2) are not conformable
is sent to the buffer.  I believe this isn't a problem, hence
the removal of the assertion.

Verifying the absence of newlines seemed tricky to do from DejaGnu, so
I added test coverage for this via a plugin.

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

OK for trunk in stage 3?

gcc/c-family/ChangeLog:
	PR other/69006
	* c-opts.c (c_diagnostic_finalizer): Replace invocation of
	pp_newline_and_flush with pp_flush.

gcc/ChangeLog:
	PR other/69006
	* diagnostic.c (default_diagnostic_finalizer): Replace invocation
	of pp_newline_and_flush with pp_flush.
	(diagnostic_append_note): Delete use of pp_newline.
	(diagnostic_append_note_at_rich_loc): Delete.
	* diagnostic.h (diagnostic_append_note_at_rich_loc): Delete.
	* diagnostic-show-locus.c (layout::print_any_fixits): After
	printing any fixits, print a trailing newline, if necessary.
	(diagnostic_show_locus): Move the pp_newline to before the
	early bailout.
	* pretty-print.c (pp_output_formatted_text): Remove assertion
	that buffer->line_length == 0.

gcc/fortran/ChangeLog:
	PR other/69006
	* error.c (gfc_diagnostic_starter): Delete use of pp_newline.

gcc/testsuite/ChangeLog:
	PR other/69006
	* gcc.dg/plugin/plugin.exp (plugin_test_list): Add
	pr69006_plugin.c, with pr69006-test-1.c and pr69006-test-2.c.
	* gcc.dg/plugin/pr69006-test-1.c: New test case.
	* gcc.dg/plugin/pr69006-test-2.c: New test case.
	* gcc.dg/plugin/pr69006_plugin.c: New test plugin.
---
 gcc/c-family/c-opts.c                        |  2 +-
 gcc/diagnostic-show-locus.c                  |  7 ++-
 gcc/diagnostic.c                             | 33 +-------------
 gcc/diagnostic.h                             |  4 --
 gcc/fortran/error.c                          |  1 -
 gcc/pretty-print.c                           |  1 -
 gcc/testsuite/gcc.dg/plugin/plugin.exp       |  3 ++
 gcc/testsuite/gcc.dg/plugin/pr69006-test-1.c | 12 ++++++
 gcc/testsuite/gcc.dg/plugin/pr69006-test-2.c | 11 +++++
 gcc/testsuite/gcc.dg/plugin/pr69006_plugin.c | 64 ++++++++++++++++++++++++++++
 10 files changed, 97 insertions(+), 41 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/plugin/pr69006-test-1.c
 create mode 100644 gcc/testsuite/gcc.dg/plugin/pr69006-test-2.c
 create mode 100644 gcc/testsuite/gcc.dg/plugin/pr69006_plugin.c

Comments

Bernd Schmidt Jan. 13, 2016, 5:32 p.m. UTC | #1 
On 01/13/2016 01:57 AM, David Malcolm wrote:
> There are five places in trunk that can call diagnostic_show_locus.

I'd kind of like to see before/after example output for all of these, to 
make sure that we are indeed removing only unnecessary newlines.

> The final caller of diagnostic_show_locus:
>    diagnostic_append_note_at_rich_loc
> is unused, so the patch deletes it.

That is ok as obvious.

> The patch required removing this assertion from
> pp_output_formatted_text:
>      gcc_assert (buffer->line_length == 0);

> with colorization - after printing the source code, the colorizer
> prints color codes to disable colorization *after* the newline,

I don't suppose that can be switched around to keep the assertion?

> Verifying the absence of newlines seemed tricky to do from DejaGnu, so
> I added test coverage for this via a plugin.

Ugh. Can it be this hard? I thought expect was designed to check output 
exactly. This might not fit in the dg-* framework too well, but IMO it 
would be better to check real compiler output than muck about with 
plugins if at all possible.


Bernd
diff mbox

Patch

diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
index f2a3815..8cc28af 100644
--- a/gcc/c-family/c-opts.c
+++ b/gcc/c-family/c-opts.c
@@ -169,7 +169,7 @@  c_diagnostic_finalizer (diagnostic_context *context,
      finalizer -- for tokens resulting from macro expansion.  */
   virt_loc_aware_diagnostic_finalizer (context, diagnostic);
   pp_destroy_prefix (context->printer);
-  pp_newline_and_flush (context->printer);
+  pp_flush (context->printer);
 }
 
 /* Common default settings for diagnostics.  */
diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index 3ef0052..998f81c 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -685,6 +685,9 @@  layout::print_any_fixits (int row, const rich_location *richloc)
 	    }
 	}
     }
+
+  /* Add a trailing newline, if necessary.  */
+  move_to_column (&column, 0);
 }
 
 /* Return true if (ROW/COLUMN) is within a range of the layout.
@@ -799,6 +802,8 @@  void
 diagnostic_show_locus (diagnostic_context * context,
 		       const diagnostic_info *diagnostic)
 {
+  pp_newline (context->printer);
+
   if (!context->show_caret
       || diagnostic_location (diagnostic, 0) <= BUILTINS_LOCATION
       || diagnostic_location (diagnostic, 0) == context->last_location)
@@ -806,8 +811,6 @@  diagnostic_show_locus (diagnostic_context * context,
 
   context->last_location = diagnostic_location (diagnostic, 0);
 
-  pp_newline (context->printer);
-
   const char *saved_prefix = pp_get_prefix (context->printer);
   pp_set_prefix (context->printer, NULL);
 
diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index effb8f2..f661b57 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -546,7 +546,7 @@  default_diagnostic_finalizer (diagnostic_context *context,
 {
   diagnostic_show_locus (context, diagnostic);
   pp_destroy_prefix (context->printer);
-  pp_newline_and_flush (context->printer);
+  pp_flush (context->printer);
 }
 
 /* Interface to specify diagnostic kind overrides.  Returns the
@@ -879,37 +879,6 @@  diagnostic_append_note (diagnostic_context *context,
   saved_prefix = pp_get_prefix (context->printer);
   pp_set_prefix (context->printer,
                  diagnostic_build_prefix (context, &diagnostic));
-  pp_newline (context->printer);
-  pp_format (context->printer, &diagnostic.message);
-  pp_output_formatted_text (context->printer);
-  pp_destroy_prefix (context->printer);
-  pp_set_prefix (context->printer, saved_prefix);
-  diagnostic_show_locus (context, &diagnostic);
-  va_end (ap);
-}
-
-/* Same as diagnostic_append_note, but at RICHLOC. */
-
-void
-diagnostic_append_note_at_rich_loc (diagnostic_context *context,
-				    rich_location *richloc,
-				    const char * gmsgid, ...)
-{
-  diagnostic_info diagnostic;
-  va_list ap;
-  const char *saved_prefix;
-
-  va_start (ap, gmsgid);
-  diagnostic_set_info (&diagnostic, gmsgid, &ap, richloc, DK_NOTE);
-  if (context->inhibit_notes_p)
-    {
-      va_end (ap);
-      return;
-    }
-  saved_prefix = pp_get_prefix (context->printer);
-  pp_set_prefix (context->printer,
-                 diagnostic_build_prefix (context, &diagnostic));
-  pp_newline (context->printer);
   pp_format (context->printer, &diagnostic.message);
   pp_output_formatted_text (context->printer);
   pp_destroy_prefix (context->printer);
diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
index 2cb6270..7cc5cff 100644
--- a/gcc/diagnostic.h
+++ b/gcc/diagnostic.h
@@ -293,10 +293,6 @@  extern void diagnostic_set_info_translated (diagnostic_info *, const char *,
      ATTRIBUTE_GCC_DIAG(2,0);
 extern void diagnostic_append_note (diagnostic_context *, location_t,
                                     const char *, ...) ATTRIBUTE_GCC_DIAG(3,4);
-extern void diagnostic_append_note_at_rich_loc (diagnostic_context *,
-						rich_location *,
-						const char *, ...)
-  ATTRIBUTE_GCC_DIAG(3,4);
 #endif
 extern char *diagnostic_build_prefix (diagnostic_context *, const diagnostic_info *);
 void default_diagnostic_starter (diagnostic_context *, diagnostic_info *);
diff --git a/gcc/fortran/error.c b/gcc/fortran/error.c
index 457a3b0..2243ead 100644
--- a/gcc/fortran/error.c
+++ b/gcc/fortran/error.c
@@ -1096,7 +1096,6 @@  gfc_diagnostic_starter (diagnostic_context *context,
       /* Fortran uses an empty line between locus and caret line.  */
       pp_newline (context->printer);
       diagnostic_show_locus (context, diagnostic);
-      pp_newline (context->printer);
       /* If the caret line was shown, the prefix does not contain the
 	 locus.  */
       pp_set_prefix (context->printer, kind_prefix);
diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c
index acb89e6..5de6878 100644
--- a/gcc/pretty-print.c
+++ b/gcc/pretty-print.c
@@ -667,7 +667,6 @@  pp_output_formatted_text (pretty_printer *pp)
   const char **args = chunk_array->args;
 
   gcc_assert (buffer->obstack == &buffer->formatted_obstack);
-  gcc_assert (buffer->line_length == 0);
 
   /* This is a third phase, first 2 phases done in pp_format_args.
      Now we actually print it.  */
diff --git a/gcc/testsuite/gcc.dg/plugin/plugin.exp b/gcc/testsuite/gcc.dg/plugin/plugin.exp
index 0dd310e..b0749dd 100644
--- a/gcc/testsuite/gcc.dg/plugin/plugin.exp
+++ b/gcc/testsuite/gcc.dg/plugin/plugin.exp
@@ -71,6 +71,9 @@  set plugin_test_list [list \
     { diagnostic_plugin_show_trees.c \
 	  diagnostic-test-show-trees-1.c } \
     { levenshtein_plugin.c levenshtein-test-1.c } \
+    { pr69006_plugin.c \
+	  pr69006-test-1.c \
+	  pr69006-test-2.c } \
 ]
 
 foreach plugin_test $plugin_test_list {
diff --git a/gcc/testsuite/gcc.dg/plugin/pr69006-test-1.c b/gcc/testsuite/gcc.dg/plugin/pr69006-test-1.c
new file mode 100644
index 0000000..144655c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/plugin/pr69006-test-1.c
@@ -0,0 +1,12 @@ 
+/* Placeholder C source file for testing PR other/69006.
+   This testcase (implicitly) tests the newlines seen with
+   -fno-diagnostics-show-caret.  */
+
+/* { dg-do compile } */
+/* { dg-options "-O" } */
+
+int
+main (int argc, char **argv)
+{
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/plugin/pr69006-test-2.c b/gcc/testsuite/gcc.dg/plugin/pr69006-test-2.c
new file mode 100644
index 0000000..f1ead93
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/plugin/pr69006-test-2.c
@@ -0,0 +1,11 @@ 
+/* Placeholder C source file for testing PR other/69006.
+   This testcase tests the newlines seen with -fdiagnostics-show-caret.  */
+
+/* { dg-do compile } */
+/* { dg-options "-O -fdiagnostics-show-caret" } */
+
+int
+main (int argc, char **argv)
+{
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/plugin/pr69006_plugin.c b/gcc/testsuite/gcc.dg/plugin/pr69006_plugin.c
new file mode 100644
index 0000000..ca6c7f9
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/plugin/pr69006_plugin.c
@@ -0,0 +1,64 @@ 
+/* Plugin for verifying absence of stray newlines in diagnostic output
+   (PR other/69006).  */
+
+#include "config.h"
+#include "gcc-plugin.h"
+#include "system.h"
+#include "coretypes.h"
+#include "diagnostic.h"
+
+int plugin_is_GPL_compatible;
+
+static int count_newlines (const char *text)
+{
+  gcc_assert (text);
+  int num_newlines = 0;
+  while (*text)
+    if (*(text++) == '\n')
+      num_newlines++;
+  return num_newlines;
+}
+
+/* Callback handler for the PLUGIN_FINISH_UNIT event.
+   Turn off flushing of diagnostics, and inject one,
+   and verify the newlines within the resulting unflushed
+   buffer.  */
+
+static void
+on_finish_unit (void */*gcc_data*/, void */*user_data*/)
+{
+  output_buffer *buff = pp_buffer (global_dc->printer);
+
+  buff->flush_p = false;
+  warning_at (input_location, 0, "this is a warning");
+
+  /* buff should now contain a NIL-terminated printing of the above
+     diagnostic.  */
+  const char *text = (const char *) obstack_base (buff->obstack);
+  gcc_assert (strstr (text, "this is a warning"));
+
+  /* Verify that we have the correct number of newline characters.
+     If we're printing source code, we expect 3 newlines:
+        "path-of-source.c:9:1: warning: this is a warning\n"
+	" }\n"
+	" ^\n"
+     Otherwise, we expect just 1 newline (for the first of the lines above).  */
+  int expected_newlines = global_dc->show_caret ? 3 : 1;
+  int num_newlines = count_newlines (text);
+  gcc_assert (num_newlines == expected_newlines);
+
+  /* Verify that we don't have a blank line.  */
+  gcc_assert (!strstr (text, "\n\n"));
+}
+
+int
+plugin_init (struct plugin_name_args *plugin_info,
+	     struct plugin_gcc_version */*version*/)
+{
+  register_callback (plugin_info->base_name,
+		     PLUGIN_FINISH_UNIT,
+		     on_finish_unit,
+		     NULL); /* void *user_data */
+
+  return 0;
+}