[PATCH] analyzer/pr94028.C and non-null warning

Tue Jun 30 17:45:02 GMT 2020

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 <dmalcolm@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 <dmalcolm@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
>>>>>
>>