std::string add nullptr attribute

std::string add nullptr attribute

[Jonny Grant]

Hello
Could GCC's STL implementation add nullptr attribute to prevent code like this compiling?

g++ -Wall -o string string.cpp

 std::string c(nullptr);


 At least GCC STL does reject it at runtime:

terminate called after throwing an instance of 'std::logic_error'
  what():  basic_string: construction from null is not valid
Aborted (core dumped)


Note, my code isn't like this, it is just an example to suggest adding the nullptr attribute, as its clearly already rejected at runtime.

Regards
Jonny

Re: std::string add nullptr attribute

[Jonathan Wakely]

On Thu, 9 Feb 2023 at 13:28, Jonny Grant wrote:
>
> Hello
> Could GCC's STL implementation add nullptr attribute to prevent code like this compiling?
>
> g++ -Wall -o string string.cpp
>
>  std::string c(nullptr);
>
>
>  At least GCC STL does reject it at runtime:
>
> terminate called after throwing an instance of 'std::logic_error'
>   what():  basic_string: construction from null is not valid
> Aborted (core dumped)
>
>
> Note, my code isn't like this, it is just an example to suggest adding the nullptr attribute, as its clearly already rejected at runtime.

I assume you mean the nonnull attribute. That was added in 2020 and
then reverted because it broke some things:

commit 2635f9e5086318f4560997d9741fdda496b9c801
Author: Ville Voutilainen
Date:   Mon Jun 29 23:59:34 2020

   Revert "Add a __nonnnull__ attribute to std::string's _CharT* constructor"

   This reverts commit b26fd416fb0a734d3f3e56629b6dff2e3c25dd40.

commit b26fd416fb0a734d3f3e56629b6dff2e3c25dd40
Author: Ville Voutilainen
Date:   Sun Jun 28 22:47:05 2020

   Add a __nonnnull__ attribute to std::string's _CharT* constructor

           Add a __nonnnull__ attribute to std::string's _CharT* constructor
           * include/bits/basic_string.h (string(_CharT*, const _Alloc&)):
           Add a __nonnull__ attribute.
           * testsuite/21_strings/basic_string/cons/char/nonnull.cc: New.
           * testsuite/21_strings/basic_string/cons/wchar_t/nonnull.cc:
Likewise.


I don't know if anybody analyzed why it broke tests, to see if it
could be made to work.

Re: std::string add nullptr attribute

[Xi Ruoyao]

On Thu, 2023-02-09 at 14:56 +0000, Jonathan Wakely via Gcc-help wrote:
> > Note, my code isn't like this, it is just an example to suggest
> > adding the nullptr attribute, as its clearly already rejected at
> > runtime.
> 
> I assume you mean the nonnull attribute. That was added in 2020 and
> then reverted because it broke some things:

I remember I'd once made the same mistake when I suggested to add
nonnull for ostream::operator<<(const string &) and I was lectured:
nonnull is not only a diagnostic attribute, it also allows the compiler
to assume the parameter is never null and rendering std::string(nullptr)
an undefined behavior.

Then the example may just silently continue to run, instead of throwing
an exception.  It would be an ironic example: an attempt to improve
diagnostic finally made diagnostic more difficult.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: std::string add nullptr attribute

[Jonathan Wakely]

On Thu, 9 Feb 2023 at 16:30, Xi Ruoyao wrote:
>
> On Thu, 2023-02-09 at 14:56 +0000, Jonathan Wakely via Gcc-help wrote:
> > > Note, my code isn't like this, it is just an example to suggest
> > > adding the nullptr attribute, as its clearly already rejected at
> > > runtime.
> >
> > I assume you mean the nonnull attribute. That was added in 2020 and
> > then reverted because it broke some things:
>
> I remember I'd once made the same mistake when I suggested to add
> nonnull for ostream::operator<<(const string &) and I was lectured:
> nonnull is not only a diagnostic attribute, it also allows the compiler
> to assume the parameter is never null and rendering std::string(nullptr)
> an undefined behavior.

Yes, I think that's what might have happened with the std::string change.

> Then the example may just silently continue to run, instead of throwing
> an exception.  It would be an ironic example: an attempt to improve
> diagnostic finally made diagnostic more difficult.

Indeed.

Maybe we can add __attribute__((access(read, 1))) instead, which says
that we will read from the pointer, which also implies it must be
non-null.

N.B. in C++23 string(nullptr) produces an error, although
string((const char*)nullptr) doesn't, so in practice it only prevents
the dumbest calls with a literal 'nullptr' token, and not the more
realistic problems where you have a pointer that happens to be null.

Re: std::string add nullptr attribute

[Jonny Grant]

On 09/02/2023 17:52, Jonathan Wakely wrote:
> On Thu, 9 Feb 2023 at 16:30, Xi Ruoyao wrote:
>>
>> On Thu, 2023-02-09 at 14:56 +0000, Jonathan Wakely via Gcc-help wrote:
>>>> Note, my code isn't like this, it is just an example to suggest
>>>> adding the nullptr attribute, as its clearly already rejected at
>>>> runtime.
>>>
>>> I assume you mean the nonnull attribute. That was added in 2020 and
>>> then reverted because it broke some things:
>>
>> I remember I'd once made the same mistake when I suggested to add
>> nonnull for ostream::operator<<(const string &) and I was lectured:
>> nonnull is not only a diagnostic attribute, it also allows the compiler
>> to assume the parameter is never null and rendering std::string(nullptr)
>> an undefined behavior.
> 
> Yes, I think that's what might have happened with the std::string change.

My apologies, Jonathan, Xi, yes it is the __attribute__((nonnull)); I was mistaken to type as nullptr.

I re-read, and it does seem nonnull is really an optimization that as a side effect may give some warnings. So I'm going to stop using it.
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes

(there is a typo in that manual section saying "nonnul" - I don't know if you have a moment to make a change in git? I didn't get replies on gcc-patches to my patches...)

I searched and see like someone investigated this problem and saw it removed NULL checks http://www.rkoucha.fr/tech_corner/nonnull_gcc_attribute.html

I saw wget2 removed the nonnull attribute due to the optimizer removing checks against NULL too
https://gitlab.com/gnuwget/wget2/-/issues/200

>> Then the example may just silently continue to run, instead of throwing
>> an exception.  It would be an ironic example: an attempt to improve
>> diagnostic finally made diagnostic more difficult.
> 
> Indeed.
> 
> Maybe we can add __attribute__((access(read, 1))) instead, which says
> that we will read from the pointer, which also implies it must be
> non-null.

I tried this with gcc 12, as read_only, but it didn't stop when compiling. Maybe you have an example that demonstrates please?

void f(const char * p) __attribute__((access(read_only, 1)));

> 
> N.B. in C++23 string(nullptr) produces an error, although
> string((const char*)nullptr) doesn't, so in practice it only prevents
> the dumbest calls with a literal 'nullptr' token, and not the more
> realistic problems where you have a pointer that happens to be null.

That's good it stops compiling, the error is not that clear "use of deleted function" for me though.

string.cpp: In function ‘int main()’:
string.cpp:13:26: error: use of deleted function ‘std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::basic_string(std::nullptr_t) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::nullptr_t = std::nullptr_t]’
   13 |     std::string c(nullptr);




I made my own test class str_string which stops the build a different way. It only works if the dumbest calls with 'nullptr' as you found in your test.

void nullptr_compile_abort() __attribute__((error("nullptr compile error")));

str_string(nullptr_t) { nullptr_compile_abort(); }


 g++ -std=c++23 -Wall -O1 -o string2 string2.cpp
In constructor ‘str_string::str_string(nullptr_t)’,
    inlined from ‘int main()’ at string2.cpp:48:25:
string2.cpp:20:50: error: call to ‘nullptr_compile_abort’ declared with attribute error: nullptr compile error
   20 |     str_string(nullptr_t) { nullptr_compile_abort(); }

Jonny

Re: std::string add nullptr attribute

[Jonathan Wakely]

On Fri, 10 Feb 2023 at 21:30, Jonny Grant <jg@jguk.org> wrote:
>
>
>
> On 09/02/2023 17:52, Jonathan Wakely wrote:
> > On Thu, 9 Feb 2023 at 16:30, Xi Ruoyao wrote:
> >>
> >> On Thu, 2023-02-09 at 14:56 +0000, Jonathan Wakely via Gcc-help wrote:
> >>>> Note, my code isn't like this, it is just an example to suggest
> >>>> adding the nullptr attribute, as its clearly already rejected at
> >>>> runtime.
> >>>
> >>> I assume you mean the nonnull attribute. That was added in 2020 and
> >>> then reverted because it broke some things:
> >>
> >> I remember I'd once made the same mistake when I suggested to add
> >> nonnull for ostream::operator<<(const string &) and I was lectured:
> >> nonnull is not only a diagnostic attribute, it also allows the compiler
> >> to assume the parameter is never null and rendering std::string(nullptr)
> >> an undefined behavior.
> >
> > Yes, I think that's what might have happened with the std::string change.
>
> My apologies, Jonathan, Xi, yes it is the __attribute__((nonnull)); I was mistaken to type as nullptr.
>
> I re-read, and it does seem nonnull is really an optimization that as a side effect may give some warnings. So I'm going to stop using it.
> https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
>
> (there is a typo in that manual section saying "nonnul" - I don't know if you have a moment to make a change in git? I didn't get replies on gcc-patches to my patches...)
>
> I searched and see like someone investigated this problem and saw it removed NULL checks http://www.rkoucha.fr/tech_corner/nonnull_gcc_attribute.html
>
> I saw wget2 removed the nonnull attribute due to the optimizer removing checks against NULL too
> https://gitlab.com/gnuwget/wget2/-/issues/200
>
> >> Then the example may just silently continue to run, instead of throwing
> >> an exception.  It would be an ironic example: an attempt to improve
> >> diagnostic finally made diagnostic more difficult.
> >
> > Indeed.
> >
> > Maybe we can add __attribute__((access(read, 1))) instead, which says
> > that we will read from the pointer, which also implies it must be
> > non-null.
>
> I tried this with gcc 12, as read_only, but it didn't stop when compiling. Maybe you have an example that demonstrates please?
>
> void f(const char * p) __attribute__((access(read_only, 1)));
>
> >
> > N.B. in C++23 string(nullptr) produces an error, although
> > string((const char*)nullptr) doesn't, so in practice it only prevents
> > the dumbest calls with a literal 'nullptr' token, and not the more
> > realistic problems where you have a pointer that happens to be null.
>
> That's good it stops compiling, the error is not that clear "use of deleted function" for me though.
>
> string.cpp: In function ‘int main()’:
> string.cpp:13:26: error: use of deleted function ‘std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::basic_string(std::nullptr_t) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::nullptr_t = std::nullptr_t]’
>    13 |     std::string c(nullptr);
>
>
>
>
> I made my own test class str_string which stops the build a different way. It only works if the dumbest calls with 'nullptr' as you found in your test.
>
> void nullptr_compile_abort() __attribute__((error("nullptr compile error")));
>
> str_string(nullptr_t) { nullptr_compile_abort(); }

This doesn't work because std::is_constructible_v<std::string,
std::nullptr_t> would be true, and we want it to be false.

>
>
>  g++ -std=c++23 -Wall -O1 -o string2 string2.cpp
> In constructor ‘str_string::str_string(nullptr_t)’,
>     inlined from ‘int main()’ at string2.cpp:48:25:
> string2.cpp:20:50: error: call to ‘nullptr_compile_abort’ declared with attribute error: nullptr compile error
>    20 |     str_string(nullptr_t) { nullptr_compile_abort(); }
>
> Jonny

Re: std::string add nullptr attribute

[Jonny Grant]

On 10/02/2023 22:03, Jonathan Wakely wrote:
> On Fri, 10 Feb 2023 at 21:30, Jonny Grant <jg@jguk.org> wrote:
>>
>>
>>
>> On 09/02/2023 17:52, Jonathan Wakely wrote:
>>> On Thu, 9 Feb 2023 at 16:30, Xi Ruoyao wrote:
>>>>
>>>> On Thu, 2023-02-09 at 14:56 +0000, Jonathan Wakely via Gcc-help wrote:
>>>>>> Note, my code isn't like this, it is just an example to suggest
>>>>>> adding the nullptr attribute, as its clearly already rejected at
>>>>>> runtime.
>>>>>
>>>>> I assume you mean the nonnull attribute. That was added in 2020 and
>>>>> then reverted because it broke some things:
>>>>
>>>> I remember I'd once made the same mistake when I suggested to add
>>>> nonnull for ostream::operator<<(const string &) and I was lectured:
>>>> nonnull is not only a diagnostic attribute, it also allows the compiler
>>>> to assume the parameter is never null and rendering std::string(nullptr)
>>>> an undefined behavior.
>>>
>>> Yes, I think that's what might have happened with the std::string change.
>>
>> My apologies, Jonathan, Xi, yes it is the __attribute__((nonnull)); I was mistaken to type as nullptr.
>>
>> I re-read, and it does seem nonnull is really an optimization that as a side effect may give some warnings. So I'm going to stop using it.
>> https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
>>
>> (there is a typo in that manual section saying "nonnul" - I don't know if you have a moment to make a change in git? I didn't get replies on gcc-patches to my patches...)
>>
>> I searched and see like someone investigated this problem and saw it removed NULL checks http://www.rkoucha.fr/tech_corner/nonnull_gcc_attribute.html
>>
>> I saw wget2 removed the nonnull attribute due to the optimizer removing checks against NULL too
>> https://gitlab.com/gnuwget/wget2/-/issues/200
>>
>>>> Then the example may just silently continue to run, instead of throwing
>>>> an exception.  It would be an ironic example: an attempt to improve
>>>> diagnostic finally made diagnostic more difficult.
>>>
>>> Indeed.
>>>
>>> Maybe we can add __attribute__((access(read, 1))) instead, which says
>>> that we will read from the pointer, which also implies it must be
>>> non-null.
>>
>> I tried this with gcc 12, as read_only, but it didn't stop when compiling. Maybe you have an example that demonstrates please?
>>
>> void f(const char * p) __attribute__((access(read_only, 1)));
>>
>>>
>>> N.B. in C++23 string(nullptr) produces an error, although
>>> string((const char*)nullptr) doesn't, so in practice it only prevents
>>> the dumbest calls with a literal 'nullptr' token, and not the more
>>> realistic problems where you have a pointer that happens to be null.
>>
>> That's good it stops compiling, the error is not that clear "use of deleted function" for me though.
>>
>> string.cpp: In function ‘int main()’:
>> string.cpp:13:26: error: use of deleted function ‘std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::basic_string(std::nullptr_t) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::nullptr_t = std::nullptr_t]’
>>    13 |     std::string c(nullptr);
>>
>>
>>
>>
>> I made my own test class str_string which stops the build a different way. It only works if the dumbest calls with 'nullptr' as you found in your test.
>>
>> void nullptr_compile_abort() __attribute__((error("nullptr compile error")));
>>
>> str_string(nullptr_t) { nullptr_compile_abort(); }
> 
> This doesn't work because std::is_constructible_v<std::string,
> std::nullptr_t> would be true, and we want it to be false.

Hmm, for me, this output is 0.
  std::cout << std::is_constructible_v<std::string,std::nullptr_t> << "\n";


Sharing my example, gives compile error for 0, nullptr but not NULL (only for dumb direct calls) :

// g++ -std=c++23 -Wall -O1 -o string3 string3.cpp

#include <iterator>
#include <string>
void nullptr_compile_abort() __attribute__((error("nullptr compile error")));

class str_string {
public:
    str_string(nullptr_t) { nullptr_compile_abort(); }
    str_string(int) { nullptr_compile_abort(); }
    str_string(void *) { nullptr_compile_abort(); }
};

int main() {  str_string y(nullptr); }

>>
>>
>>  g++ -std=c++23 -Wall -O1 -o string2 string2.cpp
>> In constructor ‘str_string::str_string(nullptr_t)’,
>>     inlined from ‘int main()’ at string2.cpp:48:25:
>> string2.cpp:20:50: error: call to ‘nullptr_compile_abort’ declared with attribute error: nullptr compile error
>>    20 |     str_string(nullptr_t) { nullptr_compile_abort(); }
>>
>> Jonny


Maybe C++ guidelines not_null is a better approach to prevent construction? I've not tried it yet.
Jonny

Re: std::string add nullptr attribute

[Jonathan Wakely]

On Fri, 10 Feb 2023 at 22:38, Jonny Grant <jg@jguk.org> wrote:
>
>
>
> On 10/02/2023 22:03, Jonathan Wakely wrote:
> > On Fri, 10 Feb 2023 at 21:30, Jonny Grant <jg@jguk.org> wrote:
> >>
> >>
> >>
> >> On 09/02/2023 17:52, Jonathan Wakely wrote:
> >>> On Thu, 9 Feb 2023 at 16:30, Xi Ruoyao wrote:
> >>>>
> >>>> On Thu, 2023-02-09 at 14:56 +0000, Jonathan Wakely via Gcc-help wrote:
> >>>>>> Note, my code isn't like this, it is just an example to suggest
> >>>>>> adding the nullptr attribute, as its clearly already rejected at
> >>>>>> runtime.
> >>>>>
> >>>>> I assume you mean the nonnull attribute. That was added in 2020 and
> >>>>> then reverted because it broke some things:
> >>>>
> >>>> I remember I'd once made the same mistake when I suggested to add
> >>>> nonnull for ostream::operator<<(const string &) and I was lectured:
> >>>> nonnull is not only a diagnostic attribute, it also allows the compiler
> >>>> to assume the parameter is never null and rendering std::string(nullptr)
> >>>> an undefined behavior.
> >>>
> >>> Yes, I think that's what might have happened with the std::string change.
> >>
> >> My apologies, Jonathan, Xi, yes it is the __attribute__((nonnull)); I was mistaken to type as nullptr.
> >>
> >> I re-read, and it does seem nonnull is really an optimization that as a side effect may give some warnings. So I'm going to stop using it.
> >> https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
> >>
> >> (there is a typo in that manual section saying "nonnul" - I don't know if you have a moment to make a change in git? I didn't get replies on gcc-patches to my patches...)
> >>
> >> I searched and see like someone investigated this problem and saw it removed NULL checks http://www.rkoucha.fr/tech_corner/nonnull_gcc_attribute.html
> >>
> >> I saw wget2 removed the nonnull attribute due to the optimizer removing checks against NULL too
> >> https://gitlab.com/gnuwget/wget2/-/issues/200
> >>
> >>>> Then the example may just silently continue to run, instead of throwing
> >>>> an exception.  It would be an ironic example: an attempt to improve
> >>>> diagnostic finally made diagnostic more difficult.
> >>>
> >>> Indeed.
> >>>
> >>> Maybe we can add __attribute__((access(read, 1))) instead, which says
> >>> that we will read from the pointer, which also implies it must be
> >>> non-null.
> >>
> >> I tried this with gcc 12, as read_only, but it didn't stop when compiling. Maybe you have an example that demonstrates please?
> >>
> >> void f(const char * p) __attribute__((access(read_only, 1)));
> >>
> >>>
> >>> N.B. in C++23 string(nullptr) produces an error, although
> >>> string((const char*)nullptr) doesn't, so in practice it only prevents
> >>> the dumbest calls with a literal 'nullptr' token, and not the more
> >>> realistic problems where you have a pointer that happens to be null.
> >>
> >> That's good it stops compiling, the error is not that clear "use of deleted function" for me though.
> >>
> >> string.cpp: In function ‘int main()’:
> >> string.cpp:13:26: error: use of deleted function ‘std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::basic_string(std::nullptr_t) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::nullptr_t = std::nullptr_t]’
> >>    13 |     std::string c(nullptr);
> >>
> >>
> >>
> >>
> >> I made my own test class str_string which stops the build a different way. It only works if the dumbest calls with 'nullptr' as you found in your test.
> >>
> >> void nullptr_compile_abort() __attribute__((error("nullptr compile error")));
> >>
> >> str_string(nullptr_t) { nullptr_compile_abort(); }
> >
> > This doesn't work because std::is_constructible_v<std::string,
> > std::nullptr_t> would be true, and we want it to be false.
>
> Hmm, for me, this output is 0.
>   std::cout << std::is_constructible_v<std::string,std::nullptr_t> << "\n";

For C++23, yes, but if you add a constructor like your
str_string(nullptr_t) it would become 1.

Using a deleted function is observably different to using a
constructor that then produces an error when called.

Re: std::string add nullptr attribute

[Jonny Grant]

On 11/02/2023 00:32, Jonathan Wakely wrote:
> On Fri, 10 Feb 2023 at 22:38, Jonny Grant <jg@jguk.org> wrote:
>>
>>
>>
>> On 10/02/2023 22:03, Jonathan Wakely wrote:
>>> On Fri, 10 Feb 2023 at 21:30, Jonny Grant <jg@jguk.org> wrote:
>>>>
>>>>
>>>>
>>>> On 09/02/2023 17:52, Jonathan Wakely wrote:
>>>>> On Thu, 9 Feb 2023 at 16:30, Xi Ruoyao wrote:
>>>>>>
>>>>>> On Thu, 2023-02-09 at 14:56 +0000, Jonathan Wakely via Gcc-help wrote:
>>>>>>>> Note, my code isn't like this, it is just an example to suggest
>>>>>>>> adding the nullptr attribute, as its clearly already rejected at
>>>>>>>> runtime.
>>>>>>>
>>>>>>> I assume you mean the nonnull attribute. That was added in 2020 and
>>>>>>> then reverted because it broke some things:
>>>>>>
>>>>>> I remember I'd once made the same mistake when I suggested to add
>>>>>> nonnull for ostream::operator<<(const string &) and I was lectured:
>>>>>> nonnull is not only a diagnostic attribute, it also allows the compiler
>>>>>> to assume the parameter is never null and rendering std::string(nullptr)
>>>>>> an undefined behavior.
>>>>>
>>>>> Yes, I think that's what might have happened with the std::string change.
>>>>
>>>> My apologies, Jonathan, Xi, yes it is the __attribute__((nonnull)); I was mistaken to type as nullptr.
>>>>
>>>> I re-read, and it does seem nonnull is really an optimization that as a side effect may give some warnings. So I'm going to stop using it.
>>>> https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
>>>>
>>>> (there is a typo in that manual section saying "nonnul" - I don't know if you have a moment to make a change in git? I didn't get replies on gcc-patches to my patches...)
>>>>
>>>> I searched and see like someone investigated this problem and saw it removed NULL checks http://www.rkoucha.fr/tech_corner/nonnull_gcc_attribute.html
>>>>
>>>> I saw wget2 removed the nonnull attribute due to the optimizer removing checks against NULL too
>>>> https://gitlab.com/gnuwget/wget2/-/issues/200
>>>>
>>>>>> Then the example may just silently continue to run, instead of throwing
>>>>>> an exception.  It would be an ironic example: an attempt to improve
>>>>>> diagnostic finally made diagnostic more difficult.
>>>>>
>>>>> Indeed.
>>>>>
>>>>> Maybe we can add __attribute__((access(read, 1))) instead, which says
>>>>> that we will read from the pointer, which also implies it must be
>>>>> non-null.
>>>>
>>>> I tried this with gcc 12, as read_only, but it didn't stop when compiling. Maybe you have an example that demonstrates please?
>>>>
>>>> void f(const char * p) __attribute__((access(read_only, 1)));
>>>>
>>>>>
>>>>> N.B. in C++23 string(nullptr) produces an error, although
>>>>> string((const char*)nullptr) doesn't, so in practice it only prevents
>>>>> the dumbest calls with a literal 'nullptr' token, and not the more
>>>>> realistic problems where you have a pointer that happens to be null.
>>>>
>>>> That's good it stops compiling, the error is not that clear "use of deleted function" for me though.
>>>>
>>>> string.cpp: In function ‘int main()’:
>>>> string.cpp:13:26: error: use of deleted function ‘std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::basic_string(std::nullptr_t) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::nullptr_t = std::nullptr_t]’
>>>>    13 |     std::string c(nullptr);
>>>>
>>>>
>>>>
>>>>
>>>> I made my own test class str_string which stops the build a different way. It only works if the dumbest calls with 'nullptr' as you found in your test.
>>>>
>>>> void nullptr_compile_abort() __attribute__((error("nullptr compile error")));
>>>>
>>>> str_string(nullptr_t) { nullptr_compile_abort(); }
>>>
>>> This doesn't work because std::is_constructible_v<std::string,
>>> std::nullptr_t> would be true, and we want it to be false.
>>
>> Hmm, for me, this output is 0.
>>   std::cout << std::is_constructible_v<std::string,std::nullptr_t> << "\n";
> 
> For C++23, yes, but if you add a constructor like your
> str_string(nullptr_t) it would become 1.
> 
> Using a deleted function is observably different to using a
> constructor that then produces an error when called.

Indeed.

May I ask if you found a way to get the read_only attribute to trigger a build warning for nullptr?

It seems like only runtime checks can catch most of them for the moment.
Jonny

Re: std::string add nullptr attribute

[Jonny Grant]

On 11/02/2023 00:32, Jonathan Wakely wrote:
> On Fri, 10 Feb 2023 at 22:38, Jonny Grant <jg@jguk.org> wrote:
>>
>>
>>
>> On 10/02/2023 22:03, Jonathan Wakely wrote:
>>> On Fri, 10 Feb 2023 at 21:30, Jonny Grant <jg@jguk.org> wrote:
>>>>
>>>>
>>>>
>>>> On 09/02/2023 17:52, Jonathan Wakely wrote:
>>>>> On Thu, 9 Feb 2023 at 16:30, Xi Ruoyao wrote:
>>>>>>
>>>>>> On Thu, 2023-02-09 at 14:56 +0000, Jonathan Wakely via Gcc-help wrote:
>>>>>>>> Note, my code isn't like this, it is just an example to suggest
>>>>>>>> adding the nullptr attribute, as its clearly already rejected at
>>>>>>>> runtime.
>>>>>>>
>>>>>>> I assume you mean the nonnull attribute. That was added in 2020 and
>>>>>>> then reverted because it broke some things:
>>>>>>
>>>>>> I remember I'd once made the same mistake when I suggested to add
>>>>>> nonnull for ostream::operator<<(const string &) and I was lectured:
>>>>>> nonnull is not only a diagnostic attribute, it also allows the compiler
>>>>>> to assume the parameter is never null and rendering std::string(nullptr)
>>>>>> an undefined behavior.
>>>>>
>>>>> Yes, I think that's what might have happened with the std::string change.
>>>>
>>>> My apologies, Jonathan, Xi, yes it is the __attribute__((nonnull)); I was mistaken to type as nullptr.
>>>>
>>>> I re-read, and it does seem nonnull is really an optimization that as a side effect may give some warnings. So I'm going to stop using it.
>>>> https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
>>>>
>>>> (there is a typo in that manual section saying "nonnul" - I don't know if you have a moment to make a change in git? I didn't get replies on gcc-patches to my patches...)
>>>>
>>>> I searched and see like someone investigated this problem and saw it removed NULL checks http://www.rkoucha.fr/tech_corner/nonnull_gcc_attribute.html
>>>>
>>>> I saw wget2 removed the nonnull attribute due to the optimizer removing checks against NULL too
>>>> https://gitlab.com/gnuwget/wget2/-/issues/200
>>>>
>>>>>> Then the example may just silently continue to run, instead of throwing
>>>>>> an exception.  It would be an ironic example: an attempt to improve
>>>>>> diagnostic finally made diagnostic more difficult.
>>>>>
>>>>> Indeed.
>>>>>
>>>>> Maybe we can add __attribute__((access(read, 1))) instead, which says
>>>>> that we will read from the pointer, which also implies it must be
>>>>> non-null.
>>>>
>>>> I tried this with gcc 12, as read_only, but it didn't stop when compiling. Maybe you have an example that demonstrates please?
>>>>
>>>> void f(const char * p) __attribute__((access(read_only, 1)));
>>>>
>>>>>
>>>>> N.B. in C++23 string(nullptr) produces an error, although
>>>>> string((const char*)nullptr) doesn't, so in practice it only prevents
>>>>> the dumbest calls with a literal 'nullptr' token, and not the more
>>>>> realistic problems where you have a pointer that happens to be null.
>>>>
>>>> That's good it stops compiling, the error is not that clear "use of deleted function" for me though.
>>>>
>>>> string.cpp: In function ‘int main()’:
>>>> string.cpp:13:26: error: use of deleted function ‘std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::basic_string(std::nullptr_t) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::nullptr_t = std::nullptr_t]’
>>>>    13 |     std::string c(nullptr);
>>>>
>>>>
>>>>
>>>>
>>>> I made my own test class str_string which stops the build a different way. It only works if the dumbest calls with 'nullptr' as you found in your test.
>>>>
>>>> void nullptr_compile_abort() __attribute__((error("nullptr compile error")));
>>>>
>>>> str_string(nullptr_t) { nullptr_compile_abort(); }
>>>
>>> This doesn't work because std::is_constructible_v<std::string,
>>> std::nullptr_t> would be true, and we want it to be false.
>>
>> Hmm, for me, this output is 0.
>>   std::cout << std::is_constructible_v<std::string,std::nullptr_t> << "\n";
> 
> For C++23, yes, but if you add a constructor like your
> str_string(nullptr_t) it would become 1.
> 
> Using a deleted function is observably different to using a
> constructor that then produces an error when called.

I noticed -Wanalyzer-null-dereference reports at build time a dereference. Also works if a function parameter. I wondered why std::string isn't detected by this static analyser option.

<source>:9:10: warning: dereference of NULL '0' [CWE-476] [-Wanalyzer-null-dereference]
    9 |     char b = *a;
      |          ^

#include <string>
#include <iostream>

int main()
{
    const char * a = nullptr;
    char b = *a;

    std::cout << b;
}

Re: std::string add nullptr attribute

[Jonny Grant]

On 19/02/2023 20:43, Jonny Grant wrote:
> 
> 
> On 11/02/2023 00:32, Jonathan Wakely wrote:
>> On Fri, 10 Feb 2023 at 22:38, Jonny Grant <jg@jguk.org> wrote:
>>>
>>>
>>>
>>> On 10/02/2023 22:03, Jonathan Wakely wrote:
>>>> On Fri, 10 Feb 2023 at 21:30, Jonny Grant <jg@jguk.org> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 09/02/2023 17:52, Jonathan Wakely wrote:
>>>>>> On Thu, 9 Feb 2023 at 16:30, Xi Ruoyao wrote:
>>>>>>>
>>>>>>> On Thu, 2023-02-09 at 14:56 +0000, Jonathan Wakely via Gcc-help wrote:
>>>>>>>>> Note, my code isn't like this, it is just an example to suggest
>>>>>>>>> adding the nullptr attribute, as its clearly already rejected at
>>>>>>>>> runtime.
>>>>>>>>
>>>>>>>> I assume you mean the nonnull attribute. That was added in 2020 and
>>>>>>>> then reverted because it broke some things:
>>>>>>>
>>>>>>> I remember I'd once made the same mistake when I suggested to add
>>>>>>> nonnull for ostream::operator<<(const string &) and I was lectured:
>>>>>>> nonnull is not only a diagnostic attribute, it also allows the compiler
>>>>>>> to assume the parameter is never null and rendering std::string(nullptr)
>>>>>>> an undefined behavior.
>>>>>>
>>>>>> Yes, I think that's what might have happened with the std::string change.
>>>>>
>>>>> My apologies, Jonathan, Xi, yes it is the __attribute__((nonnull)); I was mistaken to type as nullptr.
>>>>>
>>>>> I re-read, and it does seem nonnull is really an optimization that as a side effect may give some warnings. So I'm going to stop using it.
>>>>> https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
>>>>>
>>>>> (there is a typo in that manual section saying "nonnul" - I don't know if you have a moment to make a change in git? I didn't get replies on gcc-patches to my patches...)
>>>>>
>>>>> I searched and see like someone investigated this problem and saw it removed NULL checks http://www.rkoucha.fr/tech_corner/nonnull_gcc_attribute.html
>>>>>
>>>>> I saw wget2 removed the nonnull attribute due to the optimizer removing checks against NULL too
>>>>> https://gitlab.com/gnuwget/wget2/-/issues/200
>>>>>
>>>>>>> Then the example may just silently continue to run, instead of throwing
>>>>>>> an exception.  It would be an ironic example: an attempt to improve
>>>>>>> diagnostic finally made diagnostic more difficult.
>>>>>>
>>>>>> Indeed.
>>>>>>
>>>>>> Maybe we can add __attribute__((access(read, 1))) instead, which says
>>>>>> that we will read from the pointer, which also implies it must be
>>>>>> non-null.
>>>>>
>>>>> I tried this with gcc 12, as read_only, but it didn't stop when compiling. Maybe you have an example that demonstrates please?
>>>>>
>>>>> void f(const char * p) __attribute__((access(read_only, 1)));
>>>>>
>>>>>>
>>>>>> N.B. in C++23 string(nullptr) produces an error, although
>>>>>> string((const char*)nullptr) doesn't, so in practice it only prevents
>>>>>> the dumbest calls with a literal 'nullptr' token, and not the more
>>>>>> realistic problems where you have a pointer that happens to be null.
>>>>>
>>>>> That's good it stops compiling, the error is not that clear "use of deleted function" for me though.
>>>>>
>>>>> string.cpp: In function ‘int main()’:
>>>>> string.cpp:13:26: error: use of deleted function ‘std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::basic_string(std::nullptr_t) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::nullptr_t = std::nullptr_t]’
>>>>>    13 |     std::string c(nullptr);
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I made my own test class str_string which stops the build a different way. It only works if the dumbest calls with 'nullptr' as you found in your test.
>>>>>
>>>>> void nullptr_compile_abort() __attribute__((error("nullptr compile error")));
>>>>>
>>>>> str_string(nullptr_t) { nullptr_compile_abort(); }
>>>>
>>>> This doesn't work because std::is_constructible_v<std::string,
>>>> std::nullptr_t> would be true, and we want it to be false.
>>>
>>> Hmm, for me, this output is 0.
>>>   std::cout << std::is_constructible_v<std::string,std::nullptr_t> << "\n";
>>
>> For C++23, yes, but if you add a constructor like your
>> str_string(nullptr_t) it would become 1.
>>
>> Using a deleted function is observably different to using a
>> constructor that then produces an error when called.
> 
> I noticed -Wanalyzer-null-dereference reports at build time a dereference. Also works if a function parameter. I wondered why std::string isn't detected by this static analyser option.
> 
> <source>:9:10: warning: dereference of NULL '0' [CWE-476] [-Wanalyzer-null-dereference]
>     9 |     char b = *a;
>       |          ^
> 
> #include <string>
> #include <iostream>
> 
> int main()
> {
>     const char * a = nullptr;
>     char b = *a;
> 
>     std::cout << b;
> }



It's not pretty, but this wrapper catches NULL passed at compile time:

std::string make_std_string(const char * const str)
{
    // This line ensures: warning: dereference of NULL '0' [CWE-476] [-Wanalyzer-null-dereference]
    char b = *str; 
    std::string s(str);
    s[0] = b; // copy it back to avoid unused variable warning
    return s;
}

int main()
{
    const char * a = NULL;
    std::string result = make_std_string(a);
    std::cout << result;
}


note, there a PR in latest gcc for an issue, so need to use -Wno-analyzer-use-of-uninitialized-value to avoid std::string having an incorrect warning reported.

Re: std::string add nullptr attribute

[Xi Ruoyao]

On Sun, 2023-02-19 at 21:33 +0000, Jonny Grant wrote:

> I noticed -Wanalyzer-null-dereference reports at build time a
> dereference. Also works if a function parameter. I wondered why
> std::string isn't detected by this static analyser option.

Because the analyzer does not know the C++ standard disallows to use
NULL here.  It just analyzes the code.  The code in libstdc++ reads:

      basic_string(const _CharT* __s, const _Alloc& __a = _Alloc())
      : _M_dataplus(_M_local_data(), __a)
      {    
        // NB: Not required, but considered best practice.
        if (__s == 0)
          std::__throw_logic_error(__N("basic_string: "
                                       "construction from null is not valid"));
        const _CharT* __end = __s + traits_type::length(__s);
        _M_construct(__s, __end, forward_iterator_tag());
      }  

As you can see yourself, though the standard implies using NULL here is
a UB, libstdc++ does not really code a UB here.  So the analyzer will
consider the code absolutely valid.

Note that throwing a C++ exception is not a programming error.  It's
perfectly legal to catch the exception elsewhere.  It's also perfectly
legal not to catch it and treat it as an abort() (calling abort is also
not a programming error).


> It's not pretty, but this wrapper catches NULL passed at compile time:
> 
> std::string make_std_string(const char * const str)
> {
>     // This line ensures: warning: dereference of NULL '0' [CWE-476]
> [-Wanalyzer-null-dereference]
>     char b = *str; 

You are invoking an undefined behavior here if str is NULL, so it's
essentially same as using a nonnull attribute for make_std_string.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: std::string add nullptr attribute

[Jonathan Wakely]

On Mon, 20 Feb 2023 at 10:26, Xi Ruoyao <xry111@xry111.site> wrote:
>
> On Sun, 2023-02-19 at 21:33 +0000, Jonny Grant wrote:
>
> > I noticed -Wanalyzer-null-dereference reports at build time a
> > dereference. Also works if a function parameter. I wondered why
> > std::string isn't detected by this static analyser option.
>
> Because the analyzer does not know the C++ standard disallows to use
> NULL here.  It just analyzes the code.  The code in libstdc++ reads:
>
>       basic_string(const _CharT* __s, const _Alloc& __a = _Alloc())
>       : _M_dataplus(_M_local_data(), __a)
>       {
>         // NB: Not required, but considered best practice.
>         if (__s == 0)
>           std::__throw_logic_error(__N("basic_string: "
>                                        "construction from null is not valid"));
>         const _CharT* __end = __s + traits_type::length(__s);
>         _M_construct(__s, __end, forward_iterator_tag());
>       }
>
> As you can see yourself, though the standard implies using NULL here is
> a UB, libstdc++ does not really code a UB here.  So the analyzer will
> consider the code absolutely valid.

Right, it's defined behaviour in libstdc++, as an extension.

>
> Note that throwing a C++ exception is not a programming error.  It's
> perfectly legal to catch the exception elsewhere.  It's also perfectly
> legal not to catch it and treat it as an abort() (calling abort is also
> not a programming error).
>
>
> > It's not pretty, but this wrapper catches NULL passed at compile time:
> >
> > std::string make_std_string(const char * const str)
> > {
> >     // This line ensures: warning: dereference of NULL '0' [CWE-476]
> > [-Wanalyzer-null-dereference]
> >     char b = *str;
>
> You are invoking an undefined behavior here if str is NULL, so it's
> essentially same as using a nonnull attribute for make_std_string.

And turned defined behaviour back into UB. The warning isn't reliable
(only if the compiler can see the point is null, which isn't the case
without optimization, or if the pointer comes from some non-inline
function), the exception is. You're trading guaranteed exception for a
not guaranteed warning and unbounded misoptimization due to undefined
behaviour.

Even if this was a robust solution, is it really a problem that needs
to be solved?

Re: std::string add nullptr attribute

[Xi Ruoyao]

On Mon, 2023-02-20 at 10:37 +0000, Jonathan Wakely wrote:
> On Mon, 20 Feb 2023 at 10:26, Xi Ruoyao <xry111@xry111.site> wrote:
> > 
> > On Sun, 2023-02-19 at 21:33 +0000, Jonny Grant wrote:
> > 
> > > I noticed -Wanalyzer-null-dereference reports at build time a
> > > dereference. Also works if a function parameter. I wondered why
> > > std::string isn't detected by this static analyser option.
> > 
> > Because the analyzer does not know the C++ standard disallows to use
> > NULL here.  It just analyzes the code.  The code in libstdc++ reads:
> > 
> >       basic_string(const _CharT* __s, const _Alloc& __a = _Alloc())
> >       : _M_dataplus(_M_local_data(), __a)
> >       {
> >         // NB: Not required, but considered best practice.
> >         if (__s == 0)
> >           std::__throw_logic_error(__N("basic_string: "
> >                                        "construction from null is not valid"));
> >         const _CharT* __end = __s + traits_type::length(__s);
> >         _M_construct(__s, __end, forward_iterator_tag());
> >       }
> > 
> > As you can see yourself, though the standard implies using NULL here is
> > a UB, libstdc++ does not really code a UB here.  So the analyzer will
> > consider the code absolutely valid.
> 
> Right, it's defined behaviour in libstdc++, as an extension.
> 
> > 
> > Note that throwing a C++ exception is not a programming error.  It's
> > perfectly legal to catch the exception elsewhere.  It's also perfectly
> > legal not to catch it and treat it as an abort() (calling abort is also
> > not a programming error).
> > 
> > 
> > > It's not pretty, but this wrapper catches NULL passed at compile time:
> > > 
> > > std::string make_std_string(const char * const str)
> > > {
> > >     // This line ensures: warning: dereference of NULL '0' [CWE-476]
> > > [-Wanalyzer-null-dereference]
> > >     char b = *str;
> > 
> > You are invoking an undefined behavior here if str is NULL, so it's
> > essentially same as using a nonnull attribute for make_std_string.
> 
> And turned defined behaviour back into UB. The warning isn't reliable
> (only if the compiler can see the point is null, which isn't the case
> without optimization, or if the pointer comes from some non-inline
> function), the exception is. You're trading guaranteed exception for a
> not guaranteed warning and unbounded misoptimization due to undefined
> behaviour.

Well, maybe we should have a warning here with -Wpedantic (or something)
as the standard does not allow people to pass NULL and expect a
logic_error.  But "deliberately making a UB to raise the warning" is not
good.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: std::string add nullptr attribute

[Gabriel Ravier]

On 2/20/23 11:54, Xi Ruoyao via Gcc-help wrote:
> On Mon, 2023-02-20 at 10:37 +0000, Jonathan Wakely wrote:
>> On Mon, 20 Feb 2023 at 10:26, Xi Ruoyao <xry111@xry111.site> wrote:
>>> On Sun, 2023-02-19 at 21:33 +0000, Jonny Grant wrote:
>>>
>>>> I noticed -Wanalyzer-null-dereference reports at build time a
>>>> dereference. Also works if a function parameter. I wondered why
>>>> std::string isn't detected by this static analyser option.
>>> Because the analyzer does not know the C++ standard disallows to use
>>> NULL here.  It just analyzes the code.  The code in libstdc++ reads:
>>>
>>>        basic_string(const _CharT* __s, const _Alloc& __a = _Alloc())
>>>        : _M_dataplus(_M_local_data(), __a)
>>>        {
>>>          // NB: Not required, but considered best practice.
>>>          if (__s == 0)
>>>            std::__throw_logic_error(__N("basic_string: "
>>>                                         "construction from null is not valid"));
>>>          const _CharT* __end = __s + traits_type::length(__s);
>>>          _M_construct(__s, __end, forward_iterator_tag());
>>>        }
>>>
>>> As you can see yourself, though the standard implies using NULL here is
>>> a UB, libstdc++ does not really code a UB here.  So the analyzer will
>>> consider the code absolutely valid.
>> Right, it's defined behaviour in libstdc++, as an extension.
>>
>>> Note that throwing a C++ exception is not a programming error.  It's
>>> perfectly legal to catch the exception elsewhere.  It's also perfectly
>>> legal not to catch it and treat it as an abort() (calling abort is also
>>> not a programming error).
>>>
>>>
>>>> It's not pretty, but this wrapper catches NULL passed at compile time:
>>>>
>>>> std::string make_std_string(const char * const str)
>>>> {
>>>>      // This line ensures: warning: dereference of NULL '0' [CWE-476]
>>>> [-Wanalyzer-null-dereference]
>>>>      char b = *str;
>>> You are invoking an undefined behavior here if str is NULL, so it's
>>> essentially same as using a nonnull attribute for make_std_string.
>> And turned defined behaviour back into UB. The warning isn't reliable
>> (only if the compiler can see the point is null, which isn't the case
>> without optimization, or if the pointer comes from some non-inline
>> function), the exception is. You're trading guaranteed exception for a
>> not guaranteed warning and unbounded misoptimization due to undefined
>> behaviour.
> Well, maybe we should have a warning here with -Wpedantic (or something)
> as the standard does not allow people to pass NULL and expect a
> logic_error.  But "deliberately making a UB to raise the warning" is not
> good.

This is the kind of thing that makes me wonder why there isn't some kind 
of `__builtin_unreachable_do_not_optimize()` builtin that allows one to 
mark places in code that should never be reached and should thus be 
warned about if such a thing happens while at the same time never doing 
any optimization on the basis of the presence of the call.

Re: std::string add nullptr attribute

[Marc Glisse]

On Mon, 20 Feb 2023, Gabriel Ravier via Gcc-help wrote:

> This is the kind of thing that makes me wonder why there isn't some kind of 
> `__builtin_unreachable_do_not_optimize()` builtin that allows one to mark 
> places in code that should never be reached and should thus be warned about 
> if such a thing happens while at the same time never doing any optimization 
> on the basis of the presence of the call.

-fsanitize=unreachable -fsanitize=null and others prevent the kind of 
optimization you are worried about.

-- 
Marc Glisse

Re: std::string add nullptr attribute

[Jonny Grant]

On 20/02/2023 10:26, Xi Ruoyao wrote:
> On Sun, 2023-02-19 at 21:33 +0000, Jonny Grant wrote:
> 
>> I noticed -Wanalyzer-null-dereference reports at build time a
>> dereference. Also works if a function parameter. I wondered why
>> std::string isn't detected by this static analyser option.
> 
> Because the analyzer does not know the C++ standard disallows to use
> NULL here.  It just analyzes the code.  The code in libstdc++ reads:
> 
>       basic_string(const _CharT* __s, const _Alloc& __a = _Alloc())
>       : _M_dataplus(_M_local_data(), __a)
>       {    
>         // NB: Not required, but considered best practice.
>         if (__s == 0)
>           std::__throw_logic_error(__N("basic_string: "
>                                        "construction from null is not valid"));
>         const _CharT* __end = __s + traits_type::length(__s);
>         _M_construct(__s, __end, forward_iterator_tag());
>       }  
> 
> As you can see yourself, though the standard implies using NULL here is
> a UB, libstdc++ does not really code a UB here.  So the analyzer will
> consider the code absolutely valid.

Thank you for your reply.

As you say, throwing logic_error seems rational if a NULL gets through to the constructor; if standard didn't imply creating an empty std::string when NULL was passed through.

> Note that throwing a C++ exception is not a programming error.  It's
> perfectly legal to catch the exception elsewhere.  It's also perfectly
> legal not to catch it and treat it as an abort() (calling abort is also
> not a programming error).
> 
> 
>> It's not pretty, but this wrapper catches NULL passed at compile time:
>>
>> std::string make_std_string(const char * const str)
>> {
>>     // This line ensures: warning: dereference of NULL '0' [CWE-476]
>> [-Wanalyzer-null-dereference]
>>     char b = *str; 
> 
> You are invoking an undefined behavior here if str is NULL, so it's
> essentially same as using a nonnull attribute for make_std_string.

Thank you for the suggestion, I gave that nonnull attribute a try, but it doesn't appear to warn for this example.

https://godbolt.org/z/boqTj6oWE

It should give a warning, as -fanalyzer enables -Wanalyzer-null-argument
https://gcc.gnu.org/onlinedocs/gcc/Static-Analyzer-Options.html


My preference would be to not have that char b = *str; maybe I would just do it within a macro enabled by a specific build 
Just to share my first example, with that char b = *str; inside a macro.
https://godbolt.org/z/9Wo6zY3rT

Kind regards
Jonny

Re: std::string add nullptr attribute

[Segher Boessenkool]

On Mon, Feb 20, 2023 at 12:18:36PM +0100, Marc Glisse via Gcc-help wrote:
> On Mon, 20 Feb 2023, Gabriel Ravier via Gcc-help wrote:
> 
> >This is the kind of thing that makes me wonder why there isn't some kind 
> >of `__builtin_unreachable_do_not_optimize()` builtin that allows one to 
> >mark places in code that should never be reached and should thus be warned 
> >about if such a thing happens while at the same time never doing any 
> >optimization on the basis of the presence of the call.
> 
> -fsanitize=unreachable -fsanitize=null and others prevent the kind of 
> optimization you are worried about.

Or even just __builtin_trap(), or abort(), or similar.  Just a printf()
thing if you really want to just warn.

"Never doing any optimisation" based on <anything> is of course not a
reasonable expectation; but you *can* ask for reachable code not to be
optimised away.  This is the default, just don't mark reachable code as
unreachable :-)


Segher

Re: std::string add nullptr attribute

[Jonny Grant]

On 20/02/2023 10:37, Jonathan Wakely wrote:
> On Mon, 20 Feb 2023 at 10:26, Xi Ruoyao <xry111@xry111.site> wrote:
>>
>> On Sun, 2023-02-19 at 21:33 +0000, Jonny Grant wrote:
>>
>>> I noticed -Wanalyzer-null-dereference reports at build time a
>>> dereference. Also works if a function parameter. I wondered why
>>> std::string isn't detected by this static analyser option.
>>
>> Because the analyzer does not know the C++ standard disallows to use
>> NULL here.  It just analyzes the code.  The code in libstdc++ reads:
>>
>>       basic_string(const _CharT* __s, const _Alloc& __a = _Alloc())
>>       : _M_dataplus(_M_local_data(), __a)
>>       {
>>         // NB: Not required, but considered best practice.
>>         if (__s == 0)
>>           std::__throw_logic_error(__N("basic_string: "
>>                                        "construction from null is not valid"));
>>         const _CharT* __end = __s + traits_type::length(__s);
>>         _M_construct(__s, __end, forward_iterator_tag());
>>       }
>>
>> As you can see yourself, though the standard implies using NULL here is
>> a UB, libstdc++ does not really code a UB here.  So the analyzer will
>> consider the code absolutely valid.
> 
> Right, it's defined behaviour in libstdc++, as an extension.
> 
>>
>> Note that throwing a C++ exception is not a programming error.  It's
>> perfectly legal to catch the exception elsewhere.  It's also perfectly
>> legal not to catch it and treat it as an abort() (calling abort is also
>> not a programming error).
>>
>>
>>> It's not pretty, but this wrapper catches NULL passed at compile time:
>>>
>>> std::string make_std_string(const char * const str)
>>> {
>>>     // This line ensures: warning: dereference of NULL '0' [CWE-476]
>>> [-Wanalyzer-null-dereference]
>>>     char b = *str;
>>
>> You are invoking an undefined behavior here if str is NULL, so it's
>> essentially same as using a nonnull attribute for make_std_string.
> 
> And turned defined behaviour back into UB. The warning isn't reliable
> (only if the compiler can see the point is null, which isn't the case
> without optimization, or if the pointer comes from some non-inline
> function), the exception is. You're trading guaranteed exception for a
> not guaranteed warning and unbounded misoptimization due to undefined
> behaviour.
> 
> Even if this was a robust solution, is it really a problem that needs
> to be solved?

Feels useful to get build warnings if compiler knows nullptr is going to be dereferenced, as clang does.

Personally I feel runtime should equally handle possible nullptr by constructing strings in a try catch block so any exceptions are handled or logged at least...

Personally I would be pleased if GCC had a warning I could enable to report any logic_error exceptions it knew would execute.

Regards, Jonny

Re: std::string add nullptr attribute

[Jonny Grant]

On 20/02/2023 11:10, Gabriel Ravier wrote:
> On 2/20/23 11:54, Xi Ruoyao via Gcc-help wrote:
>> On Mon, 2023-02-20 at 10:37 +0000, Jonathan Wakely wrote:
>>> On Mon, 20 Feb 2023 at 10:26, Xi Ruoyao <xry111@xry111.site> wrote:
>>>> On Sun, 2023-02-19 at 21:33 +0000, Jonny Grant wrote:
>>>>
>>>>> I noticed -Wanalyzer-null-dereference reports at build time a
>>>>> dereference. Also works if a function parameter. I wondered why
>>>>> std::string isn't detected by this static analyser option.
>>>> Because the analyzer does not know the C++ standard disallows to use
>>>> NULL here.  It just analyzes the code.  The code in libstdc++ reads:
>>>>
>>>>        basic_string(const _CharT* __s, const _Alloc& __a = _Alloc())
>>>>        : _M_dataplus(_M_local_data(), __a)
>>>>        {
>>>>          // NB: Not required, but considered best practice.
>>>>          if (__s == 0)
>>>>            std::__throw_logic_error(__N("basic_string: "
>>>>                                         "construction from null is not valid"));
>>>>          const _CharT* __end = __s + traits_type::length(__s);
>>>>          _M_construct(__s, __end, forward_iterator_tag());
>>>>        }
>>>>
>>>> As you can see yourself, though the standard implies using NULL here is
>>>> a UB, libstdc++ does not really code a UB here.  So the analyzer will
>>>> consider the code absolutely valid.
>>> Right, it's defined behaviour in libstdc++, as an extension.
>>>
>>>> Note that throwing a C++ exception is not a programming error.  It's
>>>> perfectly legal to catch the exception elsewhere.  It's also perfectly
>>>> legal not to catch it and treat it as an abort() (calling abort is also
>>>> not a programming error).
>>>>
>>>>
>>>>> It's not pretty, but this wrapper catches NULL passed at compile time:
>>>>>
>>>>> std::string make_std_string(const char * const str)
>>>>> {
>>>>>      // This line ensures: warning: dereference of NULL '0' [CWE-476]
>>>>> [-Wanalyzer-null-dereference]
>>>>>      char b = *str;
>>>> You are invoking an undefined behavior here if str is NULL, so it's
>>>> essentially same as using a nonnull attribute for make_std_string.
>>> And turned defined behaviour back into UB. The warning isn't reliable
>>> (only if the compiler can see the point is null, which isn't the case
>>> without optimization, or if the pointer comes from some non-inline
>>> function), the exception is. You're trading guaranteed exception for a
>>> not guaranteed warning and unbounded misoptimization due to undefined
>>> behaviour.
>> Well, maybe we should have a warning here with -Wpedantic (or something)
>> as the standard does not allow people to pass NULL and expect a
>> logic_error.  But "deliberately making a UB to raise the warning" is not
>> good.
> 
> This is the kind of thing that makes me wonder why there isn't some kind of `__builtin_unreachable_do_not_optimize()` builtin that allows one to mark places in code that should never be reached and should thus be warned about if such a thing happens while at the same time never doing any optimization on the basis of the presence of the call.
> 

That sounds really useful. So something would give a useful build warning?

<source>:6:8: warning: dereference of NULL '0' [CWE-476] [-Wanalyzer-null-dereference]
   12 |     char b = *str;

void f(const char * str)
{
    if(NULL == str)
    {
        __builtin_unreachable_do_not_optimize();
    }

    __builtin_printf(str);
}

Regards, Jonny

Re: std::string add nullptr attribute

[Jonny Grant]

On 20/02/2023 11:18, Marc Glisse wrote:
> On Mon, 20 Feb 2023, Gabriel Ravier via Gcc-help wrote:
> 
>> This is the kind of thing that makes me wonder why there isn't some kind of `__builtin_unreachable_do_not_optimize()` builtin that allows one to mark places in code that should never be reached and should thus be warned about if such a thing happens while at the same time never doing any optimization on the basis of the presence of the call.
> 
> -fsanitize=unreachable -fsanitize=null and others prevent the kind of optimization you are worried about.
> 

Unfortunately santizer is only at runtime. I'm seeking build time warnings. Sharing my example:
https://godbolt.org/z/c7b17nMGd

Execution build compiler returned: 0
Program returned: 139
/app/example.cpp:4:10: runtime error: load of null pointer of type 'const char'

Regards, Jonny

Re: std::string add nullptr attribute

[Jonny Grant]

On 20/02/2023 11:28, Segher Boessenkool wrote:
> On Mon, Feb 20, 2023 at 12:18:36PM +0100, Marc Glisse via Gcc-help wrote:
>> On Mon, 20 Feb 2023, Gabriel Ravier via Gcc-help wrote:
>>
>>> This is the kind of thing that makes me wonder why there isn't some kind 
>>> of `__builtin_unreachable_do_not_optimize()` builtin that allows one to 
>>> mark places in code that should never be reached and should thus be warned 
>>> about if such a thing happens while at the same time never doing any 
>>> optimization on the basis of the presence of the call.
>>
>> -fsanitize=unreachable -fsanitize=null and others prevent the kind of 
>> optimization you are worried about.
> 
> Or even just __builtin_trap(), or abort(), or similar.  Just a printf()
> thing if you really want to just warn.

__builtin_trap() gets compiled so there isn't a symbol (unless I wrap it)

        pushq   %rbp
        movq    %rsp, %rbp
        ud2


I made an example which does create a string with the file and line number (which would aid checking assembler output for null dereference issues), but oddly the optimizer doesn't remove the string, as that code is never reached.  I'm probably misunderstanding I guess.

.LC0:
        .string "/app/example.cpp:8"

https://godbolt.org/z/Yzqb5ov6M

#define xto_str(s) to_str(s)
#define to_str(s) #s

void f(const char * str)
{
    if(nullptr == str)
    {
        __builtin_printf(__FILE__ ":" xto_str(__LINE__));
    }

    __builtin_printf("%c", str[0]);
}

int main()
{
    const char * a = "a";
    f(a);
}



> 
> "Never doing any optimisation" based on <anything> is of course not a
> reasonable expectation; but you *can* ask for reachable code not to be
> optimised away.  This is the default, just don't mark reachable code as
> unreachable :-)
> 
> 
> Segher

Re: std::string add nullptr attribute

[Xi Ruoyao]

On Mon, 2023-02-20 at 11:30 +0000, Jonny Grant wrote:

> Thank you for the suggestion, I gave that nonnull attribute a try, but
> it doesn't appear to warn for this example.
> 
> https://godbolt.org/z/boqTj6oWE

Ouch... The optimizer inlined make_std_string so both -Wnonnull and -
fanalyzer fails to catch the issue here.

Adding noipa attribute for make_std_string will work, but will also
cause the generated code stupidly slow.  Maybe:

#ifdef WANT_DIAGNOSTIC
#define MAKE_STD_STRING_ATTR __attribute__ ((noipa, nonnull))
#else
#define MAKE_STD_STRING_ATTR
#endif

std::string make_std_string(const char * const str) MAKE_STD_STRING_ATTR;

It still looks very stupid though.

> Feels useful to get build warnings if compiler knows nullptr is going
> to be dereferenced, as clang does.

The problem is in this case nullptr is not dereferenced, at all.  So if
we want a warning here we'll have to invent some new __builtin or
__attribute__ to give the compiler a hint.  AFAIK there is no such
facility now.

And you cannot simply justifying to add a new facility because "I feel
it useful".  Generally you need to show the benefit will be at least
equally great than the maintenance burden introduced into the GCC code
base.  And unfortunately usually we can only measure the burden after
really writing all the code...  So it's not easy to convince someone to
develop such a new feature.

> Personally I feel runtime should equally handle possible nullptr by
> constructing strings in a try catch block so any exceptions are
> handled or logged at least...

A portable runtime should not assume std::string(NULL) will raise an
exception because other C++ standard libraries may behave differently. 
The portable solution is to make a wrapper around std::string
constructor and check if the parameter is NULL.

> Personally I would be pleased if GCC had a warning I could enable to
> report any logic_error exceptions it knew would execute.

Or maybe when a program will definitely raise an uncatched exception. 
But is the feature really useful?  This will not happen for anything
other than simple toy programs.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: std::string add nullptr attribute

[Jonathan Wakely]

On Mon, 20 Feb 2023 at 12:59, Xi Ruoyao <xry111@xry111.site> wrote:
>
> On Mon, 2023-02-20 at 11:30 +0000, Jonny Grant wrote:
>
> > Thank you for the suggestion, I gave that nonnull attribute a try, but
> > it doesn't appear to warn for this example.
> >
> > https://godbolt.org/z/boqTj6oWE
>
> Ouch... The optimizer inlined make_std_string so both -Wnonnull and -
> fanalyzer fails to catch the issue here.
>
> Adding noipa attribute for make_std_string will work, but will also
> cause the generated code stupidly slow.  Maybe:
>
> #ifdef WANT_DIAGNOSTIC
> #define MAKE_STD_STRING_ATTR __attribute__ ((noipa, nonnull))
> #else
> #define MAKE_STD_STRING_ATTR
> #endif
>
> std::string make_std_string(const char * const str) MAKE_STD_STRING_ATTR;
>
> It still looks very stupid though.
>
> > Feels useful to get build warnings if compiler knows nullptr is going
> > to be dereferenced, as clang does.
>
> The problem is in this case nullptr is not dereferenced, at all.  So if
> we want a warning here we'll have to invent some new __builtin or
> __attribute__ to give the compiler a hint.  AFAIK there is no such
> facility now.

I think __attribute__((access(read_only, 1))) should do it.

Re: std::string add nullptr attribute

[Gabriel Ravier]

On 2/20/23 12:28, Segher Boessenkool wrote:
> On Mon, Feb 20, 2023 at 12:18:36PM +0100, Marc Glisse via Gcc-help wrote:
>> On Mon, 20 Feb 2023, Gabriel Ravier via Gcc-help wrote:
>>
>>> This is the kind of thing that makes me wonder why there isn't some kind
>>> of `__builtin_unreachable_do_not_optimize()` builtin that allows one to
>>> mark places in code that should never be reached and should thus be warned
>>> about if such a thing happens while at the same time never doing any
>>> optimization on the basis of the presence of the call.
>> -fsanitize=unreachable -fsanitize=null and others prevent the kind of
>> optimization you are worried about.
> Or even just __builtin_trap(), or abort(), or similar.  Just a printf()
> thing if you really want to just warn.
>
> "Never doing any optimisation" based on <anything> is of course not a
> reasonable expectation; but you *can* ask for reachable code not to be
> optimised away.  This is the default, just don't mark reachable code as
> unreachable :-)
>
>
> Segher

What I mean is that it would be nice to have a builtin that has the same 
effect as a `*(char *)0 = 0` in that the compiler will warn you if it 
thinks code execution will reach it, except that it should do do nothing 
else. `*(char *)0 = 0` is a really bad solution in that regard because 
while it will result in the compiler emitting a warning, it will also 
result in actual undefined behavior that can crash the program or get 
optimized out. `__builtin_trap` and `abort` are also equally bad in that 
they result in visible behavior at runtime, which is equivalently bad if 
you solely want a warning and nothing else (also, as far as I know, GCC 
will not emit a warning on the basis that they are reached either, so 
they're completely useless if one's goal is to get a build-time warning).

Something that seems instead quite useful would be some kind of builtin 
that would result in a warning if GCC thinks code execution will reach 
it, while having absolutely no other effect on code generation or 
execution (i.e. no optimization, no trap, no abort, no nothing), which 
neither deliberate UB nor calls to functions like `abort` or 
`__builtin_trap` will result in.

Re: std::string add nullptr attribute

[Jonny Grant]

On 20/02/2023 13:44, Jonathan Wakely wrote:
> On Mon, 20 Feb 2023 at 12:59, Xi Ruoyao <xry111@xry111.site> wrote:
>>
>> On Mon, 2023-02-20 at 11:30 +0000, Jonny Grant wrote:
>>
>>> Thank you for the suggestion, I gave that nonnull attribute a try, but
>>> it doesn't appear to warn for this example.
>>>
>>> https://godbolt.org/z/boqTj6oWE
>>
>> Ouch... The optimizer inlined make_std_string so both -Wnonnull and -
>> fanalyzer fails to catch the issue here.
>>
>> Adding noipa attribute for make_std_string will work, but will also
>> cause the generated code stupidly slow.  Maybe:
>>
>> #ifdef WANT_DIAGNOSTIC
>> #define MAKE_STD_STRING_ATTR __attribute__ ((noipa, nonnull))
>> #else
>> #define MAKE_STD_STRING_ATTR
>> #endif
>>
>> std::string make_std_string(const char * const str) MAKE_STD_STRING_ATTR;
>>
>> It still looks very stupid though.
>>
>>> Feels useful to get build warnings if compiler knows nullptr is going
>>> to be dereferenced, as clang does.
>>
>> The problem is in this case nullptr is not dereferenced, at all.  So if
>> we want a warning here we'll have to invent some new __builtin or
>> __attribute__ to give the compiler a hint.  AFAIK there is no such
>> facility now.
> 
> I think __attribute__((access(read_only, 1))) should do it.

I tried here, but no luck, is this example the way you mean? gcc trunk.
https://godbolt.org/z/haq18bvPP

Kind regards
Jonny

Re: std::string add nullptr attribute

[Jonathan Wakely]

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

On Mon, 20 Feb 2023, 19:21 Jonny Grant, <jg@jguk.org> wrote:

>
>
> On 20/02/2023 13:44, Jonathan Wakely wrote:
> > On Mon, 20 Feb 2023 at 12:59, Xi Ruoyao <xry111@xry111.site> wrote:
> >>
> >> On Mon, 2023-02-20 at 11:30 +0000, Jonny Grant wrote:
> >>
> >>> Thank you for the suggestion, I gave that nonnull attribute a try, but
> >>> it doesn't appear to warn for this example.
> >>>
> >>> https://godbolt.org/z/boqTj6oWE
> >>
> >> Ouch... The optimizer inlined make_std_string so both -Wnonnull and -
> >> fanalyzer fails to catch the issue here.
> >>
> >> Adding noipa attribute for make_std_string will work, but will also
> >> cause the generated code stupidly slow.  Maybe:
> >>
> >> #ifdef WANT_DIAGNOSTIC
> >> #define MAKE_STD_STRING_ATTR __attribute__ ((noipa, nonnull))
> >> #else
> >> #define MAKE_STD_STRING_ATTR
> >> #endif
> >>
> >> std::string make_std_string(const char * const str)
> MAKE_STD_STRING_ATTR;
> >>
> >> It still looks very stupid though.
> >>
> >>> Feels useful to get build warnings if compiler knows nullptr is going
> >>> to be dereferenced, as clang does.
> >>
> >> The problem is in this case nullptr is not dereferenced, at all.  So if
> >> we want a warning here we'll have to invent some new __builtin or
> >> __attribute__ to give the compiler a hint.  AFAIK there is no such
> >> facility now.
> >
> > I think __attribute__((access(read_only, 1))) should do it.
>
> I tried here, but no luck, is this example the way you mean? gcc trunk.
>

I don't mean it does do it, I mean it should do it.


https://godbolt.org/z/haq18bvPP


>

Re: std::string add nullptr attribute

[Jonny Grant]

On 20/02/2023 19:35, Jonathan Wakely wrote:
> 
> 
> On Mon, 20 Feb 2023, 19:21 Jonny Grant, <jg@jguk.org <mailto:jg@jguk.org>> wrote:
> 
> 
> 
>     On 20/02/2023 13:44, Jonathan Wakely wrote:
>     > On Mon, 20 Feb 2023 at 12:59, Xi Ruoyao <xry111@xry111.site> wrote:
>     >>
>     >> On Mon, 2023-02-20 at 11:30 +0000, Jonny Grant wrote:
>     >>
>     >>> Thank you for the suggestion, I gave that nonnull attribute a try, but
>     >>> it doesn't appear to warn for this example.
>     >>>
>     >>> https://godbolt.org/z/boqTj6oWE <https://godbolt.org/z/boqTj6oWE>
>     >>
>     >> Ouch... The optimizer inlined make_std_string so both -Wnonnull and -
>     >> fanalyzer fails to catch the issue here.
>     >>
>     >> Adding noipa attribute for make_std_string will work, but will also
>     >> cause the generated code stupidly slow.  Maybe:
>     >>
>     >> #ifdef WANT_DIAGNOSTIC
>     >> #define MAKE_STD_STRING_ATTR __attribute__ ((noipa, nonnull))
>     >> #else
>     >> #define MAKE_STD_STRING_ATTR
>     >> #endif
>     >>
>     >> std::string make_std_string(const char * const str) MAKE_STD_STRING_ATTR;
>     >>
>     >> It still looks very stupid though.
>     >>
>     >>> Feels useful to get build warnings if compiler knows nullptr is going
>     >>> to be dereferenced, as clang does.
>     >>
>     >> The problem is in this case nullptr is not dereferenced, at all.  So if
>     >> we want a warning here we'll have to invent some new __builtin or
>     >> __attribute__ to give the compiler a hint.  AFAIK there is no such
>     >> facility now.
>     >
>     > I think __attribute__((access(read_only, 1))) should do it.
> 
>     I tried here, but no luck, is this example the way you mean? gcc trunk.
> 
> 
> I don't mean it does do it, I mean it should do it.

Ah ok, couldn't find a bug report. Did you already report it?
Jonny

Re: std::string add nullptr attribute

[Jonny Grant]

On 20/02/2023 11:18, Marc Glisse wrote:
> On Mon, 20 Feb 2023, Gabriel Ravier via Gcc-help wrote:
> 
>> This is the kind of thing that makes me wonder why there isn't some kind of `__builtin_unreachable_do_not_optimize()` builtin that allows one to mark places in code that should never be reached and should thus be warned about if such a thing happens while at the same time never doing any optimization on the basis of the presence of the call.
> 
> -fsanitize=unreachable -fsanitize=null and others prevent the kind of optimization you are worried about.
> 

Santizer is roughly 2-4x slower, but for simple applications that may be fine. 

https://developers.redhat.com/blog/2021/05/05/memory-error-checking-in-c-and-c-comparing-sanitizers-and-valgrind

Jonny

Re: std::string add nullptr attribute

[Jonny Grant]

On 20/02/2023 12:59, Xi Ruoyao wrote:
> On Mon, 2023-02-20 at 11:30 +0000, Jonny Grant wrote:
> 
>> Thank you for the suggestion, I gave that nonnull attribute a try, but
>> it doesn't appear to warn for this example.
>>
>> https://godbolt.org/z/boqTj6oWE
> 
> Ouch... The optimizer inlined make_std_string so both -Wnonnull and -
> fanalyzer fails to catch the issue here.
> 
> Adding noipa attribute for make_std_string will work, but will also
> cause the generated code stupidly slow.  Maybe:
> 
> #ifdef WANT_DIAGNOSTIC
> #define MAKE_STD_STRING_ATTR __attribute__ ((noipa, nonnull))
> #else
> #define MAKE_STD_STRING_ATTR
> #endif
> 
> std::string make_std_string(const char * const str) MAKE_STD_STRING_ATTR;
> 
> It still looks very stupid though.


Hi, thank you for your reply and investigation.
I raised this issue here that you spotted

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108871


> 
>> Feels useful to get build warnings if compiler knows nullptr is going
>> to be dereferenced, as clang does.
> 
> The problem is in this case nullptr is not dereferenced, at all.  So if
> we want a warning here we'll have to invent some new __builtin or
> __attribute__ to give the compiler a hint.  AFAIK there is no such
> facility now.
> 
> And you cannot simply justifying to add a new facility because "I feel
> it useful".  Generally you need to show the benefit will be at least
> equally great than the maintenance burden introduced into the GCC code
> base.  And unfortunately usually we can only measure the burden after
> really writing all the code...  So it's not easy to convince someone to
> develop such a new feature.
> 
>> Personally I feel runtime should equally handle possible nullptr by
>> constructing strings in a try catch block so any exceptions are
>> handled or logged at least...
> 
> A portable runtime should not assume std::string(NULL) will raise an
> exception because other C++ standard libraries may behave differently. 
> The portable solution is to make a wrapper around std::string
> constructor and check if the parameter is NULL.
> 
>> Personally I would be pleased if GCC had a warning I could enable to
>> report any logic_error exceptions it knew would execute.
> 
> Or maybe when a program will definitely raise an uncatched exception. 
> But is the feature really useful?  This will not happen for anything
> other than simple toy programs.
> 

Re: std::string add nullptr attribute

[Jonny Grant]

On 20/02/2023 12:59, Xi Ruoyao wrote:
> On Mon, 2023-02-20 at 11:30 +0000, Jonny Grant wrote:
> 
>> Thank you for the suggestion, I gave that nonnull attribute a try, but
>> it doesn't appear to warn for this example.
>>
>> https://godbolt.org/z/boqTj6oWE
> 
> Ouch... The optimizer inlined make_std_string so both -Wnonnull and -
> fanalyzer fails to catch the issue here.
> 
> Adding noipa attribute for make_std_string will work, but will also
> cause the generated code stupidly slow.  Maybe:
> 
> #ifdef WANT_DIAGNOSTIC
> #define MAKE_STD_STRING_ATTR __attribute__ ((noipa, nonnull))
> #else
> #define MAKE_STD_STRING_ATTR
> #endif
> 
> std::string make_std_string(const char * const str) MAKE_STD_STRING_ATTR;
> 
> It still looks very stupid though.
> 
>> Feels useful to get build warnings if compiler knows nullptr is going
>> to be dereferenced, as clang does.
> 
> The problem is in this case nullptr is not dereferenced, at all.  So if
> we want a warning here we'll have to invent some new __builtin or
> __attribute__ to give the compiler a hint.  AFAIK there is no such
> facility now.

I see, yes it is tricky, as many functions take NULL, like time(NULL) from <time.h>, need to actually dereference it to get the build warning!

that basic_string() constructor doesn't dereference the nullptr as it throws logic_error("construction from null is not valid")


So then I am back to putting my actual dereference in, just in that build where it is enabled

TRIGGER_NULLPTR_WARNING
https://godbolt.org/z/4h3MqGh1s

I added myassert() to catch it otherwise so I can at least get the file and line info rather than just the basic_string exception throw logic_error message


> And you cannot simply justifying to add a new facility because "I feel
> it useful".  Generally you need to show the benefit will be at least
> equally great than the maintenance burden introduced into the GCC code
> base.  And unfortunately usually we can only measure the burden after
> really writing all the code...  So it's not easy to convince someone to
> develop such a new feature.
> 
>> Personally I feel runtime should equally handle possible nullptr by
>> constructing strings in a try catch block so any exceptions are
>> handled or logged at least...
> 
> A portable runtime should not assume std::string(NULL) will raise an
> exception because other C++ standard libraries may behave differently. 
> The portable solution is to make a wrapper around std::string
> constructor and check if the parameter is NULL.

You're right, I shouldn't rely on this logic_error which maybe specific to GNU.
I made another example that wraps, and also catches exceptions
https://godbolt.org/z/ez3q76cj4

There was an issue as char_traits does __builtin_strlen() which doesn't check for nullptr so crashes, so I couldn't just do out_string = str;


>> Personally I would be pleased if GCC had a warning I could enable to
>> report any logic_error exceptions it knew would execute.
> 
> Or maybe when a program will definitely raise an uncatched exception. 
> But is the feature really useful?  This will not happen for anything
> other than simple toy programs.

That's a good point. I've seen C++ programs sometimes don't have good exception handling, some don't have any at all, it goes right back to the top and calls abort() when there's a std::out_of_range exception etc.

It would be good to have the opportunity to see a build warning when there would be an uncaught exception the optimizer knows about.

Kind regards
Jonny

 

Re: std::string add nullptr attribute

[Jonny Grant]

On 20/02/2023 19:35, Jonathan Wakely wrote:
> 
> 
> On Mon, 20 Feb 2023, 19:21 Jonny Grant, <jg@jguk.org <mailto:jg@jguk.org>> wrote:
> 
> 
> 
>     On 20/02/2023 13:44, Jonathan Wakely wrote:
>     > On Mon, 20 Feb 2023 at 12:59, Xi Ruoyao <xry111@xry111.site> wrote:
>     >>
>     >> On Mon, 2023-02-20 at 11:30 +0000, Jonny Grant wrote:
>     >>
>     >>> Thank you for the suggestion, I gave that nonnull attribute a try, but
>     >>> it doesn't appear to warn for this example.
>     >>>
>     >>> https://godbolt.org/z/boqTj6oWE <https://godbolt.org/z/boqTj6oWE>
>     >>
>     >> Ouch... The optimizer inlined make_std_string so both -Wnonnull and -
>     >> fanalyzer fails to catch the issue here.
>     >>
>     >> Adding noipa attribute for make_std_string will work, but will also
>     >> cause the generated code stupidly slow.  Maybe:
>     >>
>     >> #ifdef WANT_DIAGNOSTIC
>     >> #define MAKE_STD_STRING_ATTR __attribute__ ((noipa, nonnull))
>     >> #else
>     >> #define MAKE_STD_STRING_ATTR
>     >> #endif
>     >>
>     >> std::string make_std_string(const char * const str) MAKE_STD_STRING_ATTR;
>     >>
>     >> It still looks very stupid though.
>     >>
>     >>> Feels useful to get build warnings if compiler knows nullptr is going
>     >>> to be dereferenced, as clang does.
>     >>
>     >> The problem is in this case nullptr is not dereferenced, at all.  So if
>     >> we want a warning here we'll have to invent some new __builtin or
>     >> __attribute__ to give the compiler a hint.  AFAIK there is no such
>     >> facility now.
>     >
>     > I think __attribute__((access(read_only, 1))) should do it.
> 
>     I tried here, but no luck, is this example the way you mean? gcc trunk.
> 
> 
> I don't mean it does do it, I mean it should do it.

Ok I see. I filed an issue with a simpler test case.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108893

Kind regards
Jonny

Re: std::string add nullptr attribute

[Jonny Grant]

On 20/02/2023 12:59, Xi Ruoyao wrote:
> On Mon, 2023-02-20 at 11:30 +0000, Jonny Grant wrote:
> 
>> Thank you for the suggestion, I gave that nonnull attribute a try, but
>> it doesn't appear to warn for this example.
>>
>> https://godbolt.org/z/boqTj6oWE
> 
> Ouch... The optimizer inlined make_std_string so both -Wnonnull and -
> fanalyzer fails to catch the issue here.
> 
> Adding noipa attribute for make_std_string will work, but will also
> cause the generated code stupidly slow.  Maybe:
> 
> #ifdef WANT_DIAGNOSTIC
> #define MAKE_STD_STRING_ATTR __attribute__ ((noipa, nonnull))
> #else
> #define MAKE_STD_STRING_ATTR
> #endif
> 
> std::string make_std_string(const char * const str) MAKE_STD_STRING_ATTR;
> 
> It still looks very stupid though.
> 
>> Feels useful to get build warnings if compiler knows nullptr is going
>> to be dereferenced, as clang does.
> 
> The problem is in this case nullptr is not dereferenced, at all.  So if
> we want a warning here we'll have to invent some new __builtin or
> __attribute__ to give the compiler a hint.  AFAIK there is no such
> facility now.
> 
> And you cannot simply justifying to add a new facility because "I feel
> it useful".  Generally you need to show the benefit will be at least
> equally great than the maintenance burden introduced into the GCC code
> base.  And unfortunately usually we can only measure the burden after
> really writing all the code...  So it's not easy to convince someone to
> develop such a new feature.
> 
>> Personally I feel runtime should equally handle possible nullptr by
>> constructing strings in a try catch block so any exceptions are
>> handled or logged at least...
> 
> A portable runtime should not assume std::string(NULL) will raise an
> exception because other C++ standard libraries may behave differently. 
> The portable solution is to make a wrapper around std::string
> constructor and check if the parameter is NULL.
> 
>> Personally I would be pleased if GCC had a warning I could enable to
>> report any logic_error exceptions it knew would execute.
> 
> Or maybe when a program will definitely raise an uncatched exception. 
> But is the feature really useful?  This will not happen for anything
> other than simple toy programs.

Well seeing something like this would be useful at build time for me:

app.cpp: In function 'int connect_usb()':
app.cpp:16:7: warning: unhandled throw std::logic_error("my string: construction from null is not valid")


It's true it's very difficult to handle all uncatched exceptions. At least the software might restart that module (maybe using a watchdog if it finds that module unresponsive for a number of seconds, no heartbeat etc) after informing the user of an issue (my example would be when an unsupported USB device is connected to something in an embedded system, a car infotainment system - driver fails, but at least it could be restarted by the software), if we had the ID of the USB device, we can keep it in a forbid list, so it doesn't try to use it again. Otherwise the software would keep crashing in a multiple times while such a device was connected if that device wasn't forbidden. Anyway, this is all automotive specific.

There's a standard committee paper "Unconditional termination is a serious problem" P2698R0. Not exactly the same, but it's a similar topic.

Kind regards
Jonny

Re: std::string add nullptr attribute

[Jonny Grant]

On 09/02/2023 17:52, Jonathan Wakely wrote:
> On Thu, 9 Feb 2023 at 16:30, Xi Ruoyao wrote:
>>
>> On Thu, 2023-02-09 at 14:56 +0000, Jonathan Wakely via Gcc-help wrote:
>>>> Note, my code isn't like this, it is just an example to suggest
>>>> adding the nullptr attribute, as its clearly already rejected at
>>>> runtime.
>>>
>>> I assume you mean the nonnull attribute. That was added in 2020 and
>>> then reverted because it broke some things:
>>
>> I remember I'd once made the same mistake when I suggested to add
>> nonnull for ostream::operator<<(const string &) and I was lectured:
>> nonnull is not only a diagnostic attribute, it also allows the compiler
>> to assume the parameter is never null and rendering std::string(nullptr)
>> an undefined behavior.
> 
> Yes, I think that's what might have happened with the std::string change.

How about adding a method that is called by these operators that has the __attribute__ ((nonnull)) ?

example:
https://godbolt.org/z/bqW86PP34

>> Then the example may just silently continue to run, instead of throwing
>> an exception.  It would be an ironic example: an attempt to improve
>> diagnostic finally made diagnostic more difficult.
> 
> Indeed.
> 
> Maybe we can add __attribute__((access(read, 1))) instead, which says
> that we will read from the pointer, which also implies it must be
> non-null.
> 
> N.B. in C++23 string(nullptr) produces an error, although
> string((const char*)nullptr) doesn't, so in practice it only prevents
> the dumbest calls with a literal 'nullptr' token, and not the more
> realistic problems where you have a pointer that happens to be null.

There is a way to generate a build error for even string((const char*)nullptr)

I made another example that detects nullptr being passed around (should such stupid code occur) at build time providing optimizer is on. With -O0 it just gives the error always; so I put in an __OPTIMIZE__ check. This example doesn't need -fanalyzer.

https://godbolt.org/z/TdGnno4K5

#if __OPTIMIZE__
void nullptr_compile_abort() __attribute__((error("nullptr compile error")));
#endif

static void f2(const char * str)
{
#if __OPTIMIZE__
    if (str == nullptr) nullptr_compile_abort();
#endif
}

int main()
{
    f2((const char *)nullptr);
}


Regards, Jonny

Re: std::string add nullptr attribute

[Jonathan Wakely]

On Sun, 12 Mar 2023 at 22:10, Jonny Grant wrote:
>
>
>
> On 09/02/2023 17:52, Jonathan Wakely wrote:
> > On Thu, 9 Feb 2023 at 16:30, Xi Ruoyao wrote:
> >>
> >> On Thu, 2023-02-09 at 14:56 +0000, Jonathan Wakely via Gcc-help wrote:
> >>>> Note, my code isn't like this, it is just an example to suggest
> >>>> adding the nullptr attribute, as its clearly already rejected at
> >>>> runtime.
> >>>
> >>> I assume you mean the nonnull attribute. That was added in 2020 and
> >>> then reverted because it broke some things:
> >>
> >> I remember I'd once made the same mistake when I suggested to add
> >> nonnull for ostream::operator<<(const string &) and I was lectured:
> >> nonnull is not only a diagnostic attribute, it also allows the compiler
> >> to assume the parameter is never null and rendering std::string(nullptr)
> >> an undefined behavior.
> >
> > Yes, I think that's what might have happened with the std::string change.
>
> How about adding a method that is called by these operators that has the __attribute__ ((nonnull)) ?
>
> example:
> https://godbolt.org/z/bqW86PP34
>
> >> Then the example may just silently continue to run, instead of throwing
> >> an exception.  It would be an ironic example: an attempt to improve
> >> diagnostic finally made diagnostic more difficult.
> >
> > Indeed.
> >
> > Maybe we can add __attribute__((access(read, 1))) instead, which says
> > that we will read from the pointer, which also implies it must be
> > non-null.
> >
> > N.B. in C++23 string(nullptr) produces an error, although
> > string((const char*)nullptr) doesn't, so in practice it only prevents
> > the dumbest calls with a literal 'nullptr' token, and not the more
> > realistic problems where you have a pointer that happens to be null.
>
> There is a way to generate a build error for even string((const char*)nullptr)
>
> I made another example that detects nullptr being passed around (should such stupid code occur) at build time providing optimizer is on. With -O0 it just gives the error always; so I put in an __OPTIMIZE__ check. This example doesn't need -fanalyzer.
>
> https://godbolt.org/z/TdGnno4K5
>
> #if __OPTIMIZE__
> void nullptr_compile_abort() __attribute__((error("nullptr compile error")));
> #endif
>
> static void f2(const char * str)
> {
> #if __OPTIMIZE__
>     if (str == nullptr) nullptr_compile_abort();
> #endif
> }
>
> int main()
> {
>     f2((const char *)nullptr);
> }

This causes compilation to fail for code which is never executed at
run-time, which is not permitted by the standard.

You can use __attribute__((warning(""))) instead, but that is broken^W
inconvenient for inline functions. You need a non-inline definition of
the function, which means exporting a new function from the shared
library just for this diagnostic.

All these techniques you're rediscovering have been tried before :-)

Re: std::string add nullptr attribute

[Jonny Grant]

On 13/03/2023 10:10, Jonathan Wakely wrote:
> On Sun, 12 Mar 2023 at 22:10, Jonny Grant wrote:
>>
>>
>>
>> On 09/02/2023 17:52, Jonathan Wakely wrote:
>>> On Thu, 9 Feb 2023 at 16:30, Xi Ruoyao wrote:
>>>>
>>>> On Thu, 2023-02-09 at 14:56 +0000, Jonathan Wakely via Gcc-help wrote:
>>>>>> Note, my code isn't like this, it is just an example to suggest
>>>>>> adding the nullptr attribute, as its clearly already rejected at
>>>>>> runtime.
>>>>>
>>>>> I assume you mean the nonnull attribute. That was added in 2020 and
>>>>> then reverted because it broke some things:
>>>>
>>>> I remember I'd once made the same mistake when I suggested to add
>>>> nonnull for ostream::operator<<(const string &) and I was lectured:
>>>> nonnull is not only a diagnostic attribute, it also allows the compiler
>>>> to assume the parameter is never null and rendering std::string(nullptr)
>>>> an undefined behavior.
>>>
>>> Yes, I think that's what might have happened with the std::string change.
>>
>> How about adding a method that is called by these operators that has the __attribute__ ((nonnull)) ?
>>
>> example:
>> https://godbolt.org/z/bqW86PP34
>>
>>>> Then the example may just silently continue to run, instead of throwing
>>>> an exception.  It would be an ironic example: an attempt to improve
>>>> diagnostic finally made diagnostic more difficult.
>>>
>>> Indeed.
>>>
>>> Maybe we can add __attribute__((access(read, 1))) instead, which says
>>> that we will read from the pointer, which also implies it must be
>>> non-null.
>>>
>>> N.B. in C++23 string(nullptr) produces an error, although
>>> string((const char*)nullptr) doesn't, so in practice it only prevents
>>> the dumbest calls with a literal 'nullptr' token, and not the more
>>> realistic problems where you have a pointer that happens to be null.
>>
>> There is a way to generate a build error for even string((const char*)nullptr)
>>
>> I made another example that detects nullptr being passed around (should such stupid code occur) at build time providing optimizer is on. With -O0 it just gives the error always; so I put in an __OPTIMIZE__ check. This example doesn't need -fanalyzer.
>>
>> https://godbolt.org/z/TdGnno4K5
>>
>> #if __OPTIMIZE__
>> void nullptr_compile_abort() __attribute__((error("nullptr compile error")));
>> #endif
>>
>> static void f2(const char * str)
>> {
>> #if __OPTIMIZE__
>>     if (str == nullptr) nullptr_compile_abort();
>> #endif
>> }
>>
>> int main()
>> {
>>     f2((const char *)nullptr);
>> }
> 
> This causes compilation to fail for code which is never executed at
> run-time, which is not permitted by the standard.
> 
> You can use __attribute__((warning(""))) instead, but that is broken^W
> inconvenient for inline functions. You need a non-inline definition of
> the function, which means exporting a new function from the shared
> library just for this diagnostic.
> 
> All these techniques you're rediscovering have been tried before :-)

Is there any link you can refer me to for these techniques?

Regards, Jonny