[Bug libstdc++/108886] Add basic_string throw logic_error when assigned a nullptr

["jg at jguk dot org" <gcc-bugzilla@gcc.gnu.org>]

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

--- Comment #4 from Jonny Grant <jg at jguk dot org> ---
(In reply to Jonathan Wakely from comment #3)
> (In reply to Jonny Grant from comment #2)
> > I was taught to validate parameters at University. Personally I always
> > follow defensive programming approaches to avoid crashes.
>
> So stop passing null to these functions then :-)

Feels like we have a different opinion.  I know you know more about C++ than
me.

Defensive programming is carefulness in libraries too; particularly interfaces.
The C++ standard group have a paper "P2698R0 Unconditional termination is a
serious problem" you may have seen.

What I see is that the nullptr keyword is part of the language, and is used in
many places to indicate an object is not available. For me, a function that
requires a valid pointer, should check for the nullptr constant. The crash is
the code that de-references the nullptr because it requires a valid pointer. Of
course I would like all code to check for nullptr before passing to
std::string, and I would do those checks myself as well :-)

A nullptr is different from passing a use-after-free pointer.

Wrapping std::string is the workaround, as it doesn't always throw logic_error
for the nullptr constant.

It would just use up time when the STL implementation could handle it, as need
to check every method on std::string in the codebase to check for nullptr,
because it doesn't throw logic_error say for assign or other operators.

> > So I would check
> > parameters on all interface methods and operators. I would rely on the
> > compiler to remove any unnecessary duplicate sanity checks.
>
> That doesn't necessarily work when the member functions are separately
> compiled, as with most std::string member functions.

Ok, that's a shame. If it showed up in some performance measurements it could
be adjusted.

> > > What would be the point of _GLIBCXX_DEBUG_PEDASSERT when there's already a
> > > debug assertion there? Compiling with _GLIBCXX_DEBUG will already abort.
> >
> > I don't see a debug assertion for _GLIBCXX_DEBUG_PEDASSERT  could you point
> > out the file and line number to me please.
>
> You already quoted it in your comment 0 above, it's right there in
> assign(const _CharT*)!
>
>       basic_string&
>       assign(const _CharT* __s)
>       {
> 	__glibcxx_requires_string(__s);

Ok I see. Thank you for that example __glibcxx_requires_string is a macro that
compiles down to _GLIBCXX_DEBUG_PEDASSERT. I wonder why it wasn't written in
capitals as __GLIBCXX_REQUIRES_STRING_ASSERT?

Anyway, it is good there are asserts that report nullptr issues at runtime.

> > Just compiled with -D_GLIBCXX_DEBUG but I don't get any abort, just the same
> > SEGV
> > https://godbolt.org/z/rjYG8Yrnh
>
> If you want a PEDASSERT to fire you need to actually request pedantic
> assertions.
>
> https://godbolt.org/z/874x18G1G
>
> /opt/compiler-explorer/gcc-trunk-20230227/include/c++/13.0.1/bits/
> basic_string.h:1645: constexpr std::__cxx11::basic_string<_CharT, _Traits,
> _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::assign(const
> _CharT*) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc =
> std::allocator<char>]: Assertion '__s != nullptr' failed.
>
> I'm not persuaded to change anything here. The performance of string
> assignments is very important and adding an extra branch and throwing an
> exception isn't free.

It sounds like you know the implementation really well.

Personally on embedded systems I don't find the cost of checking for NULL an
issue. The 'throw' should not impact performance, as it is within the if() and
code shouldn't often throw, still better than SEGV! Maybe you have some
performance comparison statistics showing that a NULL check really impacts
performance? STL string heap allocations and actual memory read/write are the
biggest performance impact on performance in my experience on embedded.

I doubt all uses of std::string are performance intensive. For performance
critical code identified by a profiler I just re-write that function in C.
Usually graphics code.

There is what might be a performance issue, I don't know if anyone has
measured, _M_construct() appears to copy copies byte by byte in
basic_string.tcc:177 rather than using an optimized memcpy(). I've not measured
this myself.