On 6/30/20 11:59 AM, David Edelsohn wrote:
Why did you mark PR96008 as a duplicate? The ICE is a duplicate, but the wrong IL is a C++ FE bug.
They're both caused by the same problem: the -Wnonnull warning is triggered by the C++ FE bug (assuming it is one) and the ICE by the C++ pretty printer calling itself recursively because of it. Somehow the variadic lambda is triggering this in both tests. Martin
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; MartinThanks, 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. DaveFWIW, 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. MartinHow 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.comwrote: 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, DavidThanks for looking into this; hope this is constructive. Dave
