Re: [PATCH] PR preprocessor/60723 - missing system-ness marks for macro

[Dodji Seketeli <dodji at redhat dot com>]

Jason Merrill <jason@redhat.com> writes:

> On 06/27/2014 03:27 AM, Dodji Seketeli wrote:
>> +	      && print.prev_was_system_token != !!in_system_header_at(loc))
>> +	    /* The system-ness of this token is different from the one
>> +	       of the previous token.  Let's emit a line change to
>> +	       mark the new system-ness before we emit the token.  */
>> +	    line_marker_emitted = do_line_change(pfile, token, loc, false);
>
> Missing spaces before '('.  OK with that fixed.

Thanks.

It appeared that the patch was too eager to emit line changes, even for
cases (like when preprocessing asm files) where a new line between
tokens can be significant and turn a valid statement into an invalid
one.

I have updated the patch to prevent that and tested it again on
x86_64-unknown-linux-gnu.  Christophe Lyon (who reported this latest
issue) tested it on his ARM-based system that exhibited the issue.

The relevant hunk that changes is this one:

@@ -248,9 +252,20 @@ scan_translation_unit (cpp_reader *pfile)
 	  if (cpp_get_options (parse_in)->debug)
 	      linemap_dump_location (line_table, token->src_loc,
 				     print.outf);
+
+	  if (do_line_adjustments
+	      && !in_pragma
+	      && !line_marker_emitted
+	      && print.prev_was_system_token != !!in_system_header_at(loc))
+	    /* The system-ness of this token is different from the one
+	       of the previous token.  Let's emit a line change to
+	       mark the new system-ness before we emit the token.  */
+	    line_marker_emitted = do_line_change (pfile, token, loc, false);
 	  cpp_output_token (token, print.outf);
+	  line_marker_emitted = false;
 	}
 
+      print.prev_was_system_token = !!in_system_header_at(loc);
       /* CPP_COMMENT tokens and raw-string literal tokens can
 	 have embedded new-line characters.  Rather than enumerating
 	 all the possible token types just check if token uses

In there, the change is that I am now testing that line adjustments are
allowed and that we are not inside pragmas with the:

+	  if (do_line_adjustments
+	      && !in_pragma

This make the change coherent with what is done elsewhere in
scan_translation_unit.

OK to commit this latest version to trunk?

gcc/c-family/ChangeLog:
	* c-ppoutput.c (struct print::prev_was_system_token): New data
	member.
	(init_pp_output): Initialize it.
	(maybe_print_line_1, maybe_print_line, print_line_1, print_line)
	(do_line_change): Return a flag saying if a line marker was
	emitted or not.
	(scan_translation_unit): Detect if the system-ness of the token we
	are about to emit is different from the one of the previously
	emitted token.  If so, emit a line marker.  Avoid emitting
	useless adjacent line markers.
	(scan_translation_unit_directives_only): Adjust.

gcc/testsuite/ChangeLog:
	* gcc.dg/cpp/syshdr{4,5}.{c,h}: New test files.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@212194 138bc75d-0d04-0410-961f-82ee72b054a4
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 gcc/c-family/ChangeLog             | 15 ++++++++
 gcc/c-family/c-ppoutput.c          | 78 ++++++++++++++++++++++++++------------
 gcc/testsuite/ChangeLog            |  5 +++
 gcc/testsuite/gcc.dg/cpp/syshdr4.c | 24 ++++++++++++
 gcc/testsuite/gcc.dg/cpp/syshdr4.h |  8 ++++
 gcc/testsuite/gcc.dg/cpp/syshdr5.c | 14 +++++++
 gcc/testsuite/gcc.dg/cpp/syshdr5.h |  6 +++
 7 files changed, 126 insertions(+), 24 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/cpp/syshdr4.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/syshdr4.h
 create mode 100644 gcc/testsuite/gcc.dg/cpp/syshdr5.c
 create mode 100644 gcc/testsuite/gcc.dg/cpp/syshdr5.h

diff --git a/gcc/c-family/c-ppoutput.c b/gcc/c-family/c-ppoutput.c
index f3b5fa4..400d3a7 100644
--- a/gcc/c-family/c-ppoutput.c
+++ b/gcc/c-family/c-ppoutput.c
@@ -36,6 +36,8 @@ static struct
   unsigned char printed;	/* Nonzero if something output at line.  */
   bool first_time;		/* pp_file_change hasn't been called yet.  */
   const char *src_file;		/* Current source file.  */
+  bool prev_was_system_token;	/* True if the previous token was a
+				   system token.*/
 } print;
 
 /* Defined and undefined macros being queued for output with -dU at
@@ -58,11 +60,11 @@ static void account_for_newlines (const unsigned char *, size_t);
 static int dump_macro (cpp_reader *, cpp_hashnode *, void *);
 static void dump_queued_macros (cpp_reader *);
 
-static void print_line_1 (source_location, const char*, FILE *);
-static void print_line (source_location, const char *);
-static void maybe_print_line_1 (source_location, FILE *);
-static void maybe_print_line (source_location);
-static void do_line_change (cpp_reader *, const cpp_token *,
+static bool print_line_1 (source_location, const char*, FILE *);
+static bool print_line (source_location, const char *);
+static bool maybe_print_line_1 (source_location, FILE *);
+static bool maybe_print_line (source_location);
+static bool do_line_change (cpp_reader *, const cpp_token *,
 			    source_location, int);
 
 /* Callback routines for the parser.   Most of these are active only
@@ -156,6 +158,7 @@ init_pp_output (FILE *out_stream)
   print.outf = out_stream;
   print.first_time = 1;
   print.src_file = "";
+  print.prev_was_system_token = false;
 }
 
 /* Writes out the preprocessed file, handling spacing and paste
@@ -168,6 +171,7 @@ scan_translation_unit (cpp_reader *pfile)
     = cpp_get_options (parse_in)->lang != CLK_ASM
       && !flag_no_line_commands;
   bool in_pragma = false;
+  bool line_marker_emitted = false;
 
   print.source = NULL;
   for (;;)
@@ -200,7 +204,7 @@ scan_translation_unit (cpp_reader *pfile)
 	      && do_line_adjustments
 	      && !in_pragma)
 	    {
-	      do_line_change (pfile, token, loc, false);
+	      line_marker_emitted = do_line_change (pfile, token, loc, false);
 	      putc (' ', print.outf);
 	    }
 	  else if (print.source->flags & PREV_WHITE
@@ -216,7 +220,7 @@ scan_translation_unit (cpp_reader *pfile)
 	  if (src_line != print.src_line
 	      && do_line_adjustments
 	      && !in_pragma)
-	    do_line_change (pfile, token, loc, false);
+	    line_marker_emitted = do_line_change (pfile, token, loc, false);
 	  putc (' ', print.outf);
 	}
 
@@ -228,7 +232,7 @@ scan_translation_unit (cpp_reader *pfile)
 	  const char *space;
 	  const char *name;
 
-	  maybe_print_line (token->src_loc);
+	  line_marker_emitted = maybe_print_line (token->src_loc);
 	  fputs ("#pragma ", print.outf);
 	  c_pp_lookup_pragma (token->val.pragma, &space, &name);
 	  if (space)
@@ -248,9 +252,20 @@ scan_translation_unit (cpp_reader *pfile)
 	  if (cpp_get_options (parse_in)->debug)
 	      linemap_dump_location (line_table, token->src_loc,
 				     print.outf);
+
+	  if (do_line_adjustments
+	      && !in_pragma
+	      && !line_marker_emitted
+	      && print.prev_was_system_token != !!in_system_header_at(loc))
+	    /* The system-ness of this token is different from the one
+	       of the previous token.  Let's emit a line change to
+	       mark the new system-ness before we emit the token.  */
+	    line_marker_emitted = do_line_change (pfile, token, loc, false);
 	  cpp_output_token (token, print.outf);
+	  line_marker_emitted = false;
 	}
 
+      print.prev_was_system_token = !!in_system_header_at(loc);
       /* CPP_COMMENT tokens and raw-string literal tokens can
 	 have embedded new-line characters.  Rather than enumerating
 	 all the possible token types just check if token uses
@@ -275,7 +290,7 @@ scan_translation_unit_directives_only (cpp_reader *pfile)
   struct _cpp_dir_only_callbacks cb;
 
   cb.print_lines = print_lines_directives_only;
-  cb.maybe_print_line = maybe_print_line;
+  cb.maybe_print_line = (void (*) (source_location)) maybe_print_line;
 
   _cpp_preprocess_dir_only (pfile, &cb);
 }
@@ -306,11 +321,13 @@ scan_translation_unit_trad (cpp_reader *pfile)
 
 /* If the token read on logical line LINE needs to be output on a
    different line to the current one, output the required newlines or
-   a line marker, and return 1.  Otherwise return 0.  */
+   a line marker.  If a line marker was emitted, return TRUE otherwise
+   return FALSE.  */
 
-static void
+static bool
 maybe_print_line_1 (source_location src_loc, FILE *stream)
 {
+  bool emitted_line_marker = false;
   int src_line = LOCATION_LINE (src_loc);
   const char *src_file = LOCATION_FILE (src_loc);
 
@@ -334,29 +351,34 @@ maybe_print_line_1 (source_location src_loc, FILE *stream)
 	}
     }
   else
-    print_line_1 (src_loc, "", stream);
+    emitted_line_marker = print_line_1 (src_loc, "", stream);
 
+  return emitted_line_marker;
 }
 
 /* If the token read on logical line LINE needs to be output on a
    different line to the current one, output the required newlines or
-   a line marker, and return 1.  Otherwise return 0.  */
+   a line marker.  If a line marker was emitted, return TRUE otherwise
+   return FALSE.  */
 
-static void
+static bool
 maybe_print_line (source_location src_loc)
 {
   if (cpp_get_options (parse_in)->debug)
     linemap_dump_location (line_table, src_loc,
 			   print.outf);
-  maybe_print_line_1 (src_loc, print.outf);
+  return maybe_print_line_1 (src_loc, print.outf);
 }
 
 /* Output a line marker for logical line LINE.  Special flags are "1"
-   or "2" indicating entering or leaving a file.  */
+   or "2" indicating entering or leaving a file.  If the line marker
+   was effectively emitted, return TRUE otherwise return FALSE.  */
 
-static void
+static bool
 print_line_1 (source_location src_loc, const char *special_flags, FILE *stream)
 {
+  bool emitted_line_marker = false;
+
   /* End any previous line of text.  */
   if (print.printed)
     putc ('\n', stream);
@@ -391,33 +413,39 @@ print_line_1 (source_location src_loc, const char *special_flags, FILE *stream)
 	fputs (" 3", stream);
 
       putc ('\n', stream);
+      emitted_line_marker = true;
     }
+
+  return emitted_line_marker;
 }
 
 /* Output a line marker for logical line LINE.  Special flags are "1"
-   or "2" indicating entering or leaving a file.  */
+   or "2" indicating entering or leaving a file.  Return TRUE if a
+   line marker was effectively emitted, FALSE otherwise.  */
 
-static void
+static bool
 print_line (source_location src_loc, const char *special_flags)
 {
     if (cpp_get_options (parse_in)->debug)
       linemap_dump_location (line_table, src_loc,
 			     print.outf);
-    print_line_1 (src_loc, special_flags, print.outf);
+    return print_line_1 (src_loc, special_flags, print.outf);
 }
 
-/* Helper function for cb_line_change and scan_translation_unit.  */
-static void
+/* Helper function for cb_line_change and scan_translation_unit.
+   Return TRUE if a line marker is emitted, FALSE otherwise.  */
+static bool
 do_line_change (cpp_reader *pfile, const cpp_token *token,
 		source_location src_loc, int parsing_args)
 {
+  bool emitted_line_marker = false;
   if (define_queue || undef_queue)
     dump_queued_macros (pfile);
 
   if (token->type == CPP_EOF || parsing_args)
-    return;
+    return false;
 
-  maybe_print_line (src_loc);
+  emitted_line_marker = maybe_print_line (src_loc);
   print.prev = 0;
   print.source = 0;
 
@@ -434,6 +462,8 @@ do_line_change (cpp_reader *pfile, const cpp_token *token,
       while (-- spaces >= 0)
 	putc (' ', print.outf);
     }
+
+  return emitted_line_marker;
 }
 
 /* Called when a line of output is started.  TOKEN is the first token
diff --git a/gcc/testsuite/gcc.dg/cpp/syshdr4.c b/gcc/testsuite/gcc.dg/cpp/syshdr4.c
new file mode 100644
index 0000000..fe001d2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/syshdr4.c
@@ -0,0 +1,24 @@
+/* Contributed by Nicholas Ormrod */
+/* Origin: PR preprocessor/60723 */
+
+/* This tests that multi-line macro callsites, which are defined
+   in system headers and whose expansion contains a builtin followed
+   by a non-builtin token, do not generate a line directive that
+   mark the current file as being a system file, when performing
+   non-integrated preprocessing. */
+/* System files suppress div-by-zero warnings, so the presence of
+   such indicates the lack of the bug.
+
+   { dg-do compile }
+   { dg-options -no-integrated-cpp }  */
+
+#include "syshdr4.h"
+FOO(
+)
+
+int
+foo()
+{
+  return 1 / 0; /* { dg-warning "div-by-zero" } */
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/cpp/syshdr4.h b/gcc/testsuite/gcc.dg/cpp/syshdr4.h
new file mode 100644
index 0000000..c464f6e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/syshdr4.h
@@ -0,0 +1,8 @@
+/* Contributed by Nicholas Ormrod
+   Origin: PR preprocessor/60723.
+
+   This file is to be included by the syshdr4.c file.  */
+
+#pragma GCC system_header
+
+#define FOO() int line = __LINE__ ;
diff --git a/gcc/testsuite/gcc.dg/cpp/syshdr5.c b/gcc/testsuite/gcc.dg/cpp/syshdr5.c
new file mode 100644
index 0000000..42c6263
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/syshdr5.c
@@ -0,0 +1,14 @@
+/* Origin: PR preprocessor/60723
+
+   { dg-do compile }
+   { dg-options -no-integrated-cpp }  */
+
+#include "syshdr5.h"
+
+int
+main()
+{
+  FOO(1/0 /*  { dg-warning "division by zero" }  */
+      );
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/cpp/syshdr5.h b/gcc/testsuite/gcc.dg/cpp/syshdr5.h
new file mode 100644
index 0000000..300d6c3
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/syshdr5.h
@@ -0,0 +1,6 @@
+/* Origin: PR preprocessor/60723
+
+   This header file is to be included by the syshdr5.c file.  */
+
+#pragma GCC system_header
+#define FOO(A)do {int line = __LINE__ ; A;} while(0)
-- 
		Dodji