Why did you mark PR96008 as a duplicate? The ICE is a duplicate, but
the wrong IL is a C++ FE bug.
Thanks, David
On Tue, Jun 30, 2020 at 1:45 PM Martin Sebor <mse...@gmail.com> wrote:
>
> On 6/30/20 10:47 AM, David Edelsohn wrote:
> > 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]
>
> I think this (mainly the ICE) is the subject of pr95984. AFAICT,
> the warning is working correctly but the IL the C++ front end
> emits doesn't look right: AFAICS, it creates a function object
> for the lambda and calls its operator() with a null this pointer:
>
> ; Function static decltype (((const main()::<lambda(auto:1
> ...)>*)0)->operator()<auto:1
> ...>(static_cast<auto:1&&>(main::._anon_0::_FUN::<unnamed>) ...))
> main()::<lambda(auto:1 ...)>::_FUN(auto:1 ...) [with auto:1 = {int};
> decltype (((const main()::<lambda(auto:1 ...)>*)0)->operator()<auto:1
> ...>(static_cast<auto:1&&>(main::._anon_0::_FUN::<unnamed>) ...)) =
> void] (null)
> ;; enabled by -tree-original
>
>
> <<cleanup_point <<< Unknown tree: expr_stmt
> main()::<lambda(auto:1 ...)>::operator()<int> (0B, D.2440) >>>>>;
> return;
>
> Martin
>
> >
> > 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
> >>>>>
> >>
>
