[libcxxabi] define _LIBCPP_VERBOSE_ABORT

[nickdesaulniers]

Summary

libc++ may be built with or without assertions. This causes definitions
of various methods of std::string_view to contain assertions and calls
to libcpp_verbose_abort which libcxxabi does not provide. libcxxabi
does provide abort_message with the same interface, so define
_LIBCPP_VERBOSE_ABORT to use that. Otherwise D148566 will trigger
linkage failures for the missing symbol libcpp_verbose_abort.

Link: https://libcxx.llvm.org/UsingLibcxx.html#enabling-the-safe-libc-mode

[nickdesaulniers]

It looks like libcxxabi has abort_message (libcxxabi/src/abort_message.{c|h}) that shares the same fn signature as __libcpp_verbose_abort; __libcpp_verbose_abort could be defined in terms of abort_message, but then I wonder if that would create an issue for libcxx since __libcpp_verbose_abort would be provided by libcxxabi then?

[nickdesaulniers]

Otherwise something like:

diff --git a/libcxxabi/src/abort_message.cpp b/libcxxabi/src/abort_message.cpp
index 859a5031b93f..a45ea261de82 100644
--- a/libcxxabi/src/abort_message.cpp
+++ b/libcxxabi/src/abort_message.cpp
@@ -77,3 +77,7 @@ void abort_message(const char* format, ...)
 
     abort();
 }
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+void __libcpp_verbose_abort (char const *fmt, ...) __attribute__((alias("abort_message")));
+_LIBCPP_END_NAMESPACE_STD

seems to build (maybe needs guards on _LIBCPP_BEGIN_NAMESPACE_STD+_LIBCPP_END_NAMESPACE_STD.

[nickdesaulniers]

ping for review

[philnik]

Are you sure there is no way around this? It seems like a bad idea to disable assertions when the user specifically requested them. These symbols are also provided by the time the program is run, so it should (at least theoretically) be possible to resolve the symbols then, since AFAIK the symbols are loaded lazily.

[nickdesaulniers]

Thanks for taking a look!

In D149092#4310156, @philnik wrote:

Are you sure there is no way around this? It seems like a bad idea to disable assertions when the user specifically requested them. These symbols are also provided by the time the program is run, so it should (at least theoretically) be possible to resolve the symbols then, since AFAIK the symbols are loaded lazily.

Is libcxxabi _always_ linked against libcxx? Is there a linker flag to say "an undefined reference to std::__1::__libcpp_verbose_abort(char const*, ...) is not an error?"

https://reviews.llvm.org/D149092#4293607 proposes using an alias attribute to alias the symbol __libcpp_verbose_abort to abort_message (they would have the same implementation anyways). I don't know if that's portable; IIRC alias requires a mangled name and IIRC platforms using mach-o add an underscore prefix? Also, I don't know if alias is an ELF only thing.

I could also just implement std::__1::__libcpp_verbose_abort(char const*, ...) in terms of libcxxabi's abort_message. It kind of sucks to have one variadic function call another variadic function since the args are simply forwarded.

__libcpp_verbose_abort is defined as a weak symbol in libcxx/src/verbose_abort.cpp. I don't think I'd want to provide a strong symbol in libcxxabi. Perhaps I should provide a definition of __libcpp_verbose_abort in libcxxabi/src/abort_message.cpp?

diff --git a/libcxxabi/src/abort_message.cpp b/libcxxabi/src/abort_message.cpp
index 859a5031b93f..79774fe1b3af 100644
--- a/libcxxabi/src/abort_message.cpp
+++ b/libcxxabi/src/abort_message.cpp
@@ -77,3 +77,14 @@ void abort_message(const char* format, ...)
 
     abort();
 }
+
+#if defined(_LIBCPP_ENABLE_ASSERTIONS) && defined(_LIBCPP_BEGIN_NAMESPACE_STD)
+_LIBCPP_BEGIN_NAMESPACE_STD
+void __libcpp_verbose_abort (const char *fmt, ...) {
+  va_list list;
+  va_start(list, fmt);
+  abort_message("%s", fmt);
+  va_end(list);
+}
+_LIBCPP_END_NAMESPACE_STD
+#endif

IDK

[philnik]

In D149092#4310328, @nickdesaulniers wrote:

Thanks for taking a look!

In D149092#4310156, @philnik wrote:

Are you sure there is no way around this? It seems like a bad idea to disable assertions when the user specifically requested them. These symbols are also provided by the time the program is run, so it should (at least theoretically) be possible to resolve the symbols then, since AFAIK the symbols are loaded lazily.

Is libcxxabi _always_ linked against libcxx? Is there a linker flag to say "an undefined reference to std::__1::__libcpp_verbose_abort(char const*, ...) is not an error?"

https://reviews.llvm.org/D149092#4293607 proposes using an alias attribute to alias the symbol __libcpp_verbose_abort to abort_message (they would have the same implementation anyways). I don't know if that's portable; IIRC alias requires a mangled name and IIRC platforms using mach-o add an underscore prefix? Also, I don't know if alias is an ELF only thing.

I could also just implement std::__1::__libcpp_verbose_abort(char const*, ...) in terms of libcxxabi's abort_message. It kind of sucks to have one variadic function call another variadic function since the args are simply forwarded.

__libcpp_verbose_abort is defined as a weak symbol in libcxx/src/verbose_abort.cpp. I don't think I'd want to provide a strong symbol in libcxxabi. Perhaps I should provide a definition of __libcpp_verbose_abort in libcxxabi/src/abort_message.cpp?

diff --git a/libcxxabi/src/abort_message.cpp b/libcxxabi/src/abort_message.cpp
index 859a5031b93f..79774fe1b3af 100644
--- a/libcxxabi/src/abort_message.cpp
+++ b/libcxxabi/src/abort_message.cpp
@@ -77,3 +77,14 @@ void abort_message(const char* format, ...)
 
     abort();
 }
+
+#if defined(_LIBCPP_ENABLE_ASSERTIONS) && defined(_LIBCPP_BEGIN_NAMESPACE_STD)
+_LIBCPP_BEGIN_NAMESPACE_STD
+void __libcpp_verbose_abort (const char *fmt, ...) {
+  va_list list;
+  va_start(list, fmt);
+  abort_message("%s", fmt);
+  va_end(list);
+}
+_LIBCPP_END_NAMESPACE_STD
+#endif

IDK

I also can't tell you what is the right thing to do here. I think @ldionne may want to take a look. I think it would make sense to move it into libc++abi, but that wouldn't work when not linking against it, so I'm not sure this is the right call.

[nickdesaulniers]

In D149092#4310340, @philnik wrote:

I also can't tell you what is the right thing to do here. I think @ldionne may want to take a look. I think it would make sense to move it into libc++abi, but that wouldn't work when not linking against it, so I'm not sure this is the right call.

For context, I'm trying to remove libcxxabi's StringView class and replace it with std::string_view. If std::string_view comes from libcxx, then we have a circular dependency wrt libcxx asserts or methods that throw.

[nickdesaulniers]

Hot take; this patch is NFC for libc++. IF any code from libc++ being used in libcxxabi could assert, libcxxabi would have failed to build with a linkage failure against __libcpp_verbose_abort since libc++ asserts call __libcpp_verbose_abort upon assertion failure.

Also, this patch isn't something small/for fun, it's blocking keeping libcxxabi's demangler in sync with LLVM's, so it's not something that we can ignore.

[philnik]

In D149092#4316184, @nickdesaulniers wrote:

Hot take; this patch is NFC for libc++. IF any code from libc++ being used in libcxxabi could assert, libcxxabi would have failed to build with a linkage failure against __libcpp_verbose_abort since libc++ asserts call __libcpp_verbose_abort upon assertion failure.

Also, this patch isn't something small/for fun, it's blocking keeping libcxxabi's demangler in sync with LLVM's, so it's not something that we can ignore.

Even if it is NFC, it's still surprising. Switching to string_view is in this case a functional change, so we should consider what we want to do here.

[nickdesaulniers]

In D149092#4316236, @philnik wrote:

In D149092#4316184, @nickdesaulniers wrote:

Hot take; this patch is NFC for libc++. IF any code from libc++ being used in libcxxabi could assert, libcxxabi would have failed to build with a linkage failure against __libcpp_verbose_abort since libc++ asserts call __libcpp_verbose_abort upon assertion failure.

Also, this patch isn't something small/for fun, it's blocking keeping libcxxabi's demangler in sync with LLVM's, so it's not something that we can ignore.

Even if it is NFC, it's still surprising.

Surprising that I'm explicitly disabling asserts that we weren't using in the first place? (If we were using them, how were we able to link libc++abi.so without a definition of __libcpp_verbose_abort?)

Switching to string_view is in this case a functional change, so we should consider what we want to do here.

Then that should be debated in the patch that's doing that (D148566) which has already been approved.

[nickdesaulniers]

Looking at libcxx/include/__verbose_abort, we might also be able to provide a definition of _LIBCPP_VERBOSE_ABORT perhaps?

[nickdesaulniers]

In D149092#4316297, @nickdesaulniers wrote:

Looking at libcxx/include/__verbose_abort, we might also be able to provide a definition of _LIBCPP_VERBOSE_ABORT perhaps?

Yeah, this diff seems to work. Will use that instead:

diff --git a/libcxxabi/src/cxa_demangle.cpp b/libcxxabi/src/cxa_demangle.cpp
index 2c0f82bbb207..03085cb5903b 100644
--- a/libcxxabi/src/cxa_demangle.cpp
+++ b/libcxxabi/src/cxa_demangle.cpp
@@ -10,6 +10,7 @@
 // file does not yet support:
 //   - C++ modules TS
 
+#include "demangle/DemangleConfig.h"
 #include "demangle/ItaniumDemangle.h"
 #include "__cxxabi_config.h"
 #include <cassert>
diff --git a/libcxxabi/src/demangle/DemangleConfig.h b/libcxxabi/src/demangle/DemangleConfig.h
index 9d818535b094..bebef7713522 100644
--- a/libcxxabi/src/demangle/DemangleConfig.h
+++ b/libcxxabi/src/demangle/DemangleConfig.h
@@ -11,7 +11,11 @@
 #ifndef LIBCXXABI_DEMANGLE_DEMANGLE_CONFIG_H
 #define LIBCXXABI_DEMANGLE_DEMANGLE_CONFIG_H
 
+// Must be defined befor pulling in headers from libc++.
+#define _LIBCPP_VERBOSE_ABORT(...) abort_message(__VA_ARGS__)
+
 #include <ciso646>
+#include "../abort_message.h"
 
 #ifdef _MSC_VER
 // snprintf is implemented in VS 2015

[nickdesaulniers]

• change patch to define _LIBCPP_VERBOSE_ABORT

[nickdesaulniers]

• git clang-format HEAD~, add link to libcpp doc on _LIBCPP_VERBOSE_ABORT

[ldionne]

I think this patch shows that in reality, libc++abi should be part of libc++ (i.e. it should be possible to either use an external ABI library, or libc++'s own ABI library aka libc++abi). This is a recurring cause of issues.

Anyway, I think the patch in its current form makes sense, I remember hitting this issue in a different setting and it's indeed kind of unsettling. Thanks for trying to keep the two demangler copies in sync -- that's actually another thing that we should really try to have one canonical version of.

[ldionne]

Can you just rebase and re-upload the patch to run the CI? Please make sure it's green before merging, it looks like you hit infrastructure issues.

[nickdesaulniers]

• rebase

libcxxabi/src/cxa_demangle.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // FIXME: (possibly) incomplete list of features that clang mangles that this
 // file does not yet support:
 //   - C++ modules TS
 
+#include "demangle/DemangleConfig.h"
 #include "demangle/ItaniumDemangle.h"
 #include "__cxxabi_config.h"
 #include <cassert>
 #include <cctype>
 #include <cstdio>
 #include <cstdlib>
 #include <cstring>
 #include <functional>

libcxxabi/src/demangle/DemangleConfig.h

 //===--- DemangleConfig.h --------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 // This file is contains a subset of macros copied from
 // llvm/include/llvm/Demangle/DemangleConfig.h
 //===----------------------------------------------------------------------===//
 
 #ifndef LIBCXXABI_DEMANGLE_DEMANGLE_CONFIG_H
 #define LIBCXXABI_DEMANGLE_DEMANGLE_CONFIG_H
 
+// Must be defined before pulling in headers from libc++.
+// https://libcxx.llvm.org/UsingLibcxx.html#enabling-the-safe-libc-mode
+#define _LIBCPP_VERBOSE_ABORT(...) abort_message(__VA_ARGS__)
+
+#include "../abort_message.h"
 #include <ciso646>
 
 #ifdef _MSC_VER
 // snprintf is implemented in VS 2015
 #if _MSC_VER < 1900
 #define snprintf _snprintf_s
 #endif
 #endif