On Wed, Jul 29, 2020 at 04:37:03PM -0400, Jason Merrill wrote:
> On Tue, Jul 28, 2020 at 5:45 PM Marek Polacek via Gcc-patches <
> gcc-patches@gcc.gnu.org> wrote:
>
> > In Bugzilla, for the c++ component, we currently have over 3200 open
> > bugs. In
> > my experience, a good amount of them have already been fixed; my periodical
> > sweeps always turn up a bunch of PRs that had already been fixed
> > previously.
> > Sometimes my sweeps are more or less random, but more often than not I'm
> > just
> > looking for duplicates of an existing PR. Sometimes the reason the already
> > fixed PRs are still open is because a PR that was fixed had duplicates
> > that we
> > didn't catch earlier when confirming the PR. Sometimes a PR gets fixed as
> > a
> > side-effect of fixing another PR. Manual sweeps are tedious and
> > time-consuming
> > because often you need to grab the test from the Bugzilla yet again (and
> > sometimes there are multiple tests). Even if you find a PR that was
> > fixed, you
> > still need to bisect the fix and perhaps add the test to our testsuite.
> > That's
> > draining and since the number of bugs only increases, never decreases, it
> > is not
> > sustainable.
> >
> > So I've started a personal repo where I've gathered dozens of tests and
> > wrote a
> > script that just compiles every test in the repo and reports if anything
> > changed. One line from it:
> >
> > pr=59798; $cxx $o -c $pr.C 2>&1 | grep -qE 'internal compiler error' ||
> > echo -e "$pr: ${msg_ice}"
> >
> > This has major drawbacks: you have to remember to run this manually, keep
> > updating it, and it's yet another repo that people interested in this would
> > have to clone, but the worst thing is that typically you would only
> > discover
> > that a patch fixed a different PR long after the patch was committed. And
> > quite likely it wasn't even your patch. We know that finding problems
> > earlier
> > in the developer workflow reduces costs; if we can catch this before the
> > original developer commits & pushes the changes, it's cheaper, because the
> > developer already understands what the patch does.
> >
> > A case in point: https://gcc.gnu.org/PR58156 which has been fixed recently
> > by an unrelated (?) patch. Knowing that the tsubst_pack_expansion hunk in
> > the patch had this effect would probably have been very useful. More
> > testing
> > will lead to a better compiler.
> >
> > Another case: https://gcc.gnu.org/35098 which was fixed 12 years (!) after
> > it was reported by a different change.
> >
> > Or another: https://gcc.gnu.org/91525 where the patch contained a test,
> > but
> > that was ice-on-invalid, whereas the test in PR91525 was ice-on-valid.
> >
> > To alleviate some of these problems, I propose that we introduce a means
> > to our
> > DejaGNU infrastructure that allows adding tests for old bugs that have not
> > been
> > fixed yet, and re-introduce the keyword monitored (no longer used for
> > anything
> > -- I think Volker stepped away) to the GCC Bugzilla to signal that a PR is
> > tracked in the testsuite. I don't want any unnecessary moving tests
> > around, so
> > the tests would go where they would normally go; they have to be reduced
> > and
> > have proper targets, etc. Having such tests in the testsuite means that
> > when
> > something changes, you will know immediately, before you push any changes.
> >
> > My thinking is that for:
> >
> > * rejects-valid: use the existing dg-xfail-if
> >
>
> Or dg-excess-errors, or xfailed dg-bogus.
Yeah, whatever works. With e.g.
int i = ...; // { dg-xfail-if "" { *-*-* } }
one gets expected failures when we emit an error, and unexpected successes when
we stop emitting and error there.
> > * accepts-valid: use the new dg-accepts-invalid
(Surely I meant accepts-*in*valid here.)
> >
>
> xfailed dg-error should cover this case.
That works well if you know where to expect an error. But if you don't, it's
worse. E.g.,
// { dg-xfail-if "" { *-*-* } }
int i = nothere; // demonstrates something that errors out
// { dg-error "" } didn't know where to put this
only prints unexpected failures, but no unexpected successes. I guess that's
OK though, at least for now, so I'll drop dg-accepts-invalid.
> > * ICEs: use the new dg-ice.
> >
>
> This seems like a good addition.
Thanks!
Marek
