Also, cpp1y/lambda-generic-69078-1.C elicits a similar, new warning: FAIL: g++.dg/cpp1y/lambda-generic-69078-1.C -std=gnu++14 (test for excess errors) Excess errors: g++.dg/cpp1y/lambda-generic-69078-1.C:23:13: warning: 'this' pointer null [-Wnonnull]
Thanks, David On Tue, Jun 30, 2020 at 12:23 PM David Malcolm <dmalc...@redhat.com> wrote: > > 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 > > > > >