Patchwork pragma diagnostic push/pop

login
register
mail settings
Submitter DJ Delorie
Date June 11, 2010, 1:42 a.m.
Message ID <201006110142.o5B1gqcR022118@greed.delorie.com>
Download mbox | patch
Permalink /patch/55281/
State New
Headers show

Comments

DJ Delorie - June 11, 2010, 1:42 a.m.
This is long overdue...  first try on location-sensitive pragma-based
diagnostic control.

	* diagnostic.h (diagnostic_classification_change_t): New.
	(diagnostic_context): Add history and push/pop list.
	(diagnostic_push_diagnostics): Declare.
	(diagnostic_pop_diagnostics): Declare.
	* diagnostic.c (diagnostic_classify_diagnostic): Store changes
	from pragmas in a history chain instead of the global table.
	(diagnostic_push_diagnostics): New.
	(diagnostic_pop_diagnostics): New.
	(diagnostic_report_diagnostic): Scan history chain to find state
	of diagnostics as of the diagnostic location.
	* diagnostic-core.h (DK_POP): Add after "real" diagnostics, for
	use in the history chain.
	* c-family/c-pragma.c (handle_pragma_diagnostic): Add push/pop,
	allow these pragmas anywhere.
	* doc/extend.texi: Document pragma GCC diagnostic changes.

	* gcc.dg/pragma-diag-1.c: New.
Tom Tromey - June 11, 2010, 3 a.m.
>>>>> "DJ" == DJ Delorie <dj@redhat.com> writes:

DJ> This is long overdue...  first try on location-sensitive pragma-based
DJ> diagnostic control.

Nice!

DJ> @@ -703,30 +703,34 @@ handle_pragma_diagnostic(cpp_reader *ARG
[...]
DJ>    token = pragma_lex (&x);

DJ> @@ -313,16 +313,72 @@ diagnostic_classify_diagnostic (diagnost
[...]
DJ> +  /* Handle pragmas separately, since we need to keep track of *where*
DJ> +     the pragmas were.  */
DJ> +  if (input_location > 0)

I would prefer it if new code avoided the global input_location and
instead took a source_location as a parameter.

This ought to be pretty easy since handle_pragma_diagnostic has a token
available.

Tom
DJ Delorie - June 11, 2010, 3:49 a.m.
> This ought to be pretty easy since handle_pragma_diagnostic has a token
> available.

It has a cpp_ttype, not a cpp_token.  No location information :-(

Where else would we get the location from?
Manuel López-Ibáñez - June 11, 2010, 9:32 a.m.
On 11 June 2010 05:49, DJ Delorie <dj@redhat.com> wrote:
>
>> This ought to be pretty easy since handle_pragma_diagnostic has a token
>> available.
>
> It has a cpp_ttype, not a cpp_token.  No location information :-(
>
> Where else would we get the location from?

I agree with Tom that this would be desirable but I think that it is
not possible except by modifying pragma_lex to return the location
somehow. Probably not worthwhile at this point, since the
input_location should be pretty accurate for the lexer, so the only
benefit is getting rid of a global.

Cheers,

Manuel.
Manuel López-Ibáñez - June 11, 2010, 9:34 a.m.
On 11 June 2010 03:42, DJ Delorie <dj@redhat.com> wrote:
> +         int i;
> +         /* Stupid search.  Optimize later. */

Could you add FIXME to this comment?

Once this go in, please add an entry (preferably with an example) to
gcc-4.6/changes.html. This is worth boasting about!

Manuel.
Gerald Pfeifer - June 11, 2010, 10:49 a.m.
On Fri, 11 Jun 2010, Manuel López-Ibáñez wrote:
> Once this go in, please add an entry (preferably with an example) to
> gcc-4.6/changes.html. This is worth boasting about!

Hehe, I see I can retire soon. :-)

I'd be tempted to make this a new item on our main page as well -- we are 
way, way too modest and silent there, and this is really user visible. 
(Perhaps, since it's not a huge patch, not refer to a company for this 
one, as opposed to some of the bigger ones.)

Gerald
Tom Tromey - June 11, 2010, 2:42 p.m.
>>>>> "Manuel" == Manuel López-Ibáñez <lopezibanez@gmail.com> writes:

DJ> It has a cpp_ttype, not a cpp_token.  No location information :-(
DJ> Where else would we get the location from?

Sorry about that.  I misread the code.

Manuel> I agree with Tom that this would be desirable but I think that it is
Manuel> not possible except by modifying pragma_lex to return the location
Manuel> somehow. Probably not worthwhile at this point, since the
Manuel> input_location should be pretty accurate for the lexer, so the only
Manuel> benefit is getting rid of a global.

Changing pragma_lex to also return a token does not seem like a very big
change, and helping to remove a global is a definite plus.

input_location is set in a complicated way in the lexer and parser.
It seems to happen on line- and file-changes, and also at some other
random times (see c_parser_set_source_position_from_token).  I think it
is actually more reliable to extract the location from the token.

Tom
Tom Tromey - June 11, 2010, 2:44 p.m.
>>>>> "Manuel" == Manuel López-Ibáñez <lopezibanez@gmail.com> writes:

Manuel> I agree with Tom that this would be desirable but I think that it is
Manuel> not possible except by modifying pragma_lex to return the location
Manuel> somehow. Probably not worthwhile at this point, since the
Manuel> input_location should be pretty accurate for the lexer, so the only
Manuel> benefit is getting rid of a global.

I forgot to mention: at the very least I think the diagnostic routines
should take the location as a parameter.  If we must use input_location,
let it be in the callers.

Tom
Manuel López-Ibáñez - July 2, 2010, 4:11 p.m.
On 11 June 2010 12:49, Gerald Pfeifer <gerald@pfeifer.com> wrote:
> On Fri, 11 Jun 2010, Manuel López-Ibáñez wrote:
>> Once this go in, please add an entry (preferably with an example) to
>> gcc-4.6/changes.html. This is worth boasting about!
>
> I'd be tempted to make this a new item on our main page as well -- we are
> way, way too modest and silent there, and this is really user visible.
> (Perhaps, since it's not a huge patch, not refer to a company for this
> one, as opposed to some of the bigger ones.)

Go for it please!

Perhaps would be worth mentioning also the widest LTO support, which
is also missing in gcc-4.6/changes.html, and the new -Ofast and
-Wsuggest-attribute=, which are also very user-visible.

Cheers,

Manuel.

Patch

Index: doc/extend.texi
===================================================================
--- doc/extend.texi	(revision 160588)
+++ doc/extend.texi	(working copy)
@@ -12587,21 +12587,36 @@  option.
 @example
 #pragma GCC diagnostic warning "-Wformat"
 #pragma GCC diagnostic error "-Wformat"
 #pragma GCC diagnostic ignored "-Wformat"
 @end example
 
-Note that these pragmas override any command-line options.  Also,
-while it is syntactically valid to put these pragmas anywhere in your
-sources, the only supported location for them is before any data or
-functions are defined.  Doing otherwise may result in unpredictable
-results depending on how the optimizer manages your sources.  If the
-same option is listed multiple times, the last one specified is the
-one that is in effect.  This pragma is not intended to be a general
-purpose replacement for command-line options, but for implementing
-strict control over project policies.
+Note that these pragmas override any command-line options.  GCC keeps
+track of the location of each pragma, and issues diagnostics according
+to the state as of that point in the source file.  Thus, pragmas occurring
+after a line do not affect diagnostics caused by that line.
+
+@item #pragma GCC diagnostic push
+@itemx #pragma GCC diagnostic pop
+
+Causes GCC to remember the state of the diagnostics as of each
+@code{push}, and restore to that point at each @code{pop}.  If a
+@code{pop} has no matching @code{push}, the command line options are
+restored.
+
+@example
+#pragma GCC diagnostic error "-Wuninitialized"
+  foo(a);			/* error is given for this one */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wuninitialized"
+  foo(b);			/* no diagnostic for this one */
+#pragma GCC diagnostic pop
+  foo(c);			/* error is given for this one */
+#pragma GCC diagnostic pop
+  foo(d);			/* depends on command line options */
+@end example
 
 @end table
 
 GCC also offers a simple mechanism for printing messages during
 compilation.
 
Index: c-family/c-pragma.c
===================================================================
--- c-family/c-pragma.c	(revision 160588)
+++ c-family/c-pragma.c	(working copy)
@@ -703,30 +703,34 @@  handle_pragma_diagnostic(cpp_reader *ARG
   const char *kind_string, *option_string;
   unsigned int option_index;
   enum cpp_ttype token;
   diagnostic_t kind;
   tree x;
 
-  if (cfun)
-    {
-      error ("#pragma GCC diagnostic not allowed inside functions");
-      return;
-    }
-
   token = pragma_lex (&x);
   if (token != CPP_NAME)
     GCC_BAD ("missing [error|warning|ignored] after %<#pragma GCC diagnostic%>");
   kind_string = IDENTIFIER_POINTER (x);
   if (strcmp (kind_string, "error") == 0)
     kind = DK_ERROR;
   else if (strcmp (kind_string, "warning") == 0)
     kind = DK_WARNING;
   else if (strcmp (kind_string, "ignored") == 0)
     kind = DK_IGNORED;
+  else if (strcmp (kind_string, "push") == 0)
+    {
+      diagnostic_push_diagnostics (global_dc);
+      return;
+    }
+  else if (strcmp (kind_string, "pop") == 0)
+    {
+      diagnostic_pop_diagnostics (global_dc);
+      return;
+    }
   else
-    GCC_BAD ("expected [error|warning|ignored] after %<#pragma GCC diagnostic%>");
+    GCC_BAD ("expected [error|warning|ignored|push|pop] after %<#pragma GCC diagnostic%>");
 
   token = pragma_lex (&x);
   if (token != CPP_STRING)
     GCC_BAD ("missing option after %<#pragma GCC diagnostic%> kind");
   option_string = TREE_STRING_POINTER (x);
   for (option_index = 0; option_index < cl_options_count; option_index++)
Index: diagnostic.c
===================================================================
--- diagnostic.c	(revision 160588)
+++ diagnostic.c	(working copy)
@@ -313,16 +313,72 @@  diagnostic_classify_diagnostic (diagnost
   if (option_index <= 0
       || option_index >= context->n_opts
       || new_kind >= DK_LAST_DIAGNOSTIC_KIND)
     return DK_UNSPECIFIED;
 
   old_kind = context->classify_diagnostic[option_index];
-  context->classify_diagnostic[option_index] = new_kind;
+
+  /* Handle pragmas separately, since we need to keep track of *where*
+     the pragmas were.  */
+  if (input_location > 0)
+    {
+      int i;
+
+      for (i = context->n_classification_history - 1; i >= 0; i --)
+	if (context->classification_history[i].option == option_index)
+	  {
+	    old_kind = context->classification_history[i].kind;
+	    break;
+	  }
+
+      i = context->n_classification_history;
+      context->classification_history =
+	(diagnostic_classification_change_t *) xrealloc (context->classification_history, (i + 1)
+							 * sizeof (diagnostic_classification_change_t));
+      context->classification_history[i].location = input_location;
+      context->classification_history[i].option = option_index;
+      context->classification_history[i].kind = new_kind;
+      context->n_classification_history ++;
+    }
+  else
+    context->classify_diagnostic[option_index] = new_kind;
+
   return old_kind;
 }
 
+/* Save all diagnostic classifications in a stack.  */
+void
+diagnostic_push_diagnostics (diagnostic_context *context)
+{
+  context->push_list = (int *) xrealloc (context->push_list, (context->n_push + 1) * sizeof (int));
+  context->push_list[context->n_push ++] = context->n_classification_history;
+}
+
+/* Restore the topmost classification set off the stack.  If the stack
+   is empty, revert to the state based on command line parameters.  */
+void
+diagnostic_pop_diagnostics (diagnostic_context *context)
+{
+  int jump_to;
+  int i;
+
+  if (context->n_push)
+    jump_to = context->push_list [-- context->n_push];
+  else
+    jump_to = 0;
+
+  i = context->n_classification_history;
+  context->classification_history =
+    (diagnostic_classification_change_t *) xrealloc (context->classification_history, (i + 1)
+						     * sizeof (diagnostic_classification_change_t));
+  context->classification_history[i].location = input_location;
+  context->classification_history[i].option = jump_to;
+  context->classification_history[i].kind = DK_POP;
+  context->n_classification_history ++;
+}
+
 /* Report a diagnostic message (an error or a warning) as specified by
    DC.  This function is *the* subroutine in terms of which front-ends
    should implement their specific diagnostic handling modules.  The
    front-end independent format specifiers are exactly those described
    in the documentation of output_format.
    Return true if a diagnostic was printed, false otherwise.  */
@@ -371,19 +427,47 @@  diagnostic_report_diagnostic (diagnostic
     {
       diagnostic->kind = DK_ERROR;
     }
 
   if (diagnostic->option_index)
     {
+      diagnostic_t diag_class = DK_UNSPECIFIED;
+
       /* This tests if the user provided the appropriate -Wfoo or
 	 -Wno-foo option.  */
       if (! context->option_enabled (diagnostic->option_index))
 	return false;
+
+      /* This tests for #pragma diagnostic changes.  */
+      if (context->n_classification_history > 0)
+	{
+	  int i;
+	  /* Stupid search.  Optimize later. */
+	  for (i = context->n_classification_history - 1; i >= 0; i --)
+	    {
+	      if (context->classification_history[i].location <= location)
+		{
+		  if (context->classification_history[i].kind == (int) DK_POP)
+		    {
+		      i = context->classification_history[i].option;
+		      continue;
+		    }
+		  if (context->classification_history[i].option == diagnostic->option_index)
+		    {
+		      diag_class = context->classification_history[i].kind;
+		      if (diag_class != DK_UNSPECIFIED)
+			diagnostic->kind = diag_class;
+		      break;
+		    }
+		}
+	    }
+	}
       /* This tests if the user provided the appropriate -Werror=foo
 	 option.  */
-      if (context->classify_diagnostic[diagnostic->option_index] != DK_UNSPECIFIED)
+      if (diag_class == DK_UNSPECIFIED
+	  && context->classify_diagnostic[diagnostic->option_index] != DK_UNSPECIFIED)
 	{
 	  diagnostic->kind = context->classify_diagnostic[diagnostic->option_index];
 	}
       /* This allows for future extensions, like temporarily disabling
 	 warnings for ranges of source code.  */
       if (diagnostic->kind == DK_IGNORED)
Index: diagnostic.h
===================================================================
--- diagnostic.h	(revision 160588)
+++ diagnostic.h	(working copy)
@@ -38,12 +38,22 @@  typedef struct diagnostic_info
   /* The kind of diagnostic it is about.  */
   diagnostic_t kind;
   /* Which OPT_* directly controls this diagnostic.  */
   int option_index;
 } diagnostic_info;
 
+/* Each time a diagnostic's classification is changed with a pragma,
+   we record the change and the location of the change in an array of
+   these structs.  */
+typedef struct diagnostic_classification_change_t
+{
+  location_t location;
+  int option;
+  diagnostic_t kind;
+} diagnostic_classification_change_t;
+
 /*  Forward declarations.  */
 typedef struct diagnostic_context diagnostic_context;
 typedef void (*diagnostic_starter_fn) (diagnostic_context *,
 				       diagnostic_info *);
 typedef diagnostic_starter_fn diagnostic_finalizer_fn;
 
@@ -73,12 +83,26 @@  struct diagnostic_context
      options), this array may contain a new kind that the diagnostic
      should be changed to before reporting, or DK_UNSPECIFIED to leave
      it as the reported kind, or DK_IGNORED to not report it at
      all.  */
   diagnostic_t *classify_diagnostic;
 
+  /* History of all changes to the classifications above.  This list
+     is stored in location-order, so we can search it, either
+     binary-wise or end-to-front, to find the most recent
+     classification for a given diagnostic, given the location of the
+     diagnostic.  */
+  diagnostic_classification_change_t *classification_history;
+
+  /* The size of the above array.  */
+  int n_classification_history;
+
+  /* For pragma push/pop.  */
+  int *push_list;
+  int n_push;
+
   /* True if we should print the command line option which controls
      each diagnostic, if known.  */
   bool show_option_requested;
 
   /* True if we should raise a SIGABRT on errors.  */
   bool abort_on_error;
@@ -226,12 +250,14 @@  extern void diagnostic_finish (diagnosti
 extern void diagnostic_report_current_module (diagnostic_context *);
 
 /* Force diagnostics controlled by OPTIDX to be kind KIND.  */
 extern diagnostic_t diagnostic_classify_diagnostic (diagnostic_context *,
 						    int /* optidx */,
 						    diagnostic_t /* kind */);
+extern void diagnostic_push_diagnostics (diagnostic_context *);
+extern void diagnostic_pop_diagnostics (diagnostic_context *);
 extern bool diagnostic_report_diagnostic (diagnostic_context *,
 					  diagnostic_info *);
 #ifdef ATTRIBUTE_GCC_DIAG
 extern void diagnostic_set_info (diagnostic_info *, const char *, va_list *,
 				 location_t, diagnostic_t) ATTRIBUTE_GCC_DIAG(2,0);
 extern void diagnostic_set_info_translated (diagnostic_info *, const char *,
Index: diagnostic-core.h
===================================================================
--- diagnostic-core.h	(revision 160588)
+++ diagnostic-core.h	(working copy)
@@ -29,13 +29,16 @@  along with GCC; see the file COPYING3.  
 /* Constants used to discriminate diagnostics.  */
 typedef enum
 {
 #define DEFINE_DIAGNOSTIC_KIND(K, msgid) K,
 #include "diagnostic.def"
 #undef DEFINE_DIAGNOSTIC_KIND
-  DK_LAST_DIAGNOSTIC_KIND
+  DK_LAST_DIAGNOSTIC_KIND,
+  /* This is used for tagging pragma pops in the diagnostic
+     classification history chain.  */
+  DK_POP
 } diagnostic_t;
 
 extern const char *progname;
 
 extern const char *trim_filename (const char *);
 
Index: testsuite/gcc.dg/pragma-diag-1.c
===================================================================
--- testsuite/gcc.dg/pragma-diag-1.c	(revision 0)
+++ testsuite/gcc.dg/pragma-diag-1.c	(revision 0)
@@ -0,0 +1,21 @@ 
+/* { dg-do compile } */
+/* { dg-options "-Wuninitialized -O2" } */
+/* { dg-message "warnings being treated as errors" "" {target "*-*-*"} 0 } */
+
+main()
+{
+  int a;
+  int b;
+  int c;
+  int d;
+
+#pragma GCC diagnostic error "-Wuninitialized"
+  foo(a);			/* { dg-error "uninitialized" } */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wuninitialized"
+  foo(b);
+#pragma GCC diagnostic pop
+  foo(c);			/* { dg-error "uninitialized" } */
+#pragma GCC diagnostic pop
+  foo(d);			/* { dg-warning "uninitialized" } */
+}