On Sat, 2023-08-26 at 16:44 +0200, Benjamin Priour wrote:
> Hi David,
>
> Sorry I missed out on your answer about the operator new patch on the
> IRC.
> Should I submit the first bit of the operator new patch ? Putting
> aside for
> now fixing the
> "uninitialized value" that accompanies "null deref" of nothrow new
> variants
> ?
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.
>
> 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.
>
> 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?
>
> 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]
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
