| Message ID | 20200630164348.GA3500050@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | analyzer: Fix -Wanalyzer-possible-null-argument warning | expand |
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?
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
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
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.
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)
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) > > >
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" } }
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? 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.