Hi!
On Wed, 6 Sept 2023 at 22:22, David Malcolm via Gcc-patches <
gcc-patches@gcc.gnu.org> wrote:
> On Wed, 2023-09-06 at 15:50 +0200, Benjamin Priour wrote:
> > Hi David,
> > Thanks for the review.
> >
> >
> >
> > On Tue, Sep 5, 2023 at 1:53 PM David Malcolm <dmalc...@redhat.com>
> > wrote:
> >
> > > On Mon, 2023-09-04 at 20:00 +0200, priour...@gmail.com wrote:
> > >
> > >
> > [...snip...]
> >
> >
> > > All of these "new" tests (apart from the "-noexcept" ones) look
> > > like
> > > they're meant to be existing tests that were moved, but where the
> > > copy
> > > of the test in gcc.dg/analyzer didn't get deleted, so they show up
> > > as a
> > > duplicate. See the details below.
> > >
> >
> > > > * c-c++-common/analyzer/file-pr58237-noexcept.c: New test.
> > >
> > > When duplicating a test like this, the test isn't entirely "new",
> > > so
> > > please say something like this in the ChangeLog entry, to make it
> > > clear
> > > where it came from:
> > >
> > >
> > I actually wasn't sure about these -noexcept tests. They were part
> > of gcc.dg/analyzer, thus only gcc was running them. Exceptions
> > were not disabled *explicitly*, but since it was C, they weren't
> > enabled
> > either.
> >
> > Therefore, the -noexcept tests are basically a copy, but with an
> > explicit
> > -fno-exceptions specification.
> > When I duplicated them in that way I was thinking about making it
> > clear
> > that these tests fail in C++ with exceptions enabled, so that we
> > would
> > already
> > have easy-to-spot failing tests to challenge a future exception
> > support.
>
> Ah, OK; let's go with your approach.
>
> >
> > Though perhaps *not* duplicating the tests but rather simply specify
> > -fno-exceptions,
> > with a comment "Fails with exceptions" may be better.
>
> [...snip...]
>
> > > > @@ -45,7 +45,7 @@ void test(int n)
> > > > struct iter *it = iter_new (0, n, 1);
> > > > while (!iter_done_p (it))
> > > > {
> > > > - __analyzer_eval (it->val < n); /* { dg-warning "TRUE"
> > > > "true" {
> > > xfail *-*-* } } */
> > > > + __analyzer_eval (it->val < n); /* { dg-warning "TRUE"
> > > > "true" } */
> > > > /* { dg-bogus "UNKNOWN" "unknown" { xfail *-*-* } .-1 } */
> > > > /* TODO(xfail^^^): ideally we ought to figure out i > 0
> > > > after 1st
> > > iteration. */
> > > >
> > >
> > > Presumably due to the change to
> > > region_model::add_constraints_from_binop, right?
> > > Looking at that dg-bogus "UNKNOWN", do we still get an UNKNOWN
> > > here, or
> > > can that line be removed?
> > > If so, then the 3rd comment can presumably become:
> > >
> > >
> > The bogus here still make sense - without it there is an excess error
> > -.
> > I had checked for it because I too thought it could be removed.
> > If I remember it correctly, we get UNKNOWN during the widening pass.
>
> (nods)
>
> >
> >
> > > > /* TODO: ideally we ought to figure out i > 0 after 1st
> > > iteration. */
> > >
> > > [...snip...]
> > >
> > >
> > >
> > [...snip...]
> >
> > Thanks for spotting the files I forgot to remove from gcc.dg.
> > Sorry about them, I had messed up my test folder when checking for
> > arm-eabi,
> > and I apparently missed some duplicates when retrieving my save.
> >
> > As for the files the likes of inlining-*.c, i.e. noted as Moved
> > to..../...here.
> > at the end of the ChangeLog, some tests checking for multiline
> > outputs
> > are so heavily rewritten than git marks them as Removed/New test
> > instead of moved. I've manually edited that, but perhaps I shouldn't
> > ?
> >
> > I have successfully regstrapped the improvements you suggested.
>
> Thanks. Did you want me to doublecheck the updated patch? Otherwise
> feel free to push it to trunk.
>
> Was this patch committed as r14-3823-g50b5199cff6908?
Our CI noticed regression after that revision, on arm-eabi.
I looked at the logs for out-of-bounds-diagram-11.c, where we have:
FAIL: c-c++-common/analyzer/out-of-bounds-diagram-11.c expected multiline
pattern lines 46-61
FAIL: c-c++-common/analyzer/out-of-bounds-diagram-11.c 2 blank line(s) in
output
FAIL: c-c++-common/analyzer/out-of-bounds-diagram-11.c (test for excess
errors)
After visual inspection, I noticed that we emit:
write of '(int32_t) 42'
but expect:
write of '(int) 42'
(that is, we emit 'int32_t' and expect 'int')
I didn't check all the regressions, but does that ring a bell?
Thanks,
Christophe
> > Part 3 of this serie of patches I hope will be regstrapped for
> > Friday.
>
> Thanks; I'm impressed at how much progress you've made on this problem.
>
> Dave
>
>
