| Message ID | 1465494836.4029.30.camel@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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 ();