From: David Malcolm Date: Mon, 30 Sep 2024 15:48:29 +0000 (-0400) Subject: diagnostics: use "%e" to avoid intermediate strings [PR116613] X-Git-Tag: basepoints/gcc-16~5579 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=3d3d20ccd8365970f34002bfe0a632f2868bc95b;p=thirdparty%2Fgcc.git diagnostics: use "%e" to avoid intermediate strings [PR116613] 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. 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 --- diff --git a/gcc/analyzer/kf-analyzer.cc b/gcc/analyzer/kf-analyzer.cc index 26c2e41da6f..da49baa5bff 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 &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 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 &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 d0251833c10..9fb564bd55d 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 &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 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 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 99c45750d59..00a83a785b4 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 &); extern void maybe_diag_alias_attributes (tree, tree); diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc index a3a697dd126..43468e5f62e 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 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 6900790d44e..950d96bf9d6 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 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 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 de9e4bda6ad..8e6897ce25f 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 &strings) + : m_strings (strings) + { + } + + void add_to_phase_2 (context &ctxt) final override; + +private: + const auto_vec &m_strings; +}; + } // namespace pp_markup #endif /* GCC_PRETTY_PRINT_MARKUP_H */ diff --git a/gcc/pretty-print.cc b/gcc/pretty-print.cc index 6a1d1ec484b..af452101693 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 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 none; + pp_markup::comma_separated_quoted_strings e_none (none); + + auto_vec one; + one.safe_push ("one"); + pp_markup::comma_separated_quoted_strings e_one (one); + + auto_vec 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 b3896564ef8..0d3949bac89 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'" } */ }