Re: analyzer: Weekly update on extending C++ support (3)

[Benjamin Priour <priour.be@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4961 bytes --]

Hi David,


On Wed, Aug 30, 2023 at 1:01 AM David Malcolm <dmalcolm@redhat.com> wrote:

>
> > ?
>
> Yes; please submit it, so that we can work towards getting what you
> have into trunk.
>
> Given that we don't properly support C++ yet, improvements to C++
> support don't have to be perfect.
>
>
It is next in queue for regstrapping, and great news, I nailed the
"supersedes" issue correctly.
I'll split it in two patches, first one for operator new per se that should
fix PR105948,
and all non user-defined variants of operator new should be supported, with
and without
exceptions enabled. Second will be a fix of PR110830, to prevent
superseding warnings
when their saved_diagnostic path are actually divergent.

My current implementation of the latter is a bit naive but works, thus I'm
trying to improve
its running time.


> >
> > About generalizing tests to C++, I'll soon have a second batch of
> > similar
> > size ready,
> > probably by Monday. I try to find exactly one "real" bug to build
> > each
> > batch around, to not simply
> > have a collection of C made C++ friendly.
>
> (nods)
>
> Thanks for pushing the 1st patch.  I've updated my working copies to
> try to ensure that my new tests go in c-c++-common as far as possible.
>
> >
> > A few questions on that point.
> >
> > Test gcc.dg/analyzer/pr103892.c requires -O2.
> > As such the IPA generated from C and C++ differ,
> > C++ optimization cuts off function argstr_get_word from the IPA,
> > hence reducing the number of SN nodes from the SG.
> > This lesser number of SN is sufficient to make the analysis trips
> > over
> > being too-complex.
> > The current formula for that upper limit is
> > limit = m_sg.num_nodes () * param_analyzer_bb_explosion_factor;
> > Thus shorter programs - such as the analyzer tests - are more likely
> > to
> > be diagnosed as too complex. To avoid false positives perhaps we
> > should
> > add an offset, so that short SG get their chance ?
>
> That's an interesting idea...
>
> > This is just a tweak, and pr103892.c could as well be discarded for
> > C++,
> > divergent IPA between C and C++ are unavoidable at some point, and
> > some
> > tests won't make the transition anyway.
>
> ...but this approach is simpler, so maybe go with that.
>
>
Nods.

>
> > In gcc.dg/analyzer/malloc-1.c:test_50{b,c}, C++ is imprecise as of
> > the
> > memory freed.
> >
> > void test_50b (void)
> > {
> >   free ((void *) test_50a); /* { dg-warning "'free' of '&test_50a'
> > which
> > points to memory not on the heap \\\[CWE-590\\\]" } */
> >   /* { dg-bogus "'free' of 'test_50a' which points to memory not on
> > the
> > heap \\\[CWE-590\\\]" "c++ lacks precision of freed memory" { xfail
> > c++ }
> > .-1 } */
> > }
> >
> > void test_50c (void)
> > {
> >  my_label:
> >   free (&&my_label); /* { dg-warning "'free' of '&my_label' which
> > points to
> > memory not on the heap \\\[CWE-590\\\]" } */
> >   /* { dg-warning "'free' of '&& my_label' which points to memory not
> > on
> > the heap \\\[CWE-590\\\]" "" { xfail c++ } .-1 } */
> > }
> >
> > What do you think of this ?
> > I've checked malloc_state_machine::handle_free_of_non_heap, arg and
> > diag_arg are strictly equal.
> > There might be something to be done with how a tree is transformed
> > into a
> > diagnostic tree by get_diagnostic_tree,
> > but I'll wait for your feedback first.
>
> What does g++ emit for this with your updated test?
>
>
I'm not sure what you meant here.
For a free ((void *) test_50a); gcc emits "'free' of '&test_50a'", whereas
g++ emits "'free' of 'test_50a'",
which is less precise about the actually memory freed. This however only
seems to occur with this
function pointers and labels.


> >
> > Test gcc.dg/analyzer/pr94362-1.c actually has an additional
> > null_deref
> > warning in C++, which is not affected by exceptions
> > or optimizations. I will keep updated on that one.
>
> [...snip...; I see you covered this in a followup]
>
>
The fix worked and even a few other XFAILs pass in other tests.
I am regstrapping the second batch of transitioned test,
following up with the "operator new" stuff.
I had an issue with the regstrap, I don't why but comparing the *.sum
files from my control and patched versions gives me unintelligible output.
It warns me about previous tests having disappeared, even some totally
unrelated to the analyzer, although all of them are still here and manually
running
dejagnu on each of them - one by one - gives the exact same output as
control.

So I'm cleaning up my control and patched folders, and re-regstrap
everything.
Something broke and I don't know what.


> BTW, was there any other work of yours that I need to review? (I know
> about the work on placement-new and testsuite migration)
>
> Thanks again for your work on this
> Dave
>
>
Thanks,
Benjamin.