[libc++] Remove trailing newline from _LIBCPP_ASSERTION_HANDLER calls

[sbc100]

This newline was originally added in https://reviews.llvm.org/D142184 but I think updating __libcpp_verbose_abort to add newline instead is more consistent, and works for other callers of _LIBCPP_VERBOSE_ABORT.

The _LIBCPP_ASSERTION_HANDLER calls through to either _LIBCPP_VERBOSE_ABORT macro or the __builtin_verbose_trap. From what I can tell neither of these function expect a trailing newline (at least none of the usage of _LIBCPP_VERBOSE_ABORT or __builtin_verbose_trap that I can find include a trailing newline except _LIBCPP_ASSERTION_HANDLER).

I noticed this discrepancy when working on
emscripten-core/emscripten#24543

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Sam Clegg (sbc100)

Changes

This newline was originally added in https://reviews.llvm.org/D142184 but I think updating __libcpp_verbose_abort to add newline instead is more consistent, and works for other callers of _LIBCPP_VERBOSE_ABORT.

The _LIBCPP_ASSERTION_HANDLER calls through to either _LIBCPP_VERBOSE_ABORT macro or the __builtin_verbose_trap From what I can tell neither of these function expect a trailing newline (At least non of the usage of _LIBCPP_VERBOSE_ABORT or __builtin_verbose_trap that I can find include a trailing newline except _LIBCPP_ASSERTION_HANDLER).

I noticed this discrepancy when working on
emscripten-core/emscripten#24543

---

Full diff: https://github.com/llvm/llvm-project/pull/143573.diff

3 Files Affected:

• (modified) libcxx/include/__assert (+1-1)
• (modified) libcxx/include/__cxx03/__assert (+1-1)
• (modified) libcxx/src/verbose_abort.cpp (+3)

diff --git a/libcxx/include/__assert b/libcxx/include/__assert
index 90eaa6023587b..062dc636b58b7 100644
--- a/libcxx/include/__assert
+++ b/libcxx/include/__assert
@@ -21,7 +21,7 @@
   (__builtin_expect(static_cast<bool>(expression), 1)                                                                  \
        ? (void)0                                                                                                       \
        : _LIBCPP_ASSERTION_HANDLER(__FILE__ ":" _LIBCPP_TOSTRING(__LINE__) ": assertion " _LIBCPP_TOSTRING(            \
-             expression) " failed: " message "\n"))
+             expression) " failed: " message))
 
 // WARNING: __builtin_assume can currently inhibit optimizations. Only add assumptions with a clear
 // optimization intent. See https://discourse.llvm.org/t/llvm-assume-blocks-optimization/71609 for a
diff --git a/libcxx/include/__cxx03/__assert b/libcxx/include/__cxx03/__assert
index f6c4d8e4a5d09..76579fece2829 100644
--- a/libcxx/include/__cxx03/__assert
+++ b/libcxx/include/__cxx03/__assert
@@ -21,7 +21,7 @@
   (__builtin_expect(static_cast<bool>(expression), 1)                                                                  \
        ? (void)0                                                                                                       \
        : _LIBCPP_ASSERTION_HANDLER(__FILE__ ":" _LIBCPP_TOSTRING(__LINE__) ": assertion " _LIBCPP_TOSTRING(            \
-             expression) " failed: " message "\n"))
+             expression) " failed: " message))
 
 // TODO: __builtin_assume can currently inhibit optimizations. Until this has been fixed and we can add
 //       assumptions without a clear optimization intent, disable that to avoid worsening the code generation.
diff --git a/libcxx/src/verbose_abort.cpp b/libcxx/src/verbose_abort.cpp
index 94bdb451dee7a..4167ff311901d 100644
--- a/libcxx/src/verbose_abort.cpp
+++ b/libcxx/src/verbose_abort.cpp
@@ -30,6 +30,9 @@ _LIBCPP_WEAK void __libcpp_verbose_abort(char const* format, ...) noexcept {
     va_list list;
     va_start(list, format);
     std::vfprintf(stderr, format, list);
+    // Callers of `__libcpp_verbose_abort` to not include a newline but when
+    // writing the message to stderr we need to include one.
+    std::vfprintf(stderr, "\n", list);
     va_end(list);
   }
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[sbc100]

@vitalybuka WDYT?

[ldionne]

I'm fine with this assuming CI is happy -- but let's give a bit of time for @vitalybuka to chime in.

[sbc100]

I'm seeing the following error from the CI:

  CMake Error at CMakeLists.txt:315 (message):
    Failed to determine the source files for the regular expression backend

presumably this is not related to this PR?

[sbc100]

@ldionne, are the CI bots here reliable? i.e. do we always wait for an all-green run?

[philnik777]

No, but yes. They currently aren't reliable, but if they run properly they give reliable results. There are currently just infrastructure issues. An all-green run is definitely preferred and a few restarts should be planned in currently.

[sbc100]

No, but yes. They currently aren't reliable, but if they run properly they give reliable results. There are currently just infrastructure issues. An all-green run is definitely preferred and a few restarts should be planned in currently.

Can you confirm that the current failures look unrelated to this change? I've tried restarting and rebasing a few times and I'm not having any luck getting all-green here.

[philnik777]

No, but yes. They currently aren't reliable, but if they run properly they give reliable results. There are currently just infrastructure issues. An all-green run is definitely preferred and a few restarts should be planned in currently.

Can you confirm that the current failures look unrelated to this change? I've tried restarting and rebasing a few times and I'm not having any luck getting all-green here.

It does look unrelated. I've restarted the failed job. If it fails again in the same way feel free to land.

[mysterymath]

This is causing build failures in the Fuchsia toolchain builders across all platforms. Accordingly, I'm issuing a revert.

Example:
https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8711781438372007649/overview

[2466/2505](36) Building CXX object libcxx/src/CMakeFiles/cxx_static.dir/verbose_abort.cpp.obj
FAILED: libcxx/src/CMakeFiles/cxx_static.dir/verbose_abort.cpp.obj 
/b/s/w/ir/x/w/llvm_build/./bin/clang++ --target=armv6m-none-eabi -DLIBC_NAMESPACE=__llvm_libc_common_utils -D_DEBUG -D_FILE_OFFSET_BITS=64 -D_GLIBCXX_ASSERTIONS -D_LARGEFILE_SOURCE -D_LIBCPP_BUILDING_HAS_NO_ABI_LIBRARY -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_LINK_PTHREAD_LIB -D_LIBCPP_LINK_RT_LIB -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src -I/b/s/w/ir/x/w/llvm_build/include/armv6m-unknown-none-eabi/c++/v1 -I/b/s/w/ir/x/w/llvm_build/include/c++/v1 -I/b/s/w/ir/x/w/llvm-llvm-project/cmake/Modules/../../libc -isystem /b/s/w/ir/x/w/llvm_build/include/armv6m-unknown-none-eabi --target=armv6m-none-eabi -Wno-atomic-alignment "-Dvfprintf(stream, format, vlist)=vprintf(format, vlist)" "-Dfprintf(stream, format, ...)=printf(format)" -D_LIBCPP_PRINT=1 -mthumb -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -ffunction-sections -fdata-sections -ffile-prefix-map=/b/s/w/ir/x/w/llvm_build/runtimes/runtimes-armv6m-none-eabi-bins=../../../llvm-llvm-project -ffile-prefix-map=/b/s/w/ir/x/w/llvm-llvm-project/= -no-canonical-prefixes -Os -DNDEBUG -std=c++2b -UNDEBUG -faligned-allocation -nostdinc++ -fvisibility-inlines-hidden -fvisibility=hidden -fsized-deallocation -Wall -Wextra -Wnewline-eof -Wshadow -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wundef -Wunused-template -Wformat-nonliteral -Wzero-length-array -Wdeprecated-redundant-constexpr-static-def -Wno-nullability-completeness -Wno-user-defined-literals -Wno-covered-switch-default -Wno-suggest-override -Wno-error -fno-exceptions -fno-rtti -fdebug-prefix-map=/b/s/w/ir/x/w/llvm_build/include/c++/v1=/b/s/w/ir/x/w/llvm-llvm-project/libcxx/include -nostdlibinc -MD -MT libcxx/src/CMakeFiles/cxx_static.dir/verbose_abort.cpp.obj -MF libcxx/src/CMakeFiles/cxx_static.dir/verbose_abort.cpp.obj.d -o libcxx/src/CMakeFiles/cxx_static.dir/verbose_abort.cpp.obj -c /b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/verbose_abort.cpp
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/verbose_abort.cpp:35:10: error: reference to unresolved using declaration
   35 |     std::fputc('\n', stderr);
      |          ^
/b/s/w/ir/x/w/llvm_build/include/c++/v1/cstdio:139:1: note: using declaration annotated with 'using_if_exists' here
  139 | using ::fputc _LIBCPP_USING_IF_EXISTS;
      | ^
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/verbose_abort.cpp:35:5: error: excess elements in scalar initializer
   35 |     std::fputc('\n', stderr);
      |     ^              ~~~~~