[pushed:,r15-3974] diagnostics: use "%e" to avoid intermediate strings [PR116613]

Message ID 20240930160310.2857130-1-dmalcolm@redhat.com
State Committed
Commit 3d3d20ccd8365970f34002bfe0a632f2868bc95b
Headers
Series [pushed:,r15-3974] diagnostics: use "%e" to avoid intermediate strings [PR116613] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 fail Patch failed to apply
linaro-tcwg-bot/tcwg_gcc_build--master-arm fail Patch failed to apply

Commit Message

David Malcolm Sept. 30, 2024, 4:03 p.m. UTC
  Various diagnostics build an intermediate string, potentially with
colorization, and then use this in a diagnostic message.

This won't work if we have multiple diagnostic sinks, where some might
be colorized and some not.

This patch reworks such places using "%e" and pp_element subclasses, so
that any colorization happens within report_diagnostic's call to
pp_format.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to trunk as r15-3974-g3d3d20ccd83659.

gcc/analyzer/ChangeLog:
	PR other/116613
	* kf-analyzer.cc: Include "pretty-print-markup.h".
	(kf_analyzer_dump_escaped::impl_call_pre): Defer colorization
	choices by eliminating the construction of a intermediate string,
	replacing it with a new pp_element subclass via "%e".

gcc/ChangeLog:
	PR other/116613
	* attribs.cc: Include "pretty-print-markup.h".
	(decls_mismatched_attributes): Defer colorization choices by
	replacing printing to a pretty_printer * param with appending
	to a vec of strings.
	(maybe_diag_alias_attributes): As above, replacing pretty_printer
	with usage of pp_markup::comma_separated_quoted_strings and "%e"
	in two places.
	* attribs.h (decls_mismatched_attributes): Update decl.
	* gimple-ssa-warn-access.cc: Include "pretty-print-markup.h".
	(pass_waccess::maybe_warn_memmodel): Defer colorization choices by
	replacing printing to a pretty_printer * param with use of
	pp_markup::comma_separated_quoted_strings and "%e".
	(pass_waccess::maybe_warn_memmodel): Likewise, replacing printing
	to a temporary buffer.
	* pretty-print-markup.h
	(class pp_markup::comma_separated_quoted_strings): New.
	* pretty-print.cc
	(pp_markup::comma_separated_quoted_strings::add_to_phase_2): New.
	(selftest::test_pp_printf_within_pp_element): New.
	(selftest::test_comma_separated_quoted_strings): New.
	(selftest::pretty_print_cc_tests): Call the new tests.

gcc/cp/ChangeLog:
	PR other/116613
	* pt.cc: Include "pretty-print-markup.h".
	(warn_spec_missing_attributes): Defer colorization choices by
	replacing printing to a pretty_printer * param with appending
	to a vec of strings.  Replace pretty_printer with usage of
	pp_markup::comma_separated_quoted_strings and "%e".

gcc/testsuite/ChangeLog:
	PR other/116613
	* c-c++-common/analyzer/escaping-1.c: Update expected results to
	remove type information from C++ results.  Previously we were
	using %qD with default_tree_printer, which used
	lang_hooks.decl_printable_name, whereas now we're using %qD with
	a clone of the cxx_pretty_printer.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>
---
 gcc/analyzer/kf-analyzer.cc                   | 42 ++++++---
 gcc/attribs.cc                                | 33 ++++---
 gcc/attribs.h                                 |  2 +-
 gcc/cp/pt.cc                                  | 18 ++--
 gcc/gimple-ssa-warn-access.cc                 | 21 ++---
 gcc/pretty-print-markup.h                     | 17 ++++
 gcc/pretty-print.cc                           | 92 +++++++++++++++++++
 .../c-c++-common/analyzer/escaping-1.c        |  9 +-
 8 files changed, 179 insertions(+), 55 deletions(-)
  

Patch

diff --git a/gcc/analyzer/kf-analyzer.cc b/gcc/analyzer/kf-analyzer.cc
index 26c2e41da6ff..da49baa5bff1 100644
--- a/gcc/analyzer/kf-analyzer.cc
+++ b/gcc/analyzer/kf-analyzer.cc
@@ -36,6 +36,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "analyzer/pending-diagnostic.h"
 #include "analyzer/call-details.h"
 #include "make-unique.h"
+#include "pretty-print-markup.h"
 
 #if ENABLE_ANALYZER
 
@@ -176,23 +177,40 @@  public:
        probably most user-friendly.  */
     escaped_decls.qsort (cmp_decls_ptr_ptr);
 
-    pretty_printer pp;
-    pp_format_decoder (&pp) = default_tree_printer;
-    pp_show_color (&pp) = pp_show_color (global_dc->m_printer);
-    bool first = true;
-    for (auto iter : escaped_decls)
+    class escaped_list_element : public pp_element
+    {
+    public:
+      escaped_list_element (auto_vec<tree> &escaped_decls)
+      : m_escaped_decls (escaped_decls)
       {
-	if (first)
-	  first = false;
-	else
-	  pp_string (&pp, ", ");
-	pp_printf (&pp, "%qD", iter);
       }
+
+      void add_to_phase_2 (pp_markup::context &ctxt) final override
+      {
+	/* We can't call pp_printf directly on ctxt.m_pp from within
+	   formatting.  As a workaround, work with a clone of the pp.  */
+	std::unique_ptr<pretty_printer> pp (ctxt.m_pp.clone ());
+	bool first = true;
+	for (auto iter : m_escaped_decls)
+	  {
+	    if (first)
+	      first = false;
+	    else
+	      pp_string (pp.get (), ", ");
+	    pp_printf (pp.get (), "%qD", iter);
+	  }
+	pp_string (&ctxt.m_pp, pp_formatted_text (pp.get ()));
+      }
+
+    private:
+      auto_vec<tree> &m_escaped_decls;
+    } e_escaped (escaped_decls);
+
     /* Print the number to make it easier to write DejaGnu tests for
        the "nothing has escaped" case.  */
-    warning_at (cd.get_location (), 0, "escaped: %i: %s",
+    warning_at (cd.get_location (), 0, "escaped: %i: %e",
 		escaped_decls.length (),
-		pp_formatted_text (&pp));
+		&e_escaped);
   }
 };
 
diff --git a/gcc/attribs.cc b/gcc/attribs.cc
index d0251833c10e..9fb564bd55dd 100644
--- a/gcc/attribs.cc
+++ b/gcc/attribs.cc
@@ -35,6 +35,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "hash-set.h"
 #include "diagnostic.h"
 #include "pretty-print.h"
+#include "pretty-print-markup.h"
 #include "tree-pretty-print.h"
 #include "intl.h"
 
@@ -2196,15 +2197,14 @@  has_attribute (tree node, tree attrs, const char *aname)
    the "template" function declaration TMPL and DECL.  The word "template"
    doesn't necessarily refer to a C++ template but rather a declaration
    whose attributes should be matched by those on DECL.  For a non-zero
-   return value set *ATTRSTR to a string representation of the list of
-   mismatched attributes with quoted names.
+   return value append the names of the mismatcheed attributes to OUTATTRS.
    ATTRLIST is a list of additional attributes that SPEC should be
    taken to ultimately be declared with.  */
 
 unsigned
 decls_mismatched_attributes (tree tmpl, tree decl, tree attrlist,
 			     const char* const blacklist[],
-			     pretty_printer *attrstr)
+			     auto_vec<const char *> &outattrs)
 {
   if (TREE_CODE (tmpl) != FUNCTION_DECL)
     return 0;
@@ -2278,11 +2278,7 @@  decls_mismatched_attributes (tree tmpl, tree decl, tree attrlist,
 
 	  if (!found)
 	    {
-	      if (nattrs)
-		pp_string (attrstr, ", ");
-	      pp_begin_quote (attrstr, pp_show_color (global_dc->m_printer));
-	      pp_string (attrstr, blacklist[i]);
-	      pp_end_quote (attrstr, pp_show_color (global_dc->m_printer));
+	      outattrs.safe_push (blacklist[i]);
 	      ++nattrs;
 	    }
 
@@ -2312,23 +2308,24 @@  maybe_diag_alias_attributes (tree alias, tree target)
     "returns_twice", NULL
   };
 
-  pretty_printer attrnames;
   if (warn_attribute_alias > 1)
     {
       /* With -Wattribute-alias=2 detect alias declarations that are more
 	 restrictive than their targets first.  Those indicate potential
 	 codegen bugs.  */
+      auto_vec<const char *> mismatches;
       if (unsigned n = decls_mismatched_attributes (alias, target, NULL_TREE,
-						    blacklist, &attrnames))
+						    blacklist, mismatches))
 	{
 	  auto_diagnostic_group d;
+	  pp_markup::comma_separated_quoted_strings e (mismatches);
 	  if (warning_n (DECL_SOURCE_LOCATION (alias),
 			 OPT_Wattribute_alias_, n,
 			 "%qD specifies more restrictive attribute than "
-			 "its target %qD: %s",
+			 "its target %qD: %e",
 			 "%qD specifies more restrictive attributes than "
-			 "its target %qD: %s",
-			 alias, target, pp_formatted_text (&attrnames)))
+			 "its target %qD: %e",
+			 alias, target, &e))
 	    inform (DECL_SOURCE_LOCATION (target),
 		    "%qD target declared here", alias);
 	  return;
@@ -2338,17 +2335,19 @@  maybe_diag_alias_attributes (tree alias, tree target)
   /* Detect alias declarations that are less restrictive than their
      targets.  Those suggest potential optimization opportunities
      (solved by adding the missing attribute(s) to the alias).  */
+  auto_vec<const char *> mismatches;
   if (unsigned n = decls_mismatched_attributes (target, alias, NULL_TREE,
-						blacklist, &attrnames))
+						blacklist, mismatches))
     {
       auto_diagnostic_group d;
+      pp_markup::comma_separated_quoted_strings e (mismatches);
       if (warning_n (DECL_SOURCE_LOCATION (alias),
 		     OPT_Wmissing_attributes, n,
 		     "%qD specifies less restrictive attribute than "
-		     "its target %qD: %s",
+		     "its target %qD: %e",
 		     "%qD specifies less restrictive attributes than "
-		     "its target %qD: %s",
-		     alias, target, pp_formatted_text (&attrnames)))
+		     "its target %qD: %e",
+		     alias, target, &e))
 	inform (DECL_SOURCE_LOCATION (target),
 		"%qD target declared here", alias);
     }
diff --git a/gcc/attribs.h b/gcc/attribs.h
index 99c45750d592..00a83a785b45 100644
--- a/gcc/attribs.h
+++ b/gcc/attribs.h
@@ -131,7 +131,7 @@  extern tree private_lookup_attribute (const char *attr_ns,
 
 extern unsigned decls_mismatched_attributes (tree, tree, tree,
 					     const char* const[],
-					     pretty_printer*);
+					     auto_vec<const char *> &);
 
 extern void maybe_diag_alias_attributes (tree, tree);
 
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index e826206be164..37fef7f29eb4 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -48,6 +48,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "target.h"
 #include "builtins.h"
 #include "omp-general.h"
+#include "pretty-print-markup.h"
 
 /* The type of functions taking a tree, and some additional data, and
    returning an int.  */
@@ -2764,9 +2765,9 @@  warn_spec_missing_attributes (tree tmpl, tree spec, tree attrlist)
   /* Put together a list of the black listed attributes that the primary
      template is declared with that the specialization is not, in case
      it's not apparent from the most recent declaration of the primary.  */
-  pretty_printer str;
+  auto_vec<const char *> mismatches;
   unsigned nattrs = decls_mismatched_attributes (tmpl, spec, attrlist,
-						 blacklist, &str);
+						 blacklist, mismatches);
 
   if (!nattrs)
     return;
@@ -2775,11 +2776,14 @@  warn_spec_missing_attributes (tree tmpl, tree spec, tree attrlist)
   if (warning_at (DECL_SOURCE_LOCATION (spec), OPT_Wmissing_attributes,
 		  "explicit specialization %q#D may be missing attributes",
 		  spec))
-    inform (DECL_SOURCE_LOCATION (tmpl),
-	    nattrs > 1
-	    ? G_("missing primary template attributes %s")
-	    : G_("missing primary template attribute %s"),
-	    pp_formatted_text (&str));
+    {
+      pp_markup::comma_separated_quoted_strings e (mismatches);
+      inform (DECL_SOURCE_LOCATION (tmpl),
+	      nattrs > 1
+	      ? G_("missing primary template attributes %e")
+	      : G_("missing primary template attribute %e"),
+	      &e);
+    }
 }
 
 /* Check to see if the function just declared, as indicated in
diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 6900790d44ee..950d96bf9d62 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -55,6 +55,7 @@ 
 #include "demangle.h"
 #include "attr-fnspec.h"
 #include "pointer-query.h"
+#include "pretty-print-markup.h"
 
 /* Return true if tree node X has an associated location.  */
 
@@ -2942,15 +2943,14 @@  pass_waccess::maybe_warn_memmodel (gimple *stmt, tree ord_sucs,
 	return false;
 
       /* Print a note with the valid memory models.  */
-      pretty_printer pp;
-      pp_show_color (&pp) = pp_show_color (global_dc->m_printer);
+      auto_vec<const char *> strings;
       for (unsigned i = 0; valid[i] != UCHAR_MAX; ++i)
 	{
 	  const char *modname = memory_models[valid[i]].modname;
-	  pp_printf (&pp, "%s%qs", i ? ", " : "", modname);
+	  strings.safe_push (modname);
 	}
-
-      inform (loc, "valid models are %s", pp_formatted_text (&pp));
+      pp_markup::comma_separated_quoted_strings e (strings);
+      inform (loc, "valid models are %e", &e);
       return true;
     }
 
@@ -2992,19 +2992,16 @@  pass_waccess::maybe_warn_memmodel (gimple *stmt, tree ord_sucs,
 
 	/* Print a note with the valid failure memory models which are
 	   those with a value less than or equal to the success mode.  */
-	char buf[120];
-	*buf = '\0';
+	auto_vec<const char *> strings;
 	for (unsigned i = 0;
 	     memory_models[i].modval <= memmodel_base (sucs); ++i)
 	  {
-	    if (*buf)
-	      strcat (buf, ", ");
-
 	    const char *modname = memory_models[valid[i]].modname;
-	    sprintf (buf + strlen (buf), "'%s'", modname);
+	    strings.safe_push (modname);
 	  }
+	pp_markup::comma_separated_quoted_strings e (strings);
 
-	inform (loc, "valid models are %s", buf);
+	inform (loc, "valid models are %e", &e);
 	return true;
       }
 
diff --git a/gcc/pretty-print-markup.h b/gcc/pretty-print-markup.h
index de9e4bda6ade..8e6897ce25ff 100644
--- a/gcc/pretty-print-markup.h
+++ b/gcc/pretty-print-markup.h
@@ -70,6 +70,23 @@  private:
   DISABLE_COPY_AND_ASSIGN (element);
 };
 
+/* Concrete subclass: handle "%e" by printing a comma-separated list
+   of quoted strings.  */
+
+class comma_separated_quoted_strings : public element
+{
+public:
+  comma_separated_quoted_strings (const auto_vec<const char *> &strings)
+  : m_strings (strings)
+  {
+  }
+
+  void add_to_phase_2 (context &ctxt) final override;
+
+private:
+  const auto_vec<const char *> &m_strings;
+};
+
 } // namespace pp_markup
 
 #endif /* GCC_PRETTY_PRINT_MARKUP_H */
diff --git a/gcc/pretty-print.cc b/gcc/pretty-print.cc
index 2b865212ac55..c88be8ee099a 100644
--- a/gcc/pretty-print.cc
+++ b/gcc/pretty-print.cc
@@ -3092,6 +3092,19 @@  pp_markup::context::push_back_any_text ()
 				    const char *)));
 }
 
+void
+pp_markup::comma_separated_quoted_strings::add_to_phase_2 (context &ctxt)
+{
+  for (unsigned i = 0; i < m_strings.length (); i++)
+    {
+      if (i > 0)
+	pp_string (&ctxt.m_pp, ", ");
+      ctxt.begin_quote ();
+      pp_string (&ctxt.m_pp, m_strings[i]);
+      ctxt.end_quote ();
+    }
+}
+
 /* Color names for expressing "expected" vs "actual" values.  */
 const char *const highlight_colors::expected = "highlight-a";
 const char *const highlight_colors::actual   = "highlight-b";
@@ -3669,6 +3682,54 @@  test_pp_format_stack ()
 		"In function: `test_fn'\nunexpected foo: 42 bar: `test'");
 }
 
+/* Verify usage of pp_printf from within a pp_element's
+   add_to_phase_2 vfunc.  */
+static void
+test_pp_printf_within_pp_element ()
+{
+  class kv_element : public pp_element
+  {
+  public:
+    kv_element (const char *key, int value)
+    : m_key (key), m_value (value)
+    {
+    }
+
+    void add_to_phase_2 (pp_markup::context &ctxt) final override
+    {
+      /* We can't call pp_printf directly on ctxt.m_pp from within
+	 formatting.  As a workaround, work with a clone of the pp.  */
+      std::unique_ptr<pretty_printer> pp (ctxt.m_pp.clone ());
+      pp_printf (pp.get (), "(%qs: %qi)", m_key, m_value);
+      pp_string (&ctxt.m_pp, pp_formatted_text (pp.get ()));
+    }
+
+  private:
+    const char *m_key;
+    int m_value;
+  };
+
+  auto_fix_quotes fix_quotes;
+
+  kv_element e1 ("foo", 42);
+  kv_element e2 ("bar", 1066);
+  ASSERT_PP_FORMAT_2 ("before (`foo': `42') (`bar': `1066') after",
+		      "before %e %e after",
+		      &e1, &e2);
+  assert_pp_format_colored (SELFTEST_LOCATION,
+			    ("before "
+			     "(`\33[01m\33[Kfoo\33[m\33[K'"
+			     ": "
+			     "`\33[01m\33[K42\33[m\33[K')"
+			     " "
+			     "(`\33[01m\33[Kbar\33[m\33[K'"
+			     ": "
+			     "`\33[01m\33[K1066\33[m\33[K')"
+			     " after"),
+			    "before %e %e after",
+			    &e1, &e2);
+}
+
 /* A subclass of pretty_printer for use by test_prefixes_and_wrapping.  */
 
 class test_pretty_printer : public pretty_printer
@@ -4088,6 +4149,35 @@  static void test_utf8 ()
 
 }
 
+/* Verify that class comma_separated_quoted_strings works as expected.  */
+
+static void
+test_comma_separated_quoted_strings ()
+{
+  auto_fix_quotes fix_quotes;
+
+  auto_vec<const char *> none;
+  pp_markup::comma_separated_quoted_strings e_none (none);
+
+  auto_vec<const char *> one;
+  one.safe_push ("one");
+  pp_markup::comma_separated_quoted_strings e_one (one);
+
+  auto_vec<const char *> many;
+  many.safe_push ("0");
+  many.safe_push ("1");
+  many.safe_push ("2");
+  pp_markup::comma_separated_quoted_strings e_many (many);
+
+  ASSERT_PP_FORMAT_3 ("none: () one: (`one') many: (`0', `1', `2')",
+		      "none: (%e) one: (%e) many: (%e)",
+		      &e_none, &e_one, &e_many);
+  assert_pp_format_colored (SELFTEST_LOCATION,
+			    "one: (`one')",
+			    "one: (%e)",
+			    &e_one);
+}
+
 /* Run all of the selftests within this file.  */
 
 void
@@ -4099,12 +4189,14 @@  pretty_print_cc_tests ()
   test_custom_tokens_1 ();
   test_custom_tokens_2 ();
   test_pp_format_stack ();
+  test_pp_printf_within_pp_element ();
   test_prefixes_and_wrapping ();
   test_urls ();
   test_urls_from_braces ();
   test_null_urls ();
   test_urlification ();
   test_utf8 ();
+  test_comma_separated_quoted_strings ();
 }
 
 } // namespace selftest
diff --git a/gcc/testsuite/c-c++-common/analyzer/escaping-1.c b/gcc/testsuite/c-c++-common/analyzer/escaping-1.c
index b3896564ef87..0d3949bac899 100644
--- a/gcc/testsuite/c-c++-common/analyzer/escaping-1.c
+++ b/gcc/testsuite/c-c++-common/analyzer/escaping-1.c
@@ -13,16 +13,13 @@  static void test_1 (void)
   __analyzer_dump_escaped (); /* { dg-warning "escaped: 0: " } */
 
   unknown_fn (&local_1);
-  __analyzer_dump_escaped (); /* { dg-warning "escaped: 1: 'local_1'" "" { target c } } */
-  /* { dg-warning "escaped: 1: 'int local_1'" "" { target c++ } .-1 } */
+  __analyzer_dump_escaped (); /* { dg-warning "escaped: 1: 'local_1'" } */
 
   /* Should be idempotent.  */
   unknown_fn (&local_1);
-  __analyzer_dump_escaped (); /* { dg-warning "escaped: 1: 'local_1'" "" { target c } } */
-  /* { dg-warning "escaped: 1: 'int local_1'" "" { target c++ } .-1 } */
+  __analyzer_dump_escaped (); /* { dg-warning "escaped: 1: 'local_1'" } */
 
   /* Escape a static global.  */
   unknown_fn (&only_used_by_test_1);
-  __analyzer_dump_escaped (); /* { dg-warning "escaped: 2: 'local_1', 'only_used_by_test_1'" "" { target c } } */
-  /* { dg-warning "escaped: 2: 'int local_1', 'int only_used_by_test_1'" "" { target c++ } .-1 } */
+  __analyzer_dump_escaped (); /* { dg-warning "escaped: 2: 'local_1', 'only_used_by_test_1'" } */
 }