On Tue, 2020-06-30 at 10:12 -0600, Martin Sebor wrote: > On 6/30/20 8:47 AM, David Edelsohn via Gcc-patches wrote: > > The unexpected warning is > > > > gcc/testsuite/g++.dg/analyzer/pr94028.C:28:21: warning: use of > > possibly-NULL '<unknown>' where non-null expected [CWE-690] > > [-Wanalyzer-possible-null-argument] > > > > This is the same location as one of the existing "leak" warnings. > > I'm surprised that the Wnonull change triggers > a -Wanalyzer-possible-null-argument warning. There is no > -Wnonnull for the test case with or without -fanalyzer (and > with or without optimization) so that suggests the two are > independent of one another. > > I'm also not sure I see a justification for a nonnull warning > in the test. The note printed after the warning says: > > | | (5) possible return of NULL to ‘f’ > from ‘j::operator new’ > | | (6) argument 1 (‘<unknown>’) from > (4) > could be NULL where non-null expected > | > /src/gcc/master/gcc/testsuite/g++.dg/analyzer/pr94028.C:19:3: note: > argument 1 of ‘j::j(B*, int)’ must be non-null > 19 | j (B *, int) > | ^ > > but the first argument in j::j(B*, int) is not marked nonnull, > and it's not the this pointer (treating those as implicitly > nonnull was one of the changes introduced by my patch).
Are you sure it's not the "this" pointer? Bear in mind that the C++ "support" in the analyzer is practically non-existent; I haven't yet implemented any special-casing about how arguments are numbered, so it could well be the "this" pointer. If it is a complaint about the "this" pointer, that could explain why this started happening when your patch went in. Dave > > FWIW, I don't remember if I saw it when going through the test > results for my patch but if I did, it's possible that I assumed > it was unrelated because of the -Wanalyzer- option. Sorry if > this was the wrong call. > > Martin > > How would you like pr94028.C to be adjusted in the testsuite to > > account for this? > > > > Thanks, David > > > > On Tue, Jun 30, 2020 at 10:18 AM David Malcolm <dmalc...@redhat.com > > > wrote: > > > On Tue, 2020-06-30 at 09:51 -0400, David Edelsohn wrote: > > > > The changes to the non-null warning now produce an additional > > > > warning > > > > for analyzer/pr94028.C on one of the "leak" lines. This causes > > > > new > > > > failures on trunk. > > > > > > Hi David > > > > > > Do you have the output to hand? What is the full text of the new > > > diagnostic? > > > > > > Some high level questions: > > > * Ought GCC to warn for that code? (or not) > > > * Is it behavior we ought to have a test for? > > > > > > Note that AFAIK there are *two* kinds of non-null warnings that > > > GCC can > > > emit: the kind emitted by Martin's code, versus those emitted by > > > -fanalyzer (the latter imply the much more expensive analysis > > > performed > > > by -fanalyzer, and are controlled by the various -Wanalyzer- > > > *null* > > > options; see > > > https://gcc.gnu.org/onlinedocs/gcc-10.1.0/gcc/Static-Analyzer-Options.html > > > ) > > > > > > > > > > Because non-null is not the purpose of the analyzer test, I > > > > propose > > > > pruning the output to resolve the new failures. > > > > > > Looking back through bugzilla, it seems that the main purpose of > > > adding > > > that test was to ensure that -fanalyzer doesn't ICE on that code. > > > > > > At some point I hope to properly support C++ in -fanalyzer, at > > > which > > > point some kind of null warning may be warranted on that > > > code. Sadly > > > I'm not yet at that point. FWIW I'm currently working on a big > > > rewrite > > > of how state is tracked within the analyzer, as I've identified > > > at > > > least two major flaws in the current implementation, which my > > > rewrite > > > addresses. I'm deferring on C++ support until that rewrite is > > > done. > > > > > > > Alternatively, I > > > > could explicitly test for the additional non-null warning. > > > > > > > > Do you have any preferences? > > > > > > This test is controlled by analyzer.exp and thus, for example, is > > > disabled if the analyzer was disabled at configure time. > > > > > > If this is coming from Martin's non-analyzer code, a third > > > possibility > > > would be to use -Wno-something to disable that warning, so that > > > the > > > analyzer test can focus on the -fanalyzer test, as it were (and > > > if this > > > is a behavior that ought to be checked for in Martin's warning, > > > then > > > copy the pertinent fragment of the testcase to one of the g++.dg > > > cases, > > > I suppose). I think I prefer that approach. > > > > > > How does this sound? > > > > > > > I propose appending > > > > > > > > // { dg-prune-output "where non-null expected" } > > > > > > > > to the file to prune the warning output. > > > > > > > > Thanks, David > > > > > > Thanks for looking into this; hope this is constructive. > > > Dave > > >
