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

[David Edelsohn <dje.gcc@gmail.com>]

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