On 6/30/20 10:22 AM, David Malcolm 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.
Hmm, I guess it is the this pointer it's complaining about after
all. The note says it's argument 1 but it's really talking about
the implicit this pointer to the j::j (B*, int) constructor.
The simpler test case below emits the corresponding -Wnonnull
warning:
struct S {
S ();
void* operator new (__SIZE_TYPE__) { return 0; }
};
void g (void*);
S* f ()
{
return new S ();
}
$ gcc -O -S -Wall t.C
t.C: In static member function ‘static void* S::operator new(long
unsigned int)’:
t.C:3:47: warning: ‘operator new’ must not return NULL unless it is
declared ‘throw()’ (or ‘-fcheck-new’ is in effect)
3 | void* operator new (__SIZE_TYPE__) { return 0; }
| ^
In function ‘S* f()’:
cc1plus: warning: ‘this’ pointer null [-Wnonnull]
t.C:2:3: note: in a call to non-static member function ‘S::S()’
2 | S ();
| ^
My patch changed the warning from:
cc1plus: warning: argument 1 null where non-null expected [-Wnonnull]
because referring to this as argument 1 was confusing (it even
confused me in this instance!)
Martin
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
