Re: [PATCH] print extended assertion failures to stderr

[Jonathan Wakely <jwakely@redhat.com>]

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

On Wed, 27 Oct 2021 at 09:27, Jay Feldblum via Libstdc++ <
libstdc++@gcc.gnu.org> wrote:

> From: yfeldblum <yfeldblum@gmail.com>
>
> The stdout stream is reserved for output intentionally produced by the
> application. Assertion failures and other forms of logging must be
> emitted to stderr, not to stdout.
>
> It is common for testing and monitoring infrastructure to scan stderr
> for errors, such as for assertion failures, and to collect or retain
> them for analysis or observation. It is a norm that assertion failures
> match this expectation in practice.
>
> While `__builtin_fprintf` is available as a builtin, there is no
> equivalent builtin for `stderr`. The only option in practice is to use
> the macro `stderr`, which requires `#include <cstdio>`. It is desired
> not to add such an include to `bits/c++config` so the solution is to
> write and export a function which may be called by `bits/c++config`.
>
> This is expected to be API-compatible and ABI-compatible with caveats.
> Code compiled against an earlier libstdc++ will work when linked into a
> later libstdc++ but the stream to which assertion failures are logged is
> anybody's guess, and in practice will be determined by the link line and
> the choice of linker. This fix targets builds for which all C++ code is
> built against a libstdc++ with the fix.
>

Thanks for the patch! Comments below.



>
> Alternatives:
> * This, which is the smallest change.
> * This, but also defining symbols `std::__stdin` and `std::__stdout` for
>   completeness.
> * Define a symbol like `std::__printf_stderr` which prints any message
>   with any formatting to stderr, just as `std::printf` does to stdout,
>   and call that from `std::__replacement_assert` instead of calling
>   `__builtin_printf`.
> * Move `std::__replacement_assert` into libstdc++.so and no longer mark
>   it as weak. This allows an application with some parts built against a
>   previous libstdc++ to guarantee that the fix will be applied at least
>   to the parts that are built against a libstdc++ containing the fix.
>
> libstdc++-v3/ChangeLog:
> include/bits/c++config (__glibcxx_assert): print to stderr.
> ---
>  libstdc++-v3/include/bits/c++config |  8 ++++--
>  libstdc++-v3/src/c++98/Makefile.am  |  1 +
>  libstdc++-v3/src/c++98/Makefile.in  |  2 +-
>  libstdc++-v3/src/c++98/stdio.cc     | 39 +++++++++++++++++++++++++++++
>  4 files changed, 47 insertions(+), 3 deletions(-)
>  create mode 100644 libstdc++-v3/src/c++98/stdio.cc
>
> diff --git a/libstdc++-v3/include/bits/c++config
> b/libstdc++-v3/include/bits/c++config
> index
> a64958096718126a49e8767694e913ed96108df2..d821ba09d88dc3e42ff1807200cfece71cc18bd9
> 100644
> --- a/libstdc++-v3/include/bits/c++config
> +++ b/libstdc++-v3/include/bits/c++config
> @@ -523,6 +523,10 @@ namespace std
>  # ifdef _GLIBCXX_VERBOSE_ASSERT
>  namespace std
>  {
> +  // Avoid the use of stderr, because we're trying to keep the <cstdio>
> +  // include out of the mix.
> +  extern "C++" void* __stderr() _GLIBCXX_NOEXCEPT;
>

We can declare this locally in __replacement_assert, so it isn't made
visible in namespace std.

+
>    // Avoid the use of assert, because we're trying to keep the <cassert>
>    // include out of the mix.
>    extern "C++" _GLIBCXX_NORETURN
> @@ -531,8 +535,8 @@ namespace std
>          const char* __function, const char* __condition)
>    _GLIBCXX_NOEXCEPT
>    {
> -    __builtin_printf("%s:%d: %s: Assertion '%s' failed.\n", __file,
> __line,
> -      __function, __condition);
> +    __builtin_fprintf(__stderr(), "%s:%d: %s: Assertion '%s' failed.\n",
> +                      __file, __line, __function, __condition);
>      __builtin_abort();
>    }
>  }
> diff --git a/libstdc++-v3/src/c++98/Makefile.am
> b/libstdc++-v3/src/c++98/Makefile.am
> index
> b48b57a2945780bb48496d3b5e76de4be61f836e..4032f914ea20344f51f2f219c5575d2a3858c44c
> 100644
> --- a/libstdc++-v3/src/c++98/Makefile.am
> +++ b/libstdc++-v3/src/c++98/Makefile.am
> @@ -136,6 +136,7 @@ sources = \
>   math_stubs_float.cc \
>   math_stubs_long_double.cc \
>   stdexcept.cc \
> + stdio.cc \
>

I think adding it to src/c++11/debug.cc makes sense. That file already uses
stderr itself, and is where we define other utilities for printing
assertions.

We also need to add it to the linker script, so that the symbol gets
exported from the shared library. Otherwise any use of
-D_GLIBCXX_ASSERTIONS or -D_GLIBCXX_DEBUG results in linker errors.

The attached patch does that.

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

commit 75e7612437f65abe21689845b1856bb308c6cb81
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Nov 3 16:06:29 2021

    libstdc++: Print assertion messages to stderr [PR59675]
    
    This replaces the printf used by failed debug assertions with fprintf,
    so we can write to stderr. To avoid including <stdio.h> we call a new
    function exported from the library, which returns the stderr pointer.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/59675
            * acinclude.m4 (libtool_VERSION): Bump version.
            * config/abi/pre/gnu.ver (GLIBCXX_3.4.30): Add version and
            export new symbol.
            * configure: Regenerate.
            * include/bits/c++config (__replacement_assert): Use fprintf and
            write to stderr.
            * src/c++11/debug.cc (std::__stderr): Define.
            * testsuite/util/testsuite_abi.cc: Update latest version.

diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4
index 90ecc4a87a2..30a4abb98b3 100644
--- a/libstdc++-v3/acinclude.m4
+++ b/libstdc++-v3/acinclude.m4
@@ -3798,7 +3798,7 @@ changequote([,])dnl
 fi
 
 # For libtool versioning info, format is CURRENT:REVISION:AGE
-libtool_VERSION=6:29:0
+libtool_VERSION=6:30:0
 
 # Everything parsed; figure out what files and settings to use.
 case $enable_symvers in
diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
index 5323c7f0604..3c59465ab9e 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -2397,6 +2397,12 @@ GLIBCXX_3.4.29 {
 
 } GLIBCXX_3.4.28;
 
+GLIBCXX_3.4.30 {
+
+    _ZSt8__stderrv;
+
+} GLIBCXX_3.4.29;
+
 # Symbols in the support library (libsupc++) have their own tag.
 CXXABI_1.3 {
 
diff --git a/libstdc++-v3/configure b/libstdc++-v3/configure
index c1aea827070..7dc7dcee029 100755
--- a/libstdc++-v3/configure
+++ b/libstdc++-v3/configure
@@ -74892,7 +74892,7 @@ $as_echo "$as_me: WARNING: === Symbol versioning will be disabled." >&2;}
 fi
 
 # For libtool versioning info, format is CURRENT:REVISION:AGE
-libtool_VERSION=6:29:0
+libtool_VERSION=6:30:0
 
 # Everything parsed; figure out what files and settings to use.
 case $enable_symvers in
diff --git a/libstdc++-v3/include/bits/c++config b/libstdc++-v3/include/bits/c++config
index a6495809671..75421731cae 100644
--- a/libstdc++-v3/include/bits/c++config
+++ b/libstdc++-v3/include/bits/c++config
@@ -531,8 +531,10 @@ namespace std
 		       const char* __function, const char* __condition)
   _GLIBCXX_NOEXCEPT
   {
-    __builtin_printf("%s:%d: %s: Assertion '%s' failed.\n", __file, __line,
-		     __function, __condition);
+    // Also avoid including <cstdio>.
+    extern void* __stderr() _GLIBCXX_NOEXCEPT;
+    __builtin_fprintf(__stderr(), "%s:%d: %s: Assertion '%s' failed.\n",
+		      __file, __line, __function, __condition);
     __builtin_abort();
   }
 }
diff --git a/libstdc++-v3/src/c++11/debug.cc b/libstdc++-v3/src/c++11/debug.cc
index 0128535135e..e79e49ea439 100644
--- a/libstdc++-v3/src/c++11/debug.cc
+++ b/libstdc++-v3/src/c++11/debug.cc
@@ -43,6 +43,12 @@
 
 #include "mutex_pool.h"
 
+namespace std
+{
+  // Used by std::__replacement_assert in <bits/c++config.h>.
+  void* __stderr() noexcept { return stderr; }
+}
+
 using namespace std;
 
 namespace
@@ -1202,4 +1208,5 @@ namespace __gnu_debug
                                     const char*) const;
 #endif
 
+
 } // namespace __gnu_debug
diff --git a/libstdc++-v3/testsuite/util/testsuite_abi.cc b/libstdc++-v3/testsuite/util/testsuite_abi.cc
index 3af5dc593c2..1ca7da4fcd0 100644
--- a/libstdc++-v3/testsuite/util/testsuite_abi.cc
+++ b/libstdc++-v3/testsuite/util/testsuite_abi.cc
@@ -210,6 +210,7 @@ check_version(symbol& test, bool added)
       known_versions.push_back("GLIBCXX_3.4.27");
       known_versions.push_back("GLIBCXX_3.4.28");
       known_versions.push_back("GLIBCXX_3.4.29");
+      known_versions.push_back("GLIBCXX_3.4.30");
       known_versions.push_back("GLIBCXX_LDBL_3.4.29");
       known_versions.push_back("GLIBCXX_IEEE128_3.4.29");
       known_versions.push_back("CXXABI_1.3");
@@ -245,7 +246,7 @@ check_version(symbol& test, bool added)
 	test.version_status = symbol::incompatible;
 
       // Check that added symbols are added in the latest pre-release version.
-      bool latestp = (test.version_name == "GLIBCXX_3.4.29"
+      bool latestp = (test.version_name == "GLIBCXX_3.4.30"
 	  // XXX remove next 3 lines when baselines have been regenerated
 	  // to include {IEEE128,LDBL} symbols:
 		     || test.version_name == "GLIBCXX_LDBL_3.4.29"