Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++abi] Disable undesired attempts at opening file descriptors #418

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

infinitesnow
Copy link

@infinitesnow infinitesnow commented May 12, 2024

When building for a web target, there is no stderr. However, the abort implementation in the current CXXABI runtime is implemented with writes to stderr.

Since the constructors for default objects call abort, these are rightfully not optimised out from the compiler. However, it is hardly sensible in my opinion to have to include a whole WASI runtime implementation in the browser just to handle more informative aborts by default.

To prevent this, compile the cxxabi runtime with NDEBUG and CXXABI_BAREMETAL flags.

This is not enough though, as the compiler will still try to instantiate __libcpp_verbose_abort from the stdcxx library. To prevent the compiler from instantiating __libcpp_verbose_abort, clean fix for this is to consistently set _LIBCPP_AVAILABILITY_HAS_VERBOSE_ABORT=0 in the __availability header. This requires forking or patching LLVM.

When building for a web target, there is no stderr. However, the abort
implementation in the current CXXABI runtime is implemented with writes
to stderr.

Since the constructors for default objects call abort,
these are rightfully not optimised out from the compiler. However, it is
hardly sensible in my opinion to have to include a whole WASI runtime
implementation in the browser just to handle more informative aborts by default.

To prevent this, compile the cxxabi runtime with NDEBUG and CXXABI_BAREMETAL
flags.

This is not enough though, as the compiler will still try to instantiate
__libcpp_verbose_abort from the stdcxx library. To prevent the compiler
from instantiating __libcpp_verbose_abort, clean fix for this is to consistently
set _LIBCPP_AVAILABILITY_HAS_VERBOSE_ABORT=0 in the __availability header.
This requires forking or patching LLVM.
@@ -185,6 +185,7 @@ build/compiler-rt.BUILT: build/llvm.BUILT
# $(3): the name of the target being built for
# $(4): extra compiler flags to pass
LIBCXX_CMAKE_FLAGS = \
-DNDEBUG=1 \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is -DCMAKE_BUILD_TYPE=RelWithDebInfo below not enough to get this defined?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, from further inspection it looks like it. This flag isn't piped through to make. See the thread for why I included it in the first place.

@sbc100
Copy link
Member

sbc100 commented May 13, 2024

Can you perhaps mention in the title and/or description that this change relates specifically the libc++abi? (Perhaps prefix the title with [libc++abi])

@abrown
Copy link
Collaborator

abrown commented May 13, 2024

And just to clarify: this PR does not fully solve #401 because of __libcpp_verbose_abort? Maybe we need an upstream way to configure _LIBCPP_AVAILABILITY_HAS_VERBOSE_ABORT=0 at build time or something?

@infinitesnow infinitesnow changed the title Disable undesired attempts at opening file descriptors [libc++abi] Disable undesired attempts at opening file descriptors May 13, 2024
@infinitesnow
Copy link
Author

infinitesnow commented May 13, 2024

This is the patch that needs to be applied to llvm to be able to run stdlib code bare metal in the browser. This alone is sufficient to fix #401 - at least for C++.

diff --git a/libcxx/include/__availability b/libcxx/include/__availability
index b8b2da9bb122..a1fbf8cfe3f7 100644
--- a/libcxx/include/__availability
+++ b/libcxx/include/__availability
@@ -128,7 +128,7 @@
 // This controls whether the library claims to provide a default verbose
 // termination function, and consequently whether the headers will try
 // to use it when the mechanism isn't overriden at compile-time.
-#  define _LIBCPP_AVAILABILITY_HAS_VERBOSE_ABORT 1
+#  define _LIBCPP_AVAILABILITY_HAS_VERBOSE_ABORT 0
 #  define _LIBCPP_AVAILABILITY_VERBOSE_ABORT

 // This controls the availability of the C++17 std::pmr library,
diff --git a/libcxxabi/src/abort_message.cpp b/libcxxabi/src/abort_message.cpp
index 859a5031b93f..49a9cbb3e71e 100644
--- a/libcxxabi/src/abort_message.cpp
+++ b/libcxxabi/src/abort_message.cpp
@@ -31,7 +31,7 @@ void abort_message(const char* format, ...)
     // Write message to stderr. We do this before formatting into a
     // variable-size buffer so that we still get some information if
     // formatting into the variable-sized buffer fails.
-#if !defined(NDEBUG) || !defined(LIBCXXABI_BAREMETAL)
+#if 0
     {
         fprintf(stderr, "libc++abi: ");
         va_list list;

For me it looks like setting LIBCXXABI_BAREMETAL was enough, so maybe NDEBUG is already set? (I'm not sure why they do !x || !y instead of x && y, it's a bit confusing.)

I noticed from grepping LIBCXXABI_BAREMETAL in llvm that this flag seems to change some other behavior, but it's code I'm not familiar with. It makes sense, as the browser wasm runtime is de-facto a microcontroller, but it should probably be reviewed as I don't know the inner workings of how memory is managed there.

Regarding _LIBCPP_AVAILABILITY_HAS_VERBOSE_ABORT=0 , I realise I haven't been very clear. This is not an SDK compile time flag. This is a define in the __availability header that is included at user compile time as part of libcxx, unlike libcxxabi which is binary linked against (and thus needs to be patched at SDK build time). The header is copied over as part of libcxx when the SDK is installed.

A full solution would need to both compile libcxxabi with LIBCXXABI_BAREMETAL (or suitable alternative), and patch the headers to set _LIBCPP_AVAILABILITY_HAS_VERBOSE_ABORT in case the user is compiling without runtime support.

@@ -205,7 +206,8 @@ LIBCXX_CMAKE_FLAGS = \
-DLIBCXX_ENABLE_SHARED:BOOL=$(2) \
-DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY:BOOL=OFF \
-DLIBCXX_ENABLE_EXCEPTIONS:BOOL=OFF \
-DLIBCXX_ENABLE_FILESYSTEM:BOOL=ON \
-DLIBCXX_ENABLE_FILESYSTEM:BOOL=OFF \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

@yamt
Copy link
Contributor

yamt commented Jul 13, 2024

does this disable the following message?

libc++abi: bad_alloc was thrown in -fno-exceptions mode

while i agree a library should not use stderr in general,
i feel it's a bit user-unfriendly as we usually don't have exceptions.
is there a way for user app to override behavior?

@yamt
Copy link
Contributor

yamt commented Jul 18, 2024

does this disable the following message?

libc++abi: bad_alloc was thrown in -fno-exceptions mode

while i agree a library should not use stderr in general, i feel it's a bit user-unfriendly as we usually don't have exceptions. is there a way for user app to override behavior?

answering myself, it's easy for apps to override "abort_message".

extern "C" void abort_message(const char *fmt, ...)
{
}

isn't it enough for your purpose?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants