[PATCH 2/2] C++: bulletproof the %H and %I format codes (PR c++/81167)
David Malcolm
dmalcolm@redhat.com
Thu Jun 22 23:47:00 GMT 2017
The %T format code in the C++ frontend gracefully handles being passed
a NULL type, printing nothing (and hence '' for %qT).
In r248698 (template type diff printing) I converted many uses of pairs
of %qT in the C++ FE to %qH and %qI.
PR c++/81167 reports a case where a NULL is passed to one of these %qH,
and it turns out that we now ICE for this case (with a gcc_assert)
whereas previously we printed a '' for the type.
This patch slightly reworks the %H and %I-handling code so that it
gracefully handles NULL, fixing the ICE in that PR.
Whilst I was at it, I also fixed things so that if only one of the
%H/%I codes is present, we do the right thing (i.e. fall back to
the %T behavior).
Much of the patch is work to generalize the helper code for the
selftests in pretty-print.c so that we can write selftests for
cp/error.c to directly exercise the various cases.
Presumably passing NULL to any of %T, %H, or %I is a bug; this
patch is merely defensive coding against that so that we don't crash;
the patch doesn't address the underlying bug (see the PR for more
details).
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu in
combination with the previous patch.
OK for trunk?
gcc/cp/ChangeLog:
* cp-lang.c (selftest::run_cp_tests): Call
selftest::run_cp_error_c_tests.
* cp-tree.h (selftestrun_cp_error_c_tests): New decl.
* error.c: Include "selftest.h".
(struct deferred_printed_type): Remove assertion that "type"
is non-NULL.
(comparable_template_types_p): Handle NULL types.
(cxx_format_postprocessor::handle): Likewise. Also, handle
one of the %H or %I being missing.
(selftest::cp_pretty_printer_test): New class.
(selftest::test_type_printing): New function.
(selftest::test_cp_printer): New function.
(selftest::run_cp_error_c_tests): New function.
gcc/ChangeLog:
* pretty-print.c
(selftest::pretty_printer_test::pretty_printer_test): New ctor.
(selftest::pretty_printer_test::~pretty_printer_test): New dtor.
(selftest::assert_pp_format): Convert to...
(selftest::pretty_printer_test::assert_pp_format): ...method.
(selftest::assert_pp_format_colored): Convert to...
(selftest::pretty_printer_test::assert_pp_format_colored): ...method.
(selftest::assert_pp_format_va): Convert to...
(selftest::pretty_printer_test::assert_pp_format_va): ...method,
and call set_up_printer.
(ASSERT_PP_FORMAT_1, ASSERT_PP_FORMAT_2, ASSERT_PP_FORMAT_3): Move to
selftest.h, adding "test." to handle conversion from functions to
methods.
(selftest::test_pp_format): Add pretty_printer_test instance,
moving save/restore of open_quote/close_quote to
pretty_printer_test's ctor and dtor. Update for conversion of
assertions from functions to methods.
* selftest.h (selftest::pretty_printer_test): New class.
(ASSERT_PP_FORMAT_1, ASSERT_PP_FORMAT_2, ASSERT_PP_FORMAT_3): Move
here from pretty-print.c, adding "test." to handle conversion from
functions to methods.
---
gcc/cp/cp-lang.c | 1 +
gcc/cp/cp-tree.h | 4 ++
gcc/cp/error.c | 147 +++++++++++++++++++++++++++++++++++++++++++++++++----
gcc/pretty-print.c | 129 +++++++++++++++++++++++-----------------------
gcc/selftest.h | 67 ++++++++++++++++++++++++
5 files changed, 272 insertions(+), 76 deletions(-)
diff --git a/gcc/cp/cp-lang.c b/gcc/cp/cp-lang.c
index 805319a..bc7ca63 100644
--- a/gcc/cp/cp-lang.c
+++ b/gcc/cp/cp-lang.c
@@ -247,6 +247,7 @@ run_cp_tests (void)
c_family_tests ();
/* Additional C++-specific tests. */
+ run_cp_error_c_tests ();
}
} // namespace selftest
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 126c24a..8dbdbeb 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7336,6 +7336,10 @@ extern void cp_ubsan_maybe_initialize_vtbl_ptrs (tree);
#if CHECKING_P
namespace selftest {
+ /* Declarations for specific families of tests in cp, by source file, in
+ alphabetical order. */
+ extern void run_cp_error_c_tests (void);
+
extern void run_cp_tests (void);
} // namespace selftest
#endif /* #if CHECKING_P */
diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index e53afa7..9553f4e 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -30,6 +30,7 @@ along with GCC; see the file COPYING3. If not see
#include "c-family/c-objc.h"
#include "ubsan.h"
#include "internal-fn.h"
+#include "selftest.h"
#define pp_separate_with_comma(PP) pp_cxx_separate_with (PP, ',')
#define pp_separate_with_semicolon(PP) pp_cxx_separate_with (PP, ';')
@@ -115,7 +116,8 @@ struct deferred_printed_type
: m_tree (type), m_buffer_ptr (buffer_ptr), m_verbose (verbose),
m_quote (quote)
{
- gcc_assert (type);
+ /* TYPE (and thus m_tree) can be NULL, or, at least, we should
+ gracefully handle this case. */
gcc_assert (buffer_ptr);
}
@@ -3611,6 +3613,11 @@ maybe_print_constexpr_context (diagnostic_context *context)
static bool
comparable_template_types_p (tree type_a, tree type_b)
{
+ /* If either is NULL, then treat them as incomparable, so that
+ we can gracefully fall back to type_to_string for them. */
+ if (type_a == NULL || type_b == NULL)
+ return false;
+
if (!CLASS_TYPE_P (type_a))
return false;
if (!CLASS_TYPE_P (type_b))
@@ -3892,8 +3899,9 @@ void
cxx_format_postprocessor::handle (pretty_printer *pp)
{
/* If we have one of %H and %I, the other should have
- been present. */
- if (m_type_a.m_tree || m_type_b.m_tree)
+ been present. Gracefully handle failure by falling
+ back to type_to_string if only one was present. */
+ if (m_type_a.m_buffer_ptr || m_type_b.m_buffer_ptr)
{
/* Avoid reentrancy issues by working with a copy of
m_type_a and m_type_b, resetting them now. */
@@ -3902,9 +3910,6 @@ cxx_format_postprocessor::handle (pretty_printer *pp)
m_type_a = deferred_printed_type ();
m_type_b = deferred_printed_type ();
- gcc_assert (type_a.m_buffer_ptr);
- gcc_assert (type_b.m_buffer_ptr);
-
bool show_color = pp_show_color (pp);
const char *type_a_text;
@@ -3930,19 +3935,22 @@ cxx_format_postprocessor::handle (pretty_printer *pp)
}
else
{
- /* If the types were not comparable, they are printed normally,
- and no difference tree is printed. */
+ /* If the types were not comparable (or if only one of %H/%I was
+ provided), they are printed normally, and no difference tree
+ is printed. */
type_a_text = type_to_string (type_a.m_tree, type_a.m_verbose);
type_b_text = type_to_string (type_b.m_tree, type_b.m_verbose);
}
if (type_a.m_quote)
type_a_text = add_quotes (type_a_text, show_color);
- *type_a.m_buffer_ptr = type_a_text;
+ if (type_a.m_buffer_ptr)
+ *type_a.m_buffer_ptr = type_a_text;
if (type_b.m_quote)
type_b_text = add_quotes (type_b_text, show_color);
- *type_b.m_buffer_ptr = type_b_text;
+ if (type_b.m_buffer_ptr)
+ *type_b.m_buffer_ptr = type_b_text;
}
}
@@ -4238,3 +4246,122 @@ qualified_name_lookup_error (tree scope, tree name,
suggest_alternatives_for (location, name, true);
}
}
+
+#if CHECKING_P
+
+namespace selftest {
+
+/* Subclass of pretty_printer_test for testing cp_printer. */
+
+class cp_pretty_printer_test : public pretty_printer_test
+{
+ public:
+ void set_up_printer (pretty_printer &pp) FINAL OVERRIDE
+ {
+ pp.format_decoder = cp_printer;
+ pp.m_format_postprocessor = new cxx_format_postprocessor ();
+ }
+};
+
+/* Verify one of the type-printing codes: %T, %H and %I (CODE),
+ reusing the fixture TEST. */
+
+static void
+test_type_printing (cp_pretty_printer_test &test, char code)
+{
+ /* Generate "%T %x" and %qT %x", where 'T' == code. */
+ char *unquoted = xasprintf ("%%%c %%x", code);
+ char *quoted = xasprintf ("%%q%c %%x", code);
+
+ /* Ensure they can print a type. */
+ ASSERT_PP_FORMAT_2 ("int 12345678", unquoted, integer_type_node, 0x12345678);
+ ASSERT_PP_FORMAT_2 ("`int' 12345678", quoted, integer_type_node, 0x12345678);
+
+ /* Ensure they gracefully handle NULL. */
+ ASSERT_PP_FORMAT_2 (" 12345678", unquoted, NULL, 0x12345678);
+ ASSERT_PP_FORMAT_2 ("`' 12345678", quoted, NULL, 0x12345678);
+
+ free (unquoted);
+ free (quoted);
+}
+
+/* Verify the behavior of cp_printer. */
+
+static void
+test_cp_printer (void)
+{
+ cp_pretty_printer_test test;
+
+ /* Verify various individual format codes, in the order listed in the
+ comment for cp_printer. For each code, we append a second
+ argument with a known bit pattern (0x12345678), to ensure that we
+ are consuming arguments correctly. */
+
+ // TODO: %A function argument-list.
+ // TODO: %C tree code.
+ // TODO: %D declaration.
+ // TODO: %E expression.
+ // TODO: %F function declaration.
+ // TODO: %L language as used in extern "lang".
+ // TODO: %O binary operator.
+ // TODO: %P function parameter whose position is indicated by an integer.
+ // TODO: %Q assignment operator.
+ // TODO: %S substitution (template + args)
+
+ /* Verify %T and %qT. */
+ test_type_printing (test, 'T');
+
+ // TODO: %V cv-qualifier.
+ // TODO: %X exception-specification.
+
+ /* Verify %H and %I type difference (from/to). */
+ /* Test of %H/%I by themselves (they should act like %T). */
+ test_type_printing (test, 'H');
+ test_type_printing (test, 'I');
+ /* Test of combinations of %H and %I: unquoted and quoted, in either
+ order, with non-NULL/NULL types. */
+ ASSERT_PP_FORMAT_3 ("int int 12345678", "%H %I %x",
+ integer_type_node, integer_type_node, 0x12345678);
+ ASSERT_PP_FORMAT_3 ("`int' `int' 12345678", "%qH %qI %x",
+ integer_type_node, integer_type_node, 0x12345678);
+ ASSERT_PP_FORMAT_3 ("int int 12345678", "%I %H %x",
+ integer_type_node, integer_type_node, 0x12345678);
+ ASSERT_PP_FORMAT_3 ("`int' `int' 12345678", "%qI %qH %x",
+ integer_type_node, integer_type_node, 0x12345678);
+ ASSERT_PP_FORMAT_3 (" int 12345678", "%H %I %x",
+ NULL, integer_type_node, 0x12345678);
+ ASSERT_PP_FORMAT_3 ("`' `int' 12345678", "%qH %qI %x",
+ NULL, integer_type_node, 0x12345678);
+ ASSERT_PP_FORMAT_3 (" int 12345678", "%I %H %x",
+ NULL, integer_type_node, 0x12345678);
+ ASSERT_PP_FORMAT_3 ("`' `int' 12345678", "%qI %qH %x",
+ NULL, integer_type_node, 0x12345678);
+ ASSERT_PP_FORMAT_3 ("int 12345678", "%H %I %x",
+ integer_type_node, NULL, 0x12345678);
+ ASSERT_PP_FORMAT_3 ("`int' `' 12345678", "%qH %qI %x",
+ integer_type_node, NULL, 0x12345678);
+ ASSERT_PP_FORMAT_3 ("int 12345678", "%I %H %x",
+ integer_type_node, NULL, 0x12345678);
+ ASSERT_PP_FORMAT_3 ("`int' `' 12345678", "%qI %qH %x",
+ integer_type_node, NULL, 0x12345678);
+ ASSERT_PP_FORMAT_3 (" 12345678", "%H %I %x",
+ NULL, NULL, 0x12345678);
+ ASSERT_PP_FORMAT_3 ("`' `' 12345678", "%qH %qI %x",
+ NULL, NULL, 0x12345678);
+ ASSERT_PP_FORMAT_3 (" 12345678", "%I %H %x",
+ NULL, NULL, 0x12345678);
+ ASSERT_PP_FORMAT_3 ("`' `' 12345678", "%qI %qH %x",
+ NULL, NULL, 0x12345678);
+}
+
+/* Run all of the selftests within this file. */
+
+void
+run_cp_error_c_tests (void)
+{
+ test_cp_printer ();
+}
+
+} // namespace selftest
+
+#endif /* #if CHECKING_P */
diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c
index 570dec7..89c5cd1 100644
--- a/gcc/pretty-print.c
+++ b/gcc/pretty-print.c
@@ -1302,36 +1302,34 @@ test_basic_printing ()
ASSERT_STREQ ("hello world", pp_formatted_text (&pp));
}
-/* Helper function for testing pp_format.
- Verify that pp_format (FMT, ...) followed by pp_output_formatted_text
- prints EXPECTED, assuming that pp_show_color is SHOW_COLOR. */
+/* class selftest::pretty_printer_test */
-static void
-assert_pp_format_va (const location &loc, const char *expected,
- bool show_color, const char *fmt, va_list *ap)
+/* Save the current values of open_quote and close_quote, and set them to
+ locale-independent values. */
+
+pretty_printer_test::pretty_printer_test ()
+: old_open_quote (open_quote),
+ old_close_quote (close_quote)
{
- pretty_printer pp;
- text_info ti;
- rich_location rich_loc (line_table, UNKNOWN_LOCATION);
+ open_quote = "`";
+ close_quote = "'";
+}
- ti.format_spec = fmt;
- ti.args_ptr = ap;
- ti.err_no = 0;
- ti.x_data = NULL;
- ti.m_richloc = &rich_loc;
+/* Restore the saved values of open_quote and close_quote. */
- pp_show_color (&pp) = show_color;
- pp_format (&pp, &ti);
- pp_output_formatted_text (&pp);
- ASSERT_STREQ_AT (loc, expected, pp_formatted_text (&pp));
+pretty_printer_test::~pretty_printer_test ()
+{
+ open_quote = old_open_quote;
+ close_quote = old_close_quote;
}
/* Verify that pp_format (FMT, ...) followed by pp_output_formatted_text
prints EXPECTED, with show_color disabled. */
-static void
-assert_pp_format (const location &loc, const char *expected,
- const char *fmt, ...)
+void
+pretty_printer_test::assert_pp_format (const location &loc,
+ const char *expected,
+ const char *fmt, ...)
{
va_list ap;
@@ -1342,9 +1340,10 @@ assert_pp_format (const location &loc, const char *expected,
/* As above, but with colorization enabled. */
-static void
-assert_pp_format_colored (const location &loc, const char *expected,
- const char *fmt, ...)
+void
+pretty_printer_test::assert_pp_format_colored (const location &loc,
+ const char *expected,
+ const char *fmt, ...)
{
/* The tests of colorization assume the default color scheme.
If GCC_COLORS is set, then the colors have potentially been
@@ -1359,28 +1358,33 @@ assert_pp_format_colored (const location &loc, const char *expected,
va_end (ap);
}
-/* Helper function for calling testing pp_format,
- by calling assert_pp_format with various numbers of arguments.
- These exist mostly to avoid having to write SELFTEST_LOCATION
- throughout test_pp_format. */
-
-#define ASSERT_PP_FORMAT_1(EXPECTED, FMT, ARG1) \
- SELFTEST_BEGIN_STMT \
- assert_pp_format ((SELFTEST_LOCATION), (EXPECTED), (FMT), \
- (ARG1)); \
- SELFTEST_END_STMT
-
-#define ASSERT_PP_FORMAT_2(EXPECTED, FMT, ARG1, ARG2) \
- SELFTEST_BEGIN_STMT \
- assert_pp_format ((SELFTEST_LOCATION), (EXPECTED), (FMT), \
- (ARG1), (ARG2)); \
- SELFTEST_END_STMT
-
-#define ASSERT_PP_FORMAT_3(EXPECTED, FMT, ARG1, ARG2, ARG3) \
- SELFTEST_BEGIN_STMT \
- assert_pp_format ((SELFTEST_LOCATION), (EXPECTED), (FMT), \
- (ARG1), (ARG2), (ARG3)); \
- SELFTEST_END_STMT
+/* Helper function for testing pp_format.
+ Verify that pp_format (FMT, ...) followed by pp_output_formatted_text
+ prints EXPECTED, assuming that pp_show_color is SHOW_COLOR. */
+
+void
+pretty_printer_test::assert_pp_format_va (const location &loc,
+ const char *expected,
+ bool show_color, const char *fmt,
+ va_list *ap)
+{
+ pretty_printer pp;
+
+ text_info ti;
+ rich_location rich_loc (line_table, UNKNOWN_LOCATION);
+
+ ti.format_spec = fmt;
+ ti.args_ptr = ap;
+ ti.err_no = 0;
+ ti.x_data = NULL;
+ ti.m_richloc = &rich_loc;
+
+ set_up_printer (pp);
+ pp_show_color (&pp) = show_color;
+ pp_format (&pp, &ti);
+ pp_output_formatted_text (&pp);
+ ASSERT_STREQ_AT (loc, expected, pp_formatted_text (&pp));
+}
/* Verify that pp_format works, for various format codes. */
@@ -1389,13 +1393,10 @@ test_pp_format ()
{
/* Avoid introducing locale-specific differences in the results
by hardcoding open_quote and close_quote. */
- const char *old_open_quote = open_quote;
- const char *old_close_quote = close_quote;
- open_quote = "`";
- close_quote = "'";
+ pretty_printer_test test;
/* Verify that plain text is passed through unchanged. */
- assert_pp_format (SELFTEST_LOCATION, "unformatted", "unformatted");
+ test.assert_pp_format (SELFTEST_LOCATION, "unformatted", "unformatted");
/* Verify various individual format codes, in the order listed in the
comment for pp_format above. For each code, we append a second
@@ -1433,7 +1434,7 @@ test_pp_format ()
ASSERT_PP_FORMAT_2 ("normal colored normal 12345678",
"normal %rcolored%R normal %x",
"error", 0x12345678);
- assert_pp_format_colored
+ test.assert_pp_format_colored
(SELFTEST_LOCATION,
"normal \33[01;31m\33[Kcolored\33[m\33[K normal 12345678",
"normal %rcolored%R normal %x", "error", 0x12345678);
@@ -1448,9 +1449,9 @@ test_pp_format ()
/* Verify flag 'q'. */
ASSERT_PP_FORMAT_2 ("`foo' 12345678", "%qs %x", "foo", 0x12345678);
- assert_pp_format_colored (SELFTEST_LOCATION,
- "`\33[01m\33[Kfoo\33[m\33[K' 12345678", "%qs %x",
- "foo", 0x12345678);
+ test.assert_pp_format_colored (SELFTEST_LOCATION,
+ "`\33[01m\33[Kfoo\33[m\33[K' 12345678", "%qs %x",
+ "foo", 0x12345678);
/* Verify %Z. */
int v[] = { 1, 2, 3 };
@@ -1460,17 +1461,13 @@ test_pp_format ()
ASSERT_PP_FORMAT_3 ("0 12345678", "%Z %x", v2, 1, 0x12345678);
/* Verify that combinations work, along with unformatted text. */
- assert_pp_format (SELFTEST_LOCATION,
- "the quick brown fox jumps over the lazy dog",
- "the %s %s %s jumps over the %s %s",
- "quick", "brown", "fox", "lazy", "dog");
- assert_pp_format (SELFTEST_LOCATION, "item 3 of 7", "item %i of %i", 3, 7);
- assert_pp_format (SELFTEST_LOCATION, "problem with `bar' at line 10",
- "problem with %qs at line %i", "bar", 10);
-
- /* Restore old values of open_quote and close_quote. */
- open_quote = old_open_quote;
- close_quote = old_close_quote;
+ test.assert_pp_format (SELFTEST_LOCATION,
+ "the quick brown fox jumps over the lazy dog",
+ "the %s %s %s jumps over the %s %s",
+ "quick", "brown", "fox", "lazy", "dog");
+ ASSERT_PP_FORMAT_2 ("item 3 of 7", "item %i of %i", 3, 7);
+ ASSERT_PP_FORMAT_2 ("problem with `bar' at line 10",
+ "problem with %qs at line %i", "bar", 10);
}
/* Run all of the selftests within this file. */
diff --git a/gcc/selftest.h b/gcc/selftest.h
index 0572fef..96eccac 100644
--- a/gcc/selftest.h
+++ b/gcc/selftest.h
@@ -322,6 +322,73 @@ extern int num_passes;
#define SELFTEST_BEGIN_STMT do {
#define SELFTEST_END_STMT } while (0)
+
+/* pretty-print.c. */
+
+namespace selftest {
+
+/* Fixture for testing pretty-printing.
+ Avoids locale-specific differences in the results of prettyprinting tests
+ by temporarily overriding open_quote and close_quote (via ctor/dtor). */
+
+class pretty_printer_test
+{
+ public:
+ pretty_printer_test ();
+ ~pretty_printer_test ();
+
+ /* Hook for allowing subclasses to manipulate the underlying printer. */
+ virtual void set_up_printer (pretty_printer &) {}
+
+ /* Verify that pp_format (FMT, ...) followed by pp_output_formatted_text
+ prints EXPECTED, with show_color disabled. */
+
+ void assert_pp_format (const location &loc, const char *expected,
+ const char *fmt, ...);
+
+ /* As above, but with colorization enabled. */
+
+ void assert_pp_format_colored (const location &loc, const char *expected,
+ const char *fmt, ...);
+
+ /* Helper function for the above. */
+
+ void assert_pp_format_va (const location &loc, const char *expected,
+ bool show_color, const char *fmt,
+ va_list *ap);
+
+ private:
+ const char *old_open_quote;
+ const char *old_close_quote;
+};
+
+} // namespace selftest
+
+/* Helper function for calling testing pp_format, assuming an instance
+ of pretty_printer_test named "test", by calling test.assert_pp_format
+ with various numbers of arguments.
+ These exist mostly to avoid having to write SELFTEST_LOCATION
+ and "test" throughout test cases. */
+
+#define ASSERT_PP_FORMAT_1(EXPECTED, FMT, ARG1) \
+ SELFTEST_BEGIN_STMT \
+ test.assert_pp_format ((SELFTEST_LOCATION), (EXPECTED), (FMT), \
+ (ARG1)); \
+ SELFTEST_END_STMT
+
+#define ASSERT_PP_FORMAT_2(EXPECTED, FMT, ARG1, ARG2) \
+ SELFTEST_BEGIN_STMT \
+ test.assert_pp_format ((SELFTEST_LOCATION), (EXPECTED), (FMT), \
+ (ARG1), (ARG2)); \
+ SELFTEST_END_STMT
+
+#define ASSERT_PP_FORMAT_3(EXPECTED, FMT, ARG1, ARG2, ARG3) \
+ SELFTEST_BEGIN_STMT \
+ test.assert_pp_format ((SELFTEST_LOCATION), (EXPECTED), (FMT), \
+ (ARG1), (ARG2), (ARG3)); \
+ SELFTEST_END_STMT
+
+
#endif /* #if CHECKING_P */
#endif /* GCC_SELFTEST_H */
--
1.8.5.3
More information about the Gcc-patches
mailing list