[PATCH] analyzer: Fix -Wanalyzer-possible-null-argument warning

[PATCH] analyzer: Fix -Wanalyzer-possible-null-argument warning

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 469 bytes --]

gcc/testsuite/ChangeLog:

	* g++.dg/analyzer/pr94028.C: Make operator new non-throwing so
	that the compiler doesn't implicitly mark it as returning
	non-null.

Fixes these:

FAIL: g++.dg/analyzer/pr94028.C  -std=c++98 (test for excess errors)
FAIL: g++.dg/analyzer/pr94028.C  -std=c++14 (test for excess errors)
FAIL: g++.dg/analyzer/pr94028.C  -std=c++17 (test for excess errors)
FAIL: g++.dg/analyzer/pr94028.C  -std=c++2a (test for excess errors)

OK for master?



[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 832 bytes --]

commit 34e9c12533c6313d37f56900876e41f69e8474bc
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Jun 30 17:40:08 2020 +0100

    analyzer: Fix -Wanalyzer-possible-null-argument warning
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/analyzer/pr94028.C: Make operator new non-throwing so
            that the compiler doesn't implicitly mark it as returning
            non-null.

diff --git a/gcc/testsuite/g++.dg/analyzer/pr94028.C b/gcc/testsuite/g++.dg/analyzer/pr94028.C
index 0a222d1b991..c0c35d65829 100644
--- a/gcc/testsuite/g++.dg/analyzer/pr94028.C
+++ b/gcc/testsuite/g++.dg/analyzer/pr94028.C
@@ -12,7 +12,7 @@ enum e {} i;
 
 struct j
 {
-  void *operator new (__SIZE_TYPE__ b)
+  void *operator new (__SIZE_TYPE__ b) throw()
   {
     return calloc (b, sizeof (int)); // { dg-warning "leak" }
   }

Re: [PATCH] analyzer: Fix -Wanalyzer-possible-null-argument warning

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]

On 30/06/20 17:43 +0100, Jonathan Wakely wrote:
>gcc/testsuite/ChangeLog:
>
>	* g++.dg/analyzer/pr94028.C: Make operator new non-throwing so
>	that the compiler doesn't implicitly mark it as returning
>	non-null.
>
>Fixes these:
>
>FAIL: g++.dg/analyzer/pr94028.C  -std=c++98 (test for excess errors)
>FAIL: g++.dg/analyzer/pr94028.C  -std=c++14 (test for excess errors)
>FAIL: g++.dg/analyzer/pr94028.C  -std=c++17 (test for excess errors)
>FAIL: g++.dg/analyzer/pr94028.C  -std=c++2a (test for excess errors)

Updated to add PR 96014 to the commit log.

OK for master?


[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 874 bytes --]

commit 8fedcc43382868ffc345a9410584f668ff9c04c0
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Jun 30 17:40:08 2020 +0100

    analyzer: Fix -Wanalyzer-possible-null-argument warning (PR 96014)
    
    gcc/testsuite/ChangeLog:
    
            PR testsuite/96014
            * g++.dg/analyzer/pr94028.C: Make operator new non-throwing so
            that the compiler doesn't implicitly mark it as returning
            non-null.

diff --git a/gcc/testsuite/g++.dg/analyzer/pr94028.C b/gcc/testsuite/g++.dg/analyzer/pr94028.C
index 0a222d1b991..c0c35d65829 100644
--- a/gcc/testsuite/g++.dg/analyzer/pr94028.C
+++ b/gcc/testsuite/g++.dg/analyzer/pr94028.C
@@ -12,7 +12,7 @@ enum e {} i;
 
 struct j
 {
-  void *operator new (__SIZE_TYPE__ b)
+  void *operator new (__SIZE_TYPE__ b) throw()
   {
     return calloc (b, sizeof (int)); // { dg-warning "leak" }
   }

Re: [PATCH] analyzer: Fix -Wanalyzer-possible-null-argument warning

[Nathan Sidwell]

On 7/1/20 1:29 PM, Jonathan Wakely wrote:
> On 30/06/20 17:43 +0100, Jonathan Wakely wrote:
>> gcc/testsuite/ChangeLog:
>>
>>     * g++.dg/analyzer/pr94028.C: Make operator new non-throwing so
>>     that the compiler doesn't implicitly mark it as returning
>>     non-null.
>>
>> Fixes these:
>>
>> FAIL: g++.dg/analyzer/pr94028.C  -std=c++98 (test for excess errors)
>> FAIL: g++.dg/analyzer/pr94028.C  -std=c++14 (test for excess errors)
>> FAIL: g++.dg/analyzer/pr94028.C  -std=c++17 (test for excess errors)
>> FAIL: g++.dg/analyzer/pr94028.C  -std=c++2a (test for excess errors)
> 
> Updated to add PR 96014 to the commit log.
> 
> OK for master?

ok


-- 
Nathan Sidwell

Re: [PATCH] analyzer: Fix -Wanalyzer-possible-null-argument warning

[David Malcolm]

On Wed, 2020-07-01 at 18:29 +0100, Jonathan Wakely wrote:
> On 30/06/20 17:43 +0100, Jonathan Wakely wrote:
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/analyzer/pr94028.C: Make operator new non-throwing so
> > 	that the compiler doesn't implicitly mark it as returning
> > 	non-null.
> > 
> > Fixes these:
> > 
> > FAIL: g++.dg/analyzer/pr94028.C  -std=c++98 (test for excess
> > errors)
> > FAIL: g++.dg/analyzer/pr94028.C  -std=c++14 (test for excess
> > errors)
> > FAIL: g++.dg/analyzer/pr94028.C  -std=c++17 (test for excess
> > errors)
> > FAIL: g++.dg/analyzer/pr94028.C  -std=c++2a (test for excess
> > errors)
> 
> Updated to add PR 96014 to the commit log.
> 
> OK for master?

Sorry for not responding to this earlier.

My knowledge of C++ exceptions is a little rusty; I found the addition
of "throw()" to mark the decl as non-throwing to be confusing.

Looking in my copy of Stroustrup 4th edition (C++11) p367 it says this
is an empty exception specification, and is equivalent to "noexcept",
and Stroustrup recommends using the latter instead.  Did you use this
syntax for backwards compat with C++98, or is "noexcept" available in
the earlier C++ dialects?

Thanks
Dave

Re: [PATCH] analyzer: Fix -Wanalyzer-possible-null-argument warning

[Jonathan Wakely]

On 05/07/20 21:48 -0400, David Malcolm wrote:
>On Wed, 2020-07-01 at 18:29 +0100, Jonathan Wakely wrote:
>> On 30/06/20 17:43 +0100, Jonathan Wakely wrote:
>> > gcc/testsuite/ChangeLog:
>> >
>> > 	* g++.dg/analyzer/pr94028.C: Make operator new non-throwing so
>> > 	that the compiler doesn't implicitly mark it as returning
>> > 	non-null.
>> >
>> > Fixes these:
>> >
>> > FAIL: g++.dg/analyzer/pr94028.C  -std=c++98 (test for excess
>> > errors)
>> > FAIL: g++.dg/analyzer/pr94028.C  -std=c++14 (test for excess
>> > errors)
>> > FAIL: g++.dg/analyzer/pr94028.C  -std=c++17 (test for excess
>> > errors)
>> > FAIL: g++.dg/analyzer/pr94028.C  -std=c++2a (test for excess
>> > errors)
>>
>> Updated to add PR 96014 to the commit log.
>>
>> OK for master?
>
>Sorry for not responding to this earlier.
>
>My knowledge of C++ exceptions is a little rusty; I found the addition
>of "throw()" to mark the decl as non-throwing to be confusing.

An operator new is required to report allocation failure by throwing
an exception that can be caught by `catch (const std::bad_alloc&)`,
unless it is marked as non-throwing, in which case it reports
allocation failure by returning a null pointer.

I believe the C++ front end adds the returns_nonnull attribute to
operator new unless it is marked non-throwing.

The operator new in this test just calls calloc, which can return
null, but it isn't marked as non-throwing so has the returns_nonnull
attribute. Therefore it has undefined behaviour if calloc ever fails
and returns null. Your analyzer seems to be noticing this, and so
warning, which is nice.

The way to fix it is to either check the return value of calloc and
throw std::bad_alloc() if calloc returned null, or to simply mark the
operator new as non-throwing so that returning null is OK.


>Looking in my copy of Stroustrup 4th edition (C++11) p367 it says this
>is an empty exception specification, and is equivalent to "noexcept",
>and Stroustrup recommends using the latter instead.  Did you use this
>syntax for backwards compat with C++98, or is "noexcept" available in
>the earlier C++ dialects?

noexcept is not valid in C++98/C++03. But I forgot that throw() is
deprecated in C++17 and removed in C++20, so although G++ still
accepts throw() to be futureproof we should use:

#if __cplusplus < 201103L
# define NOTHROW throw()
#else
# define NOTHROW noexcept
#endif

and then mark it NOTHROW.

Although it would take fewer lines of code to just check what calloc
returns and turn a null pointer into a std::bad_alloc exception.

Re: [PATCH] analyzer: Fix -Wanalyzer-possible-null-argument warning

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 4096 bytes --]

On 06/07/20 11:27 +0100, Jonathan Wakely wrote:
>On 05/07/20 21:48 -0400, David Malcolm wrote:
>>On Wed, 2020-07-01 at 18:29 +0100, Jonathan Wakely wrote:
>>>On 30/06/20 17:43 +0100, Jonathan Wakely wrote:
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>> 	* g++.dg/analyzer/pr94028.C: Make operator new non-throwing so
>>>> 	that the compiler doesn't implicitly mark it as returning
>>>> 	non-null.
>>>>
>>>> Fixes these:
>>>>
>>>> FAIL: g++.dg/analyzer/pr94028.C  -std=c++98 (test for excess
>>>> errors)
>>>> FAIL: g++.dg/analyzer/pr94028.C  -std=c++14 (test for excess
>>>> errors)
>>>> FAIL: g++.dg/analyzer/pr94028.C  -std=c++17 (test for excess
>>>> errors)
>>>> FAIL: g++.dg/analyzer/pr94028.C  -std=c++2a (test for excess
>>>> errors)
>>>
>>>Updated to add PR 96014 to the commit log.
>>>
>>>OK for master?
>>
>>Sorry for not responding to this earlier.
>>
>>My knowledge of C++ exceptions is a little rusty; I found the addition
>>of "throw()" to mark the decl as non-throwing to be confusing.
>
>An operator new is required to report allocation failure by throwing
>an exception that can be caught by `catch (const std::bad_alloc&)`,
>unless it is marked as non-throwing, in which case it reports
>allocation failure by returning a null pointer.
>
>I believe the C++ front end adds the returns_nonnull attribute to
>operator new unless it is marked non-throwing.
>
>The operator new in this test just calls calloc, which can return
>null, but it isn't marked as non-throwing so has the returns_nonnull
>attribute. Therefore it has undefined behaviour if calloc ever fails
>and returns null. Your analyzer seems to be noticing this, and so
>warning, which is nice.
>
>The way to fix it is to either check the return value of calloc and
>throw std::bad_alloc() if calloc returned null, or to simply mark the
>operator new as non-throwing so that returning null is OK.
>
>
>>Looking in my copy of Stroustrup 4th edition (C++11) p367 it says this
>>is an empty exception specification, and is equivalent to "noexcept",
>>and Stroustrup recommends using the latter instead.  Did you use this
>>syntax for backwards compat with C++98, or is "noexcept" available in
>>the earlier C++ dialects?
>
>noexcept is not valid in C++98/C++03. But I forgot that throw() is
>deprecated in C++17 and removed in C++20, so although G++ still
>accepts throw() to be futureproof we should use:
>
>#if __cplusplus < 201103L
># define NOTHROW throw()
>#else
># define NOTHROW noexcept
>#endif
>
>and then mark it NOTHROW.
>
>Although it would take fewer lines of code to just check what calloc
>returns and turn a null pointer into a std::bad_alloc exception.

It looks like I accidentally pushed this patch without approval (when
pushing my own a1a0dc4548979f8a340a7ea71624a52a20e1e0b3 change).

As discussed above, the throw() is not actually valid in C++20. Should
I push this patch to use noexcept instead for C++11 and up?


Tested x86_64-linux:

PASS: g++.dg/analyzer/pr94028.C  -std=c++14  (test for warnings, line 22)
PASS: g++.dg/analyzer/pr94028.C  -std=c++14  (test for warnings, line 26)
PASS: g++.dg/analyzer/pr94028.C  -std=c++14  (test for warnings, line 33)
PASS: g++.dg/analyzer/pr94028.C  -std=c++14 (test for excess errors)
PASS: g++.dg/analyzer/pr94028.C  -std=c++17  (test for warnings, line 22)
PASS: g++.dg/analyzer/pr94028.C  -std=c++17  (test for warnings, line 26)
PASS: g++.dg/analyzer/pr94028.C  -std=c++17  (test for warnings, line 33)
PASS: g++.dg/analyzer/pr94028.C  -std=c++17 (test for excess errors)
PASS: g++.dg/analyzer/pr94028.C  -std=c++2a  (test for warnings, line 22)
PASS: g++.dg/analyzer/pr94028.C  -std=c++2a  (test for warnings, line 26)
PASS: g++.dg/analyzer/pr94028.C  -std=c++2a  (test for warnings, line 33)
PASS: g++.dg/analyzer/pr94028.C  -std=c++2a (test for excess errors)
PASS: g++.dg/analyzer/pr94028.C  -std=c++98  (test for warnings, line 22)
PASS: g++.dg/analyzer/pr94028.C  -std=c++98  (test for warnings, line 26)
PASS: g++.dg/analyzer/pr94028.C  -std=c++98  (test for warnings, line 33)
PASS: g++.dg/analyzer/pr94028.C  -std=c++98 (test for excess errors)




[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 916 bytes --]

commit bd5fbae9d5caaa9810fb3c2a6652f39666d2a728
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Jul 16 11:44:32 2020

    analyzer: Use noexcept instead of throw() for C++11 and later (PR 96014)
    
    gcc/testsuite/ChangeLog:
    
            PR testsuite/96014
            * g++.dg/analyzer/pr94028.C: Replace dynamic exception
            specification with noexcept-specifier for C++11 and later.

diff --git a/gcc/testsuite/g++.dg/analyzer/pr94028.C b/gcc/testsuite/g++.dg/analyzer/pr94028.C
index c0c35d65829..5c8e2c9b286 100644
--- a/gcc/testsuite/g++.dg/analyzer/pr94028.C
+++ b/gcc/testsuite/g++.dg/analyzer/pr94028.C
@@ -12,7 +12,12 @@ enum e {} i;
 
 struct j
 {
-  void *operator new (__SIZE_TYPE__ b) throw()
+  void *operator new (__SIZE_TYPE__ b)
+#if __cplusplus >= 201103L
+    noexcept
+#else
+    throw()
+#endif
   {
     return calloc (b, sizeof (int)); // { dg-warning "leak" }
   }

Re: [PATCH] analyzer: Fix -Wanalyzer-possible-null-argument warning

[Nathan Sidwell]

On 7/16/20 9:01 AM, Jonathan Wakely wrote:
> On 06/07/20 11:27 +0100, Jonathan Wakely wrote:
>> On 05/07/20 21:48 -0400, David Malcolm wrote:
>>> On Wed, 2020-07-01 at 18:29 +0100, Jonathan Wakely wrote:
>>>> On 30/06/20 17:43 +0100, Jonathan Wakely wrote:
>>>>> gcc/testsuite/ChangeLog:
>>>>>
>>>>>     * g++.dg/analyzer/pr94028.C: Make operator new non-throwing so
>>>>>     that the compiler doesn't implicitly mark it as returning
>>>>>     non-null.
>>>>>
>>>>> Fixes these:
>>>>>
>>>>> FAIL: g++.dg/analyzer/pr94028.C  -std=c++98 (test for excess
>>>>> errors)
>>>>> FAIL: g++.dg/analyzer/pr94028.C  -std=c++14 (test for excess
>>>>> errors)
>>>>> FAIL: g++.dg/analyzer/pr94028.C  -std=c++17 (test for excess
>>>>> errors)
>>>>> FAIL: g++.dg/analyzer/pr94028.C  -std=c++2a (test for excess
>>>>> errors)
>>>>
>>>> Updated to add PR 96014 to the commit log.
>>>>
>>>> OK for master?
>>>
>>> Sorry for not responding to this earlier.
>>>
>>> My knowledge of C++ exceptions is a little rusty; I found the addition
>>> of "throw()" to mark the decl as non-throwing to be confusing.
>>
>> An operator new is required to report allocation failure by throwing
>> an exception that can be caught by `catch (const std::bad_alloc&)`,
>> unless it is marked as non-throwing, in which case it reports
>> allocation failure by returning a null pointer.
>>
>> I believe the C++ front end adds the returns_nonnull attribute to
>> operator new unless it is marked non-throwing.
>>
>> The operator new in this test just calls calloc, which can return
>> null, but it isn't marked as non-throwing so has the returns_nonnull
>> attribute. Therefore it has undefined behaviour if calloc ever fails
>> and returns null. Your analyzer seems to be noticing this, and so
>> warning, which is nice.
>>
>> The way to fix it is to either check the return value of calloc and
>> throw std::bad_alloc() if calloc returned null, or to simply mark the
>> operator new as non-throwing so that returning null is OK.
>>
>>
>>> Looking in my copy of Stroustrup 4th edition (C++11) p367 it says this
>>> is an empty exception specification, and is equivalent to "noexcept",
>>> and Stroustrup recommends using the latter instead.  Did you use this
>>> syntax for backwards compat with C++98, or is "noexcept" available in
>>> the earlier C++ dialects?
>>
>> noexcept is not valid in C++98/C++03. But I forgot that throw() is
>> deprecated in C++17 and removed in C++20, so although G++ still
>> accepts throw() to be futureproof we should use:
>>
>> #if __cplusplus < 201103L
>> # define NOTHROW throw()
>> #else
>> # define NOTHROW noexcept
>> #endif
>>
>> and then mark it NOTHROW.
>>
>> Although it would take fewer lines of code to just check what calloc
>> returns and turn a null pointer into a std::bad_alloc exception.
> 
> It looks like I accidentally pushed this patch without approval (when
> pushing my own a1a0dc4548979f8a340a7ea71624a52a20e1e0b3 change).
> 
> As discussed above, the throw() is not actually valid in C++20. Should
> I push this patch to use noexcept instead for C++11 and up?

yes thanks

> Tested x86_64-linux:
> 
> PASS: g++.dg/analyzer/pr94028.C  -std=c++14  (test for warnings, line 22)
> PASS: g++.dg/analyzer/pr94028.C  -std=c++14  (test for warnings, line 26)
> PASS: g++.dg/analyzer/pr94028.C  -std=c++14  (test for warnings, line 33)
> PASS: g++.dg/analyzer/pr94028.C  -std=c++14 (test for excess errors)
> PASS: g++.dg/analyzer/pr94028.C  -std=c++17  (test for warnings, line 22)
> PASS: g++.dg/analyzer/pr94028.C  -std=c++17  (test for warnings, line 26)
> PASS: g++.dg/analyzer/pr94028.C  -std=c++17  (test for warnings, line 33)
> PASS: g++.dg/analyzer/pr94028.C  -std=c++17 (test for excess errors)
> PASS: g++.dg/analyzer/pr94028.C  -std=c++2a  (test for warnings, line 22)
> PASS: g++.dg/analyzer/pr94028.C  -std=c++2a  (test for warnings, line 26)
> PASS: g++.dg/analyzer/pr94028.C  -std=c++2a  (test for warnings, line 33)
> PASS: g++.dg/analyzer/pr94028.C  -std=c++2a (test for excess errors)
> PASS: g++.dg/analyzer/pr94028.C  -std=c++98  (test for warnings, line 22)
> PASS: g++.dg/analyzer/pr94028.C  -std=c++98  (test for warnings, line 26)
> PASS: g++.dg/analyzer/pr94028.C  -std=c++98  (test for warnings, line 33)
> PASS: g++.dg/analyzer/pr94028.C  -std=c++98 (test for excess errors)
> 
> 
> 


-- 
Nathan Sidwell