Hi Christophe,
On Mon, Sep 11, 2023 at 4:23 PM Christophe Lyon <christophe.l...@linaro.org>
wrote:
> 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?
>
>
Indeed.
> 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
>
>
Thanks for spotting that.
I checked the diff of the aforementioned patch. The variable type has not
been changed
nor the expected multiline-output.
Do you observe the regression in C and/or C++ ?
Thanks,
Benjamin.
