diagnostic_metadata: unbreak xgettext

Message ID 20200128143017.29239-1-dmalcolm@redhat.com
State New
Headers show
Series
  • diagnostic_metadata: unbreak xgettext
Related show

Commit Message

David Malcolm Jan. 28, 2020, 2:30 p.m.
On Tue, 2020-01-28 at 11:16 +0100, Jakub Jelinek wrote:
> On Wed, Dec 18, 2019 at 07:08:25PM -0500, David Malcolm wrote:

> > This patch adds support for associating a diagnostic message with

> > an

> > optional diagnostic_metadata object, so that plugins can add extra

> > data

> > to their diagnostics (e.g. mapping a diagnostic to a taxonomy or

> > coding

> > standard such as from CERT or MISRA).

> > 

> > Currently this only supports associating a CWE identifier with a

> > diagnostic (which is what I'm using for the warnings in the

> > analyzer

> > patch kit), but adding a diagnostic_metadata class allows for

> > future

> > growth in this area without an explosion of further "warning_at"

> > overloads for all of the different kinds of custom data that a

> > plugin

> > might want to add.

> > 

> > This version of the patch renames the overly-general

> > -fdiagnostics-show-metadata to -fdiagnostics-show-cwe and adds test

> > coverage for it via a plugin.

> > 

> > It also adds a note to the documentation that no GCC diagnostics

> > currently use this; it's a feature for plugins (and, at some point,

> > I hope, the analyzer).

> > 

> > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

> > 

> > Committed to trunk as r279556.

> 

> Unfortunately, this patch broke the i18n.

> $ make gcc.pot

> /bin/sh ../../gcc/../mkinstalldirs po

> make srcextra

> make[1]: Entering directory '/usr/src/gcc/obj/gcc'

> cp -p gengtype-lex.c ../../gcc

> cp -p gengtype-lex.c ../../gcc

> make[1]: Leaving directory '/usr/src/gcc/obj/gcc'

> AWK=gawk /bin/sh ../../gcc/po/exgettext \

> 	/usr/bin/xgettext gcc ../../gcc

> scanning for keywords, %e and %n strings...

> emit_diagnostic_valist used incompatibly as both --

> keyword=emit_diagnostic_valist:4

> --flag=emit_diagnostic_valist:4:gcc-internal-format and --

> keyword=emit_diagnostic_valist:5

> --flag=emit_diagnostic_valist:5:gcc-internal-format

> make: *** [Makefile:4338: po/gcc.pot] Error 1

> 

> While C++ can have overloads, xgettext can't deal with overloads that

> have

> different argument positions.

> emit_diagnostic_valist with the new args (i.e. the :5 one) isn't used

> right

> now, so one way around at least for now is to remove it again,

> another is to

> rename it.

> 

> 	Jakub


Sorry about this.

There are actually two broken names; here's the patch I'm currently testing
(which fixes "make gcc.pot" for me); bootstrap is in progress:


While C++ can have overloads, xgettext can't deal with overloads that have
different argument positions, leading to two failures in "make gcc.pot":

emit_diagnostic_valist used incompatibly as both
--keyword=emit_diagnostic_valist:4
--flag=emit_diagnostic_valist:4:gcc-internal-format and
--keyword=emit_diagnostic_valist:5
--flag=emit_diagnostic_valist:5:gcc-internal-format

warning_at used incompatibly as both
--keyword=warning_at:3 --flag=warning_at:3:gcc-internal-format and
--keyword=warning_at:4 --flag=warning_at:4:gcc-internal-format

The emit_diagnostic_valist overload isn't used anywhere (I think it's
a leftover from an earlier iteration of the analyzer patch kit).

The warning_at overload is used throughout the analyzer but nowhere else.

Ideally I'd like to consolidate this argument with something
constructable in various ways:
- from a metadata and an int or
- from an int (or, better an "enum opt_code"),
so that the overload happens when implicitly choosing the ctor, but
that feels like stage 1 material.

In the meantime, fix xgettext by deleting the unused overload and
renaming the used one.

gcc/analyzer/ChangeLog:
	* region-model.cc (poisoned_value_diagnostic::emit): Update for
	renaming of warning_at overload to warning_with_metadata_at.
	* sm-file.cc (file_leak::emit): Likewise.
	* sm-malloc.cc (double_free::emit): Likewise.
	(possible_null_deref::emit): Likewise.
	(possible_null_arg::emit): Likewise.
	(null_deref::emit): Likewise.
	(null_arg::emit): Likewise.
	(use_after_free::emit): Likewise.
	(malloc_leak::emit): Likewise.
	(free_of_non_heap::emit): Likewise.
	* sm-sensitive.cc (exposure_through_output_file::emit): Likewise.
	* sm-signal.cc (signal_unsafe_call::emit): Likewise.
	* sm-taint.cc (tainted_array_index::emit): Likewise.

gcc/ChangeLog:
	* diagnostic-core.h (warning_at): Rename overload to...
	(warning_with_metadata_at): ...this.
	(emit_diagnostic_valist): Delete decl of overload taking
	diagnostic_metadata.
	* diagnostic.c (emit_diagnostic_valist): Likewise for defn.
	(warning_at): Rename overload taking diagnostic_metadata to...
	(warning_with_metadata_at): ...this.
---
 gcc/analyzer/region-model.cc | 24 ++++++++--------
 gcc/analyzer/sm-file.cc      |  6 ++--
 gcc/analyzer/sm-malloc.cc    | 54 ++++++++++++++++++++----------------
 gcc/analyzer/sm-sensitive.cc |  7 +++--
 gcc/analyzer/sm-signal.cc    |  8 +++---
 gcc/analyzer/sm-taint.cc     | 27 ++++++++++--------
 gcc/diagnostic-core.h        |  9 ++----
 gcc/diagnostic.c             | 16 ++---------
 8 files changed, 74 insertions(+), 77 deletions(-)

-- 
2.21.0

Comments

Jakub Jelinek Jan. 28, 2020, 2:36 p.m. | #1
On Tue, Jan 28, 2020 at 09:30:17AM -0500, David Malcolm wrote:
> 	* diagnostic-core.h (warning_at): Rename overload to...

> 	(warning_with_metadata_at): ...this.


I think this is just too long, especially for a function used at least
in the analyzer pretty often, leading to lots of ugly formatting.
Can't you use warning_meta or something similar instead?

> 	(emit_diagnostic_valist): Delete decl of overload taking

> 	diagnostic_metadata.

> 	* diagnostic.c (emit_diagnostic_valist): Likewise for defn.

> 	(warning_at): Rename overload taking diagnostic_metadata to...

> 	(warning_with_metadata_at): ...this.


	Jakub

Patch

diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 62c96a6ceea..74602f1a66f 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -3827,30 +3827,30 @@  public:
 	{
 	  diagnostic_metadata m;
 	  m.add_cwe (457); /* "CWE-457: Use of Uninitialized Variable".  */
-	  return warning_at (rich_loc, m,
-			     OPT_Wanalyzer_use_of_uninitialized_value,
-			     "use of uninitialized value %qE",
-			     m_expr);
+	  return warning_with_metadata_at
+	    (rich_loc, m, OPT_Wanalyzer_use_of_uninitialized_value,
+	     "use of uninitialized value %qE",
+	     m_expr);
 	}
 	break;
       case POISON_KIND_FREED:
 	{
 	  diagnostic_metadata m;
 	  m.add_cwe (416); /* "CWE-416: Use After Free".  */
-	  return warning_at (rich_loc, m,
-			     OPT_Wanalyzer_use_after_free,
-			     "use after %<free%> of %qE",
-			     m_expr);
+	  return warning_with_metadata_at (rich_loc, m,
+					   OPT_Wanalyzer_use_after_free,
+					   "use after %<free%> of %qE",
+					   m_expr);
 	}
 	break;
       case POISON_KIND_POPPED_STACK:
 	{
 	  diagnostic_metadata m;
 	  /* TODO: which CWE?  */
-	  return warning_at (rich_loc, m,
-			     OPT_Wanalyzer_use_of_pointer_in_stale_stack_frame,
-			     "use of pointer %qE within stale stack frame",
-			     m_expr);
+	  return warning_with_metadata_at
+	    (rich_loc, m, OPT_Wanalyzer_use_of_pointer_in_stale_stack_frame,
+	     "use of pointer %qE within stale stack frame",
+	     m_expr);
 	}
 	break;
       }
diff --git a/gcc/analyzer/sm-file.cc b/gcc/analyzer/sm-file.cc
index 11444926538..695c50d8c31 100644
--- a/gcc/analyzer/sm-file.cc
+++ b/gcc/analyzer/sm-file.cc
@@ -178,9 +178,9 @@  public:
     /* CWE-775: "Missing Release of File Descriptor or Handle after
        Effective Lifetime". */
     m.add_cwe (775);
-    return warning_at (rich_loc, m, OPT_Wanalyzer_file_leak,
-		       "leak of FILE %qE",
-		       m_arg);
+    return warning_with_metadata_at (rich_loc, m, OPT_Wanalyzer_file_leak,
+				     "leak of FILE %qE",
+				     m_arg);
   }
 
   label_text describe_state_change (const evdesc::state_change &change)
diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc
index 526680accf2..54fba26ac6c 100644
--- a/gcc/analyzer/sm-malloc.cc
+++ b/gcc/analyzer/sm-malloc.cc
@@ -145,8 +145,8 @@  public:
     auto_diagnostic_group d;
     diagnostic_metadata m;
     m.add_cwe (415); /* CWE-415: Double Free.  */
-    return warning_at (rich_loc, m, OPT_Wanalyzer_double_free,
-		       "double-%<free%> of %qE", m_arg);
+    return warning_with_metadata_at (rich_loc, m, OPT_Wanalyzer_double_free,
+				     "double-%<free%> of %qE", m_arg);
   }
 
   label_text describe_state_change (const evdesc::state_change &change)
@@ -235,8 +235,9 @@  public:
     /* CWE-690: Unchecked Return Value to NULL Pointer Dereference.  */
     diagnostic_metadata m;
     m.add_cwe (690);
-    return warning_at (rich_loc, m, OPT_Wanalyzer_possible_null_dereference,
-		       "dereference of possibly-NULL %qE", m_arg);
+    return warning_with_metadata_at (rich_loc, m,
+				     OPT_Wanalyzer_possible_null_dereference,
+				     "dereference of possibly-NULL %qE", m_arg);
   }
 
   label_text describe_final_event (const evdesc::final_event &ev) FINAL OVERRIDE
@@ -296,10 +297,10 @@  public:
     auto_diagnostic_group d;
     diagnostic_metadata m;
     m.add_cwe (690);
-    bool warned
-      = warning_at (rich_loc, m, OPT_Wanalyzer_possible_null_argument,
-		    "use of possibly-NULL %qE where non-null expected",
-		    m_arg);
+    bool warned = warning_with_metadata_at
+      (rich_loc, m, OPT_Wanalyzer_possible_null_argument,
+       "use of possibly-NULL %qE where non-null expected",
+       m_arg);
     if (warned)
       inform_nonnull_attribute (m_fndecl, m_arg_idx);
     return warned;
@@ -338,8 +339,9 @@  public:
     /* CWE-690: Unchecked Return Value to NULL Pointer Dereference.  */
     diagnostic_metadata m;
     m.add_cwe (690);
-    return warning_at (rich_loc, m, OPT_Wanalyzer_null_dereference,
-		       "dereference of NULL %qE", m_arg);
+    return warning_with_metadata_at (rich_loc, m,
+				     OPT_Wanalyzer_null_dereference,
+				     "dereference of NULL %qE", m_arg);
   }
 
   label_text describe_return_of_state (const evdesc::return_of_state &info)
@@ -386,8 +388,11 @@  public:
     auto_diagnostic_group d;
     diagnostic_metadata m;
     m.add_cwe (690);
-    bool warned = warning_at (rich_loc, m, OPT_Wanalyzer_null_argument,
-			      "use of NULL %qE where non-null expected", m_arg);
+    bool warned
+      = warning_with_metadata_at (rich_loc,
+				  m, OPT_Wanalyzer_null_argument,
+				  "use of NULL %qE where non-null expected",
+				  m_arg);
     if (warned)
       inform_nonnull_attribute (m_fndecl, m_arg_idx);
     return warned;
@@ -419,8 +424,8 @@  public:
     /* CWE-416: Use After Free.  */
     diagnostic_metadata m;
     m.add_cwe (416);
-    return warning_at (rich_loc, m, OPT_Wanalyzer_use_after_free,
-		       "use after %<free%> of %qE", m_arg);
+    return warning_with_metadata_at (rich_loc, m, OPT_Wanalyzer_use_after_free,
+				     "use after %<free%> of %qE", m_arg);
   }
 
   label_text describe_state_change (const evdesc::state_change &change)
@@ -459,8 +464,8 @@  public:
   {
     diagnostic_metadata m;
     m.add_cwe (401);
-    return warning_at (rich_loc, m, OPT_Wanalyzer_malloc_leak,
-		       "leak of %qE", m_arg);
+    return warning_with_metadata_at (rich_loc, m, OPT_Wanalyzer_malloc_leak,
+				     "leak of %qE", m_arg);
   }
 
   label_text describe_state_change (const evdesc::state_change &change)
@@ -514,16 +519,17 @@  public:
       default:
 	gcc_unreachable ();
       case KIND_UNKNOWN:
-	return warning_at (rich_loc, m, OPT_Wanalyzer_free_of_non_heap,
-			   "%<free%> of %qE which points to memory"
-			   " not on the heap",
-			   m_arg);
+	return warning_with_metadata_at
+	  (rich_loc, m, OPT_Wanalyzer_free_of_non_heap,
+	   "%<free%> of %qE which points to memory not on the heap",
+	   m_arg);
 	break;
       case KIND_ALLOCA:
-	return warning_at (rich_loc, m, OPT_Wanalyzer_free_of_non_heap,
-			   "%<free%> of memory allocated on the stack by"
-			   " %qs (%qE) will corrupt the heap",
-			   "alloca", m_arg);
+	return warning_with_metadata_at
+	  (rich_loc, m, OPT_Wanalyzer_free_of_non_heap,
+	   "%<free%> of memory allocated on the stack by"
+	   " %qs (%qE) will corrupt the heap",
+	   "alloca", m_arg);
 	break;
       }
   }
diff --git a/gcc/analyzer/sm-sensitive.cc b/gcc/analyzer/sm-sensitive.cc
index a067c18fba4..04654a3ef62 100644
--- a/gcc/analyzer/sm-sensitive.cc
+++ b/gcc/analyzer/sm-sensitive.cc
@@ -105,9 +105,10 @@  public:
     diagnostic_metadata m;
     /* CWE-532: Information Exposure Through Log Files */
     m.add_cwe (532);
-    return warning_at (rich_loc, m, OPT_Wanalyzer_exposure_through_output_file,
-		       "sensitive value %qE written to output file",
-		       m_arg);
+    return warning_with_metadata_at
+      (rich_loc, m, OPT_Wanalyzer_exposure_through_output_file,
+       "sensitive value %qE written to output file",
+       m_arg);
   }
 
   label_text describe_state_change (const evdesc::state_change &change)
diff --git a/gcc/analyzer/sm-signal.cc b/gcc/analyzer/sm-signal.cc
index b4daf3ad617..3031993e869 100644
--- a/gcc/analyzer/sm-signal.cc
+++ b/gcc/analyzer/sm-signal.cc
@@ -126,10 +126,10 @@  public:
     diagnostic_metadata m;
     /* CWE-479: Signal Handler Use of a Non-reentrant Function.  */
     m.add_cwe (479);
-    return warning_at (rich_loc, m,
-		       OPT_Wanalyzer_unsafe_call_within_signal_handler,
-		       "call to %qD from within signal handler",
-		       m_unsafe_fndecl);
+    return warning_with_metadata_at
+      (rich_loc, m, OPT_Wanalyzer_unsafe_call_within_signal_handler,
+       "call to %qD from within signal handler",
+       m_unsafe_fndecl);
   }
 
   label_text describe_state_change (const evdesc::state_change &change)
diff --git a/gcc/analyzer/sm-taint.cc b/gcc/analyzer/sm-taint.cc
index 00b794a3698..0ea2c764454 100644
--- a/gcc/analyzer/sm-taint.cc
+++ b/gcc/analyzer/sm-taint.cc
@@ -114,22 +114,25 @@  public:
       default:
 	gcc_unreachable ();
       case BOUNDS_NONE:
-	return warning_at (rich_loc, m, OPT_Wanalyzer_tainted_array_index,
-			   "use of tainted value %qE in array lookup"
-			   " without bounds checking",
-			   m_arg);
+	return warning_with_metadata_at
+	  (rich_loc, m, OPT_Wanalyzer_tainted_array_index,
+	   "use of tainted value %qE in array lookup"
+	   " without bounds checking",
+	   m_arg);
 	break;
       case BOUNDS_UPPER:
-	return warning_at (rich_loc, m, OPT_Wanalyzer_tainted_array_index,
-			   "use of tainted value %qE in array lookup"
-			   " without lower-bounds checking",
-			   m_arg);
+	return warning_with_metadata_at
+	  (rich_loc, m, OPT_Wanalyzer_tainted_array_index,
+	   "use of tainted value %qE in array lookup"
+	   " without lower-bounds checking",
+	   m_arg);
 	break;
       case BOUNDS_LOWER:
-	return warning_at (rich_loc, m, OPT_Wanalyzer_tainted_array_index,
-			   "use of tainted value %qE in array lookup"
-			   " without upper-bounds checking",
-			   m_arg);
+	return warning_with_metadata_at
+	  (rich_loc, m, OPT_Wanalyzer_tainted_array_index,
+	   "use of tainted value %qE in array lookup"
+	   " without upper-bounds checking",
+	   m_arg);
 	break;
       }
   }
diff --git a/gcc/diagnostic-core.h b/gcc/diagnostic-core.h
index 38b8557fd55..e322326531c 100644
--- a/gcc/diagnostic-core.h
+++ b/gcc/diagnostic-core.h
@@ -81,8 +81,9 @@  extern bool warning_at (location_t, int, const char *, ...)
     ATTRIBUTE_GCC_DIAG(3,4);
 extern bool warning_at (rich_location *, int, const char *, ...)
     ATTRIBUTE_GCC_DIAG(3,4);
-extern bool warning_at (rich_location *, const diagnostic_metadata &, int,
-			const char *, ...)
+extern bool warning_with_metadata_at (rich_location *,
+				      const diagnostic_metadata &, int,
+				      const char *, ...)
     ATTRIBUTE_GCC_DIAG(4,5);
 extern void error (const char *, ...) ATTRIBUTE_GCC_DIAG(1,2);
 extern void error_n (location_t, unsigned HOST_WIDE_INT, const char *,
@@ -115,10 +116,6 @@  extern bool emit_diagnostic (diagnostic_t, rich_location *, int,
 			     const char *, ...) ATTRIBUTE_GCC_DIAG(4,5);
 extern bool emit_diagnostic_valist (diagnostic_t, location_t, int, const char *,
 				    va_list *) ATTRIBUTE_GCC_DIAG (4,0);
-extern bool emit_diagnostic_valist (diagnostic_t, rich_location *,
-				    const diagnostic_metadata *metadata,
-				    int, const char *, va_list *)
-  ATTRIBUTE_GCC_DIAG (5,0);
 extern bool seen_error (void);
 
 #ifdef BUFSIZ
diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index 72afd7c6adf..4a599e4c380 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -1356,17 +1356,6 @@  emit_diagnostic_valist (diagnostic_t kind, location_t location, int opt,
   return diagnostic_impl (&richloc, NULL, opt, gmsgid, ap, kind);
 }
 
-/* Wrapper around diagnostic_impl taking a va_list parameter.  */
-
-bool
-emit_diagnostic_valist (diagnostic_t kind, rich_location *richloc,
-			const diagnostic_metadata *metadata,
-			int opt,
-			const char *gmsgid, va_list *ap)
-{
-  return diagnostic_impl (richloc, metadata, opt, gmsgid, ap, kind);
-}
-
 /* An informative note at LOCATION.  Use this for additional details on an error
    message.  */
 void
@@ -1457,8 +1446,9 @@  warning_at (rich_location *richloc, int opt, const char *gmsgid, ...)
 /* Same as "warning at" above, but using METADATA.  */
 
 bool
-warning_at (rich_location *richloc, const diagnostic_metadata &metadata,
-	    int opt, const char *gmsgid, ...)
+warning_with_metadata_at (rich_location *richloc,
+			  const diagnostic_metadata &metadata,
+			  int opt, const char *gmsgid, ...)
 {
   gcc_assert (richloc);