From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 48) id DE1283858C52; Sun, 12 Mar 2023 23:03:34 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org DE1283858C52 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1678662214; bh=7CwaIqV9MUPUyV7arRjVZVMIcZL3cUNPtKb+yKpuC7Q=; h=From:To:Subject:Date:In-Reply-To:References:From; b=SJImj8xk0Jl6sbA4d5mbhcxORn9yOrsENlCwZuOtaR+c7QKHdQDA/sJw8E9KHilV7 pI1aR2rLlwPEVlYGp+JW9+TFyuk+V0zMBHhfYdw+BT4o0Q7itf9P2che1/H3KPrg6Y Rt8ZIu7FsmlLFtZwiPqeuh3AAE0UxO3TQccq3ONM= From: "jg at jguk dot org" To: gcc-bugs@gcc.gnu.org Subject: [Bug libstdc++/108886] Add basic_string throw logic_error when assigned a nullptr Date: Sun, 12 Mar 2023 23:03:33 +0000 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: gcc X-Bugzilla-Component: libstdc++ X-Bugzilla-Version: 12.2.0 X-Bugzilla-Keywords: X-Bugzilla-Severity: normal X-Bugzilla-Who: jg at jguk dot org X-Bugzilla-Status: UNCONFIRMED X-Bugzilla-Resolution: X-Bugzilla-Priority: P3 X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org X-Bugzilla-Target-Milestone: --- X-Bugzilla-Flags: X-Bugzilla-Changed-Fields: Message-ID: In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/ Auto-Submitted: auto-generated MIME-Version: 1.0 List-Id: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D108886 --- Comment #4 from Jonny Grant --- (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 interfa= ces. 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_er= ror for the nullptr constant. It would just use up time when the STL implementation could handle it, as n= eed 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 cou= ld be adjusted. > > > What would be the point of _GLIBCXX_DEBUG_PEDASSERT when there's alre= ady a > > > debug assertion there? Compiling with _GLIBCXX_DEBUG will already abo= rt. > > > > I don't see a debug assertion for _GLIBCXX_DEBUG_PEDASSERT could you p= oint > > 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 t= hat 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 =3D char; _Traits =3D std::char_traits; _Allo= c =3D > std::allocator]: Assertion '__s !=3D 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 t= he 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 meas= ured this myself.=