Re: [PATCH] Add selftest for pretty-print.c (v2)

Re: [PATCH] Add selftest for pretty-print.c (v2)

[David Edelsohn]

This caused a bootstrap failure on AIX, and possible other architectures.

/tmp/20160608/./gcc/xgcc -B/tmp/20160608/./gcc/ -xc -S -c /dev/null -fself-test
/nasfarm/edelsohn/src/src/gcc/pretty-print.c:1246: FAIL: ASSERT_STREQ (expected,
 pp_formatted_text (&pp))
cc1: internal compiler error: in fail, at selftest.c:44

This is a completely unacceptable way to introduce these self-tests.
Please stop adding self-tests that only are tested on x86 Linux and
cause bootstrap failures.

Thanks, David

Re: [PATCH] Add selftest for pretty-print.c (v2)

[Bernd Schmidt]

On 06/09/2016 02:21 PM, David Edelsohn wrote:

> This is a completely unacceptable way to introduce these self-tests.
> Please stop adding self-tests that only are tested on x86 Linux and
> cause bootstrap failures.

We have no requirement to test patches on more than one target. I think 
your request is unreasonable.

Have you at least investigated whether the self-test exposes a real bug 
or not?


Bernd

Re: [PATCH] Add selftest for pretty-print.c (v2)

[David Edelsohn]

On Thu, Jun 9, 2016 at 8:48 AM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 06/09/2016 02:21 PM, David Edelsohn wrote:
>
>> This is a completely unacceptable way to introduce these self-tests.
>> Please stop adding self-tests that only are tested on x86 Linux and
>> cause bootstrap failures.
>
>
> We have no requirement to test patches on more than one target. I think your
> request is unreasonable.

Bernd,

This is a completely inappropriate response.  GCC must maintain a
stable, working development base on which developers can work.  GCC
specifically supports multiple architectures and targets.

GCC is not your personal kingdom and playground.

Thanks, David

>
> Have you at least investigated whether the self-test exposes a real bug or
> not?
>
>
> Bernd

Re: [PATCH] Add selftest for pretty-print.c (v2)

[Jakub Jelinek]

On Thu, Jun 09, 2016 at 09:02:27AM -0400, David Edelsohn wrote:
> On Thu, Jun 9, 2016 at 8:48 AM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> > On 06/09/2016 02:21 PM, David Edelsohn wrote:
> >
> >> This is a completely unacceptable way to introduce these self-tests.
> >> Please stop adding self-tests that only are tested on x86 Linux and
> >> cause bootstrap failures.
> >
> >
> > We have no requirement to test patches on more than one target. I think your
> > request is unreasonable.
> 
> Bernd,
> 
> This is a completely inappropriate response.  GCC must maintain a
> stable, working development base on which developers can work.  GCC
> specifically supports multiple architectures and targets.
> 
> GCC is not your personal kingdom and playground.

For patches that the submitter can easily expect problems on some
architectures, testing there is desirable, but we certainly don't and should
not require every single generic code change to be expected on all supported
targets, not even on primary targets.  That just doesn't scale, various
people submit multiple changes a day and having to wait days or weeks before
testing is over is unacceptable.

You've reported a problem, David is going to look at it and fix.
I don't see why it would be unacceptable to add further self-tests, as long
as there is nothing in them where one should expect issues on targets other
than the tested one.

	Jakub

Re: [PATCH] Add selftest for pretty-print.c (v2)

[David Edelsohn]

On Thu, Jun 9, 2016 at 9:10 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Jun 09, 2016 at 09:02:27AM -0400, David Edelsohn wrote:
>> On Thu, Jun 9, 2016 at 8:48 AM, Bernd Schmidt <bschmidt@redhat.com> wrote:
>> > On 06/09/2016 02:21 PM, David Edelsohn wrote:
>> >
>> >> This is a completely unacceptable way to introduce these self-tests.
>> >> Please stop adding self-tests that only are tested on x86 Linux and
>> >> cause bootstrap failures.
>> >
>> >
>> > We have no requirement to test patches on more than one target. I think your
>> > request is unreasonable.
>>
>> Bernd,
>>
>> This is a completely inappropriate response.  GCC must maintain a
>> stable, working development base on which developers can work.  GCC
>> specifically supports multiple architectures and targets.
>>
>> GCC is not your personal kingdom and playground.
>
> For patches that the submitter can easily expect problems on some
> architectures, testing there is desirable, but we certainly don't and should
> not require every single generic code change to be expected on all supported
> targets, not even on primary targets.  That just doesn't scale, various
> people submit multiple changes a day and having to wait days or weeks before
> testing is over is unacceptable.
>
> You've reported a problem, David is going to look at it and fix.
> I don't see why it would be unacceptable to add further self-tests, as long
> as there is nothing in them where one should expect issues on targets other
> than the tested one.

I didn't request that every patch be tested on every architecture.
Please don't play a rhetorical game of arguing against the strawman
that you created.

The self-tests specifically abort the build and break bootstrap upon
failure.  Most other changes that inadvertently have bugs or tickle a
latent issue in a target will introduce some additional testsuite
failures, not a bootstrap failure.  x86 developers seem to get quite
annoyed when a patch causes a bootstrap failure for an x86
configuration.

Second, all of the large changes that may have unknown effects on
various targets have been tested extensively on multiple
architectures, as have most global optimization changes.  It may not
be required, but it generally has been considered "good form" and has
been a stipulation of patch approval by some reviewers.  It would be
very unfortunate for GCC to lower the bar for patches by some
developers and not others.

Thanks, David

[PATCH] PR bootstrap/71471: remove selftest for pp_format (%p)

[David Malcolm]

I was confused by the comment to pp_format:

  /* The following format specifiers are recognized as being client independent:
     ...
     %p: pointer
     ... */

into thinking that %p is printed in a host-independent manner, when
"client independent" means in relation to different pretty-printers e.g the
C++ pretty printer, or normal tree one etc.

The following patch removes the overzealous test case and clarifies
the comment.

OK for trunk?

I'm working on a followup that makes selftest failures easier to
track down.

gcc/ChangeLog:
	PR bootstrap/71471
	* pretty-print.c (pp_indent): Specify that %p is printed in a
	host-dependent manner.
	(test_pp_format): Remove the test for %p.
---
 gcc/pretty-print.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c
index d805da4..8febda0 100644
--- a/gcc/pretty-print.c
+++ b/gcc/pretty-print.c
@@ -279,7 +279,7 @@ pp_indent (pretty_printer *pp)
    %wd, %wi, %wo, %wu, %wx: HOST_WIDE_INT versions.
    %c: character.
    %s: string.
-   %p: pointer.
+   %p: pointer (printed in a host-dependent manner).
    %r: if pp_show_color(pp), switch to color identified by const char *.
    %R: if pp_show_color(pp), reset color.
    %m: strerror(text->err_no) - does not consume a value from args_ptr.
@@ -1317,8 +1317,8 @@ test_pp_format ()
   assert_pp_format ("A 12345678", "%c %x", 'A', 0x12345678);
   assert_pp_format ("hello world 12345678", "%s %x", "hello world",
 		    0x12345678);
-  assert_pp_format ("0xcafebabe 12345678", "%p %x", (void *)0xcafebabe,
-		    0x12345678);
+  /* We can't test for %p; the pointer is printed in an implementation-defined
+     manner.  */
   assert_pp_format ("normal colored normal 12345678",
 		    "normal %rcolored%R normal %x",
 		    "error", 0x12345678);
-- 
1.8.5.3

Re: [PATCH] PR bootstrap/71471: remove selftest for pp_format (%p)

[Jeff Law]

On 06/09/2016 10:45 AM, David Malcolm wrote:
> I was confused by the comment to pp_format:
>
>   /* The following format specifiers are recognized as being client independent:
>      ...
>      %p: pointer
>      ... */
>
> into thinking that %p is printed in a host-independent manner, when
> "client independent" means in relation to different pretty-printers e.g the
> C++ pretty printer, or normal tree one etc.
>
> The following patch removes the overzealous test case and clarifies
> the comment.
>
> OK for trunk?
>
> I'm working on a followup that makes selftest failures easier to
> track down.
>
> gcc/ChangeLog:
> 	PR bootstrap/71471
> 	* pretty-print.c (pp_indent): Specify that %p is printed in a
> 	host-dependent manner.
> 	(test_pp_format): Remove the test for %p.
OK.
jeff

Re: [PATCH] Add selftest for pretty-print.c (v2)

[Jeff Law]

On 06/09/2016 07:30 AM, David Edelsohn wrote:
>
> The self-tests specifically abort the build and break bootstrap upon
> failure.  Most other changes that inadvertently have bugs or tickle a
> latent issue in a target will introduce some additional testsuite
> failures, not a bootstrap failure.  x86 developers seem to get quite
> annoyed when a patch causes a bootstrap failure for an x86
> configuration.
>
> Second, all of the large changes that may have unknown effects on
> various targets have been tested extensively on multiple
> architectures, as have most global optimization changes.  It may not
> be required, but it generally has been considered "good form" and has
> been a stipulation of patch approval by some reviewers.  It would be
> very unfortunate for GCC to lower the bar for patches by some
> developers and not others.
Let's all calm down a bit here.  Everyone here just wants to make a 
better compiler and mistakes happen.

What I see in David Malcolm's change is a fairly minor bug.  I don't 
think David (or anyone) could have really expected that %p is printed 
differently across different hosts and thus his patch would need wider 
host testing.  And AFAICT David addressed this issue as soon as he 
started his day.

So let's all take a deep breath and get back to improving GCC rather 
than taking jabs at each other.

jeff

Re: [PATCH] Add selftest for pretty-print.c (v2)

[David Malcolm]

On Thu, 2016-06-09 at 11:22 -0600, Jeff Law wrote:
> On 06/09/2016 07:30 AM, David Edelsohn wrote:
> > 
> > The self-tests specifically abort the build and break bootstrap
> > upon
> > failure.  Most other changes that inadvertently have bugs or tickle
> > a
> > latent issue in a target will introduce some additional testsuite
> > failures, not a bootstrap failure.  x86 developers seem to get
> > quite
> > annoyed when a patch causes a bootstrap failure for an x86
> > configuration.
> > 
> > Second, all of the large changes that may have unknown effects on
> > various targets have been tested extensively on multiple
> > architectures, as have most global optimization changes.  It may
> > not
> > be required, but it generally has been considered "good form" and
> > has
> > been a stipulation of patch approval by some reviewers.  It would
> > be
> > very unfortunate for GCC to lower the bar for patches by some
> > developers and not others.
> Let's all calm down a bit here.  Everyone here just wants to make a 
> better compiler and mistakes happen.

> What I see in David Malcolm's change is a fairly minor bug.  I don't 
> think David (or anyone) could have really expected that %p is printed
> differently across different hosts and thus his patch would need
> wider 
> host testing.  And AFAICT David addressed this issue as soon as he 
> started his day.
> 
> So let's all take a deep breath and get back to improving GCC rather 
> than taking jabs at each other.
> 

Sorry about the breakage.  I've committed a fix as r237271, which I've
tested on PPC AIX (and on x86_64 linux).

The selftest code is very new.  I tested both it and the pretty-print.c
tests for every known-good *target* in config-list.mk; the issue here
was a *host*-specific issue.

Maybe the current "fail the build on any selftest failures" is too
aggressive.  That said, note that if one knows which file the failing
test is in (which we did), it's trivial to disable the tests in that
file by hacking gcc/selftests-run-tests.c and commenting out/deleting
the call:

diff --git a/gcc/selftest-run-tests.c b/gcc/selftest-run-tests.c
index 934e700..1c8128b 100644
--- a/gcc/selftest-run-tests.c
+++ b/gcc/selftest-run-tests.c
@@ -46,7 +46,7 @@ selftest::run_tests ()
   hash_map_tests_c_tests ();
   hash_set_tests_c_tests ();
   vec_c_tests ();
-  pretty_print_c_tests ();
+  //pretty_print_c_tests ();
   wide_int_cc_tests ();
 

whilst the underlying failure is investigated, so adding a new selftest
is presumably not as risky an event as, say, changing an optimizer: the
change is localized and can be readily disabled if it turns out to have
a config-specific assumption.

The selftests currently in trunk aren't the most exciting; I'm much
more interested in the ggc-tests.c patch (awaiting review), since this
would finally give us self-testing of gengtype and ggc, which AFAIK we
haven't been able to test directly before.  I hate gengtype, and it's
been a goal of mine to try to tame it since I started working on gcc.
(FWIW I've successfully tested the ggc patch on AIX PPC now, for stage
1 at least, and for all targets in config-list.mk).

Sorry again about the breakage.

Dave

Re: [PATCH] Add selftest for pretty-print.c (v2)

[David Edelsohn]

On Thu, Jun 9, 2016 at 1:53 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Thu, 2016-06-09 at 11:22 -0600, Jeff Law wrote:
>> On 06/09/2016 07:30 AM, David Edelsohn wrote:
>> >
>> > The self-tests specifically abort the build and break bootstrap
>> > upon
>> > failure.  Most other changes that inadvertently have bugs or tickle
>> > a
>> > latent issue in a target will introduce some additional testsuite
>> > failures, not a bootstrap failure.  x86 developers seem to get
>> > quite
>> > annoyed when a patch causes a bootstrap failure for an x86
>> > configuration.
>> >
>> > Second, all of the large changes that may have unknown effects on
>> > various targets have been tested extensively on multiple
>> > architectures, as have most global optimization changes.  It may
>> > not
>> > be required, but it generally has been considered "good form" and
>> > has
>> > been a stipulation of patch approval by some reviewers.  It would
>> > be
>> > very unfortunate for GCC to lower the bar for patches by some
>> > developers and not others.
>> Let's all calm down a bit here.  Everyone here just wants to make a
>> better compiler and mistakes happen.
>
>> What I see in David Malcolm's change is a fairly minor bug.  I don't
>> think David (or anyone) could have really expected that %p is printed
>> differently across different hosts and thus his patch would need
>> wider
>> host testing.  And AFAICT David addressed this issue as soon as he
>> started his day.
>>
>> So let's all take a deep breath and get back to improving GCC rather
>> than taking jabs at each other.
>>
>
> Sorry about the breakage.  I've committed a fix as r237271, which I've
> tested on PPC AIX (and on x86_64 linux).
>
> The selftest code is very new.  I tested both it and the pretty-print.c
> tests for every known-good *target* in config-list.mk; the issue here
> was a *host*-specific issue.
>
> Maybe the current "fail the build on any selftest failures" is too
> aggressive.  That said, note that if one knows which file the failing
> test is in (which we did), it's trivial to disable the tests in that
> file by hacking gcc/selftests-run-tests.c and commenting out/deleting
> the call:
>
> diff --git a/gcc/selftest-run-tests.c b/gcc/selftest-run-tests.c
> index 934e700..1c8128b 100644
> --- a/gcc/selftest-run-tests.c
> +++ b/gcc/selftest-run-tests.c
> @@ -46,7 +46,7 @@ selftest::run_tests ()
>    hash_map_tests_c_tests ();
>    hash_set_tests_c_tests ();
>    vec_c_tests ();
> -  pretty_print_c_tests ();
> +  //pretty_print_c_tests ();
>    wide_int_cc_tests ();
>
>
> whilst the underlying failure is investigated, so adding a new selftest
> is presumably not as risky an event as, say, changing an optimizer: the
> change is localized and can be readily disabled if it turns out to have
> a config-specific assumption.
>
> The selftests currently in trunk aren't the most exciting; I'm much
> more interested in the ggc-tests.c patch (awaiting review), since this
> would finally give us self-testing of gengtype and ggc, which AFAIK we
> haven't been able to test directly before.  I hate gengtype, and it's
> been a goal of mine to try to tame it since I started working on gcc.
> (FWIW I've successfully tested the ggc patch on AIX PPC now, for stage
> 1 at least, and for all targets in config-list.mk).
>
> Sorry again about the breakage.

Thanks for fixing this so quickly.

Maybe we need to consider some sort of "warn on failure" beta testing
period for new self-tests before they cause errors.  If self-tests can
trigger host-dependent behavior and cause bootstrap failure as a
consequence, we need to think about how this interacts with other GCC
development policies.

Thanks, David

Re: [PATCH] Add selftest for pretty-print.c (v2)

[Bernd Schmidt]

On 06/08/2016 02:56 AM, David Malcolm wrote:
> Good idea.  In the following I did it by adding 0x12345678 as a
> successor argument to each test.  I chose that bit pattern on the
> grounds that each nybble is unique and non-zero.
> I printed them with %x to make it easier (I hope) to track down
> problems.
>
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
>
> OK for trunk?

Sure, that was implied by "otherwise OK".


Bernd

[PATCH] Add selftest for pretty-print.c (v2)

[David Malcolm]

On Tue, 2016-06-07 at 12:02 +0200, Bernd Schmidt wrote:
> On 06/06/2016 11:28 PM, David Malcolm wrote:
> > +  assert_pp_format ("0xcafebabe", "%wx",
> > (HOST_WIDE_INT)0xcafebabe);
> 
> More interesting tests would be to have multiple arguments to test
> that
> we really used the right size for the varargs. Maybe append a single
> %d
> arg with a unique bit pattern to all of them?
> 
> Otherwise ok.

Good idea.  In the following I did it by adding 0x12345678 as a
successor argument to each test.  I chose that bit pattern on the
grounds that each nybble is unique and non-zero.
I printed them with %x to make it easier (I hope) to track down
problems.

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

OK for trunk?

gcc/ChangeLog:
	* pretty-print.c: Include "selftest.h".
	(pp_format): Fix comment.
	(identifier_to_locale): Likewise.
	(selftest::test_basic_printing): New function.
	(selftest::assert_pp_format): New function.
	(selftest::test_pp_format): New function.
	(selftest::pretty_print_c_tests): New function.
	* selftest-run-tests.c (selftest::run_tests): Call
	selftest::pretty_print_c_tests.
	* selftest.h (pretty_print_c_tests): New declaration.
---
 gcc/pretty-print.c       | 164 ++++++++++++++++++++++++++++++++++++++++++++++-
 gcc/selftest-run-tests.c |   1 +
 gcc/selftest.h           |   1 +
 3 files changed, 165 insertions(+), 1 deletion(-)

diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c
index cc2b8cc..d805da4 100644
--- a/gcc/pretty-print.c
+++ b/gcc/pretty-print.c
@@ -24,6 +24,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "intl.h"
 #include "pretty-print.h"
 #include "diagnostic-color.h"
+#include "selftest.h"
 
 #if HAVE_ICONV
 #include <iconv.h>
@@ -304,7 +305,7 @@ pp_indent (pretty_printer *pp)
 
 /* Formatting phases 1 and 2: render TEXT->format_spec plus
    TEXT->args_ptr into a series of chunks in pp_buffer (PP)->args[].
-   Phase 3 is in pp_format_text.  */
+   Phase 3 is in pp_output_formatted_text.  */
 
 void
 pp_format (pretty_printer *pp, text_info *text)
@@ -1203,3 +1204,164 @@ identifier_to_locale (const char *ident)
     return ret;
   }
 }
+
+#if CHECKING_P
+
+namespace selftest {
+
+/* Smoketest for pretty_printer.  */
+
+static void
+test_basic_printing ()
+{
+  pretty_printer pp;
+  pp_string (&pp, "hello");
+  pp_space (&pp);
+  pp_string (&pp, "world");
+
+  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.  */
+
+static void
+assert_pp_format_va (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;
+
+  pp_show_color (&pp) = show_color;
+  pp_format (&pp, &ti);
+  pp_output_formatted_text (&pp);
+  ASSERT_STREQ (expected, pp_formatted_text (&pp));
+}
+
+/* Verify that pp_format (FMT, ...) followed by pp_output_formatted_text
+   prints EXPECTED, with show_color disabled.  */
+
+static void
+assert_pp_format (const char *expected, const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start (ap, fmt);
+  assert_pp_format_va (expected, false, fmt, &ap);
+  va_end (ap);
+}
+
+/* As above, but with colorization enabled.  */
+
+static void
+assert_pp_format_colored (const char *expected, const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start (ap, fmt);
+  assert_pp_format_va (expected, true, fmt, &ap);
+  va_end (ap);
+}
+
+/* Verify that pp_format works, for various format codes.  */
+
+static void
+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 = "'";
+
+  /* Verify that plain text is passed through unchanged.  */
+  assert_pp_format ("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
+     argument with a known bit pattern (0x12345678), to ensure that we
+     are consuming arguments correctly.  */
+  assert_pp_format ("-27 12345678", "%d %x", -27, 0x12345678);
+  assert_pp_format ("-5 12345678", "%i %x", -5, 0x12345678);
+  assert_pp_format ("10 12345678", "%u %x", 10, 0x12345678);
+  assert_pp_format ("17 12345678", "%o %x", 15, 0x12345678);
+  assert_pp_format ("cafebabe 12345678", "%x %x", 0xcafebabe, 0x12345678);
+  assert_pp_format ("-27 12345678", "%ld %x", (long)-27, 0x12345678);
+  assert_pp_format ("-5 12345678", "%li %x", (long)-5, 0x12345678);
+  assert_pp_format ("10 12345678", "%lu %x", (long)10, 0x12345678);
+  assert_pp_format ("17 12345678", "%lo %x", (long)15, 0x12345678);
+  assert_pp_format ("cafebabe 12345678", "%lx %x", (long)0xcafebabe,
+		    0x12345678);
+  assert_pp_format ("-27 12345678", "%lld %x", (long long)-27, 0x12345678);
+  assert_pp_format ("-5 12345678", "%lli %x", (long long)-5, 0x12345678);
+  assert_pp_format ("10 12345678", "%llu %x", (long long)10, 0x12345678);
+  assert_pp_format ("17 12345678", "%llo %x", (long long)15, 0x12345678);
+  assert_pp_format ("cafebabe 12345678", "%llx %x", (long long)0xcafebabe,
+		    0x12345678);
+  assert_pp_format ("-27 12345678", "%wd %x", (HOST_WIDE_INT)-27, 0x12345678);
+  assert_pp_format ("-5 12345678", "%wi %x", (HOST_WIDE_INT)-5, 0x12345678);
+  assert_pp_format ("10 12345678", "%wu %x", (unsigned HOST_WIDE_INT)10,
+		    0x12345678);
+  assert_pp_format ("17 12345678", "%wo %x", (HOST_WIDE_INT)15, 0x12345678);
+  assert_pp_format ("0xcafebabe 12345678", "%wx %x", (HOST_WIDE_INT)0xcafebabe,
+		    0x12345678);
+  assert_pp_format ("A 12345678", "%c %x", 'A', 0x12345678);
+  assert_pp_format ("hello world 12345678", "%s %x", "hello world",
+		    0x12345678);
+  assert_pp_format ("0xcafebabe 12345678", "%p %x", (void *)0xcafebabe,
+		    0x12345678);
+  assert_pp_format ("normal colored normal 12345678",
+		    "normal %rcolored%R normal %x",
+		    "error", 0x12345678);
+  /* The following assumes an empty value for GCC_COLORS.  */
+  assert_pp_format_colored
+    ("normal \33[01;31m\33[Kcolored\33[m\33[K normal 12345678",
+     "normal %rcolored%R normal %x", "error", 0x12345678);
+  /* TODO:
+     %m: strerror(text->err_no) - does not consume a value from args_ptr.  */
+  assert_pp_format ("% 12345678", "%% %x", 0x12345678);
+  assert_pp_format ("` 12345678", "%< %x", 0x12345678);
+  assert_pp_format ("' 12345678", "%> %x", 0x12345678);
+  assert_pp_format ("' 12345678", "%' %x", 0x12345678);
+  assert_pp_format ("abc 12345678", "%.*s %x", 3, "abcdef", 0x12345678);
+  assert_pp_format ("abc 12345678", "%.3s %x", "abcdef", 0x12345678);
+
+  /* Verify flag 'q'.  */
+  assert_pp_format ("`foo' 12345678", "%qs %x", "foo", 0x12345678);
+  assert_pp_format_colored ("`\33[01m\33[Kfoo\33[m\33[K' 12345678", "%qs %x",
+			    "foo", 0x12345678);
+
+  /* Verify that combinations work, along with unformatted text.  */
+  assert_pp_format ("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 ("item 3 of 7", "item %i of %i", 3, 7);
+  assert_pp_format ("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;
+}
+
+/* Run all of the selftests within this file.  */
+
+void
+pretty_print_c_tests ()
+{
+  test_basic_printing ();
+  test_pp_format ();
+}
+
+} // namespace selftest
+
+#endif /* CHECKING_P */
diff --git a/gcc/selftest-run-tests.c b/gcc/selftest-run-tests.c
index 502eb4b..934e700 100644
--- a/gcc/selftest-run-tests.c
+++ b/gcc/selftest-run-tests.c
@@ -46,6 +46,7 @@ selftest::run_tests ()
   hash_map_tests_c_tests ();
   hash_set_tests_c_tests ();
   vec_c_tests ();
+  pretty_print_c_tests ();
   wide_int_cc_tests ();
   ggc_tests_c_tests ();
 
diff --git a/gcc/selftest.h b/gcc/selftest.h
index 1c89842..d1f8acc 100644
--- a/gcc/selftest.h
+++ b/gcc/selftest.h
@@ -51,6 +51,7 @@ extern void ggc_tests_c_tests ();
 extern void hash_map_tests_c_tests ();
 extern void hash_set_tests_c_tests ();
 extern void input_c_tests ();
+extern void pretty_print_c_tests ();
 extern void rtl_tests_c_tests ();
 extern void spellcheck_c_tests ();
 extern void tree_c_tests ();
-- 
1.8.5.3