[libc][OSUtil] refactor quick_exit to be an object library everywhere #85955
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-libc Author: Nick Desaulniers (nickdesaulniers) ChangesThe usage of __builtin_unreachable after calls to quick_exit were distressing. Looking into this further, we seem to have header only implementations for CPU Remove darwin support for quick_exit. This isn't being tested, and we can bring Full diff: https://github.com/llvm/llvm-project/pull/85955.diff 14 Files Affected:
diff --git a/libc/src/__support/OSUtil/CMakeLists.txt b/libc/src/__support/OSUtil/CMakeLists.txt
index ca3b3bf1263e0a..94d1042ccbb4a0 100644
--- a/libc/src/__support/OSUtil/CMakeLists.txt
+++ b/libc/src/__support/OSUtil/CMakeLists.txt
@@ -8,23 +8,10 @@ if(NOT TARGET ${target_os_util})
return()
endif()
-# The OSUtil is an object library in GPU mode.
-if(NOT LIBC_TARGET_OS_IS_GPU)
- add_header_library(
- osutil
- HDRS
- io.h
- quick_exit.h
- syscall.h
- DEPENDS
- ${target_os_util}
- )
-else()
- add_object_library(
- osutil
- ALIAS
- ${target_os_util}
- DEPENDS
- ${target_os_util}
- )
-endif()
+add_object_library(
+ osutil
+ ALIAS
+ ${target_os_util}
+ DEPENDS
+ ${target_os_util}
+)
diff --git a/libc/src/__support/OSUtil/baremetal/CMakeLists.txt b/libc/src/__support/OSUtil/baremetal/CMakeLists.txt
index 280ff87cf1470d..23da40326bbb72 100644
--- a/libc/src/__support/OSUtil/baremetal/CMakeLists.txt
+++ b/libc/src/__support/OSUtil/baremetal/CMakeLists.txt
@@ -1,8 +1,9 @@
-add_header_library(
+add_object_library(
baremetal_util
+ SRCS
+ quick_exit.cpp
HDRS
io.h
- quick_exit.h
DEPENDS
libc.src.__support.common
libc.src.__support.CPP.string_view
diff --git a/libc/src/__support/OSUtil/baremetal/quick_exit.h b/libc/src/__support/OSUtil/baremetal/quick_exit.cpp
similarity index 61%
rename from libc/src/__support/OSUtil/baremetal/quick_exit.h
rename to libc/src/__support/OSUtil/baremetal/quick_exit.cpp
index 74f9142e21b81e..f0971d4db352e0 100644
--- a/libc/src/__support/OSUtil/baremetal/quick_exit.h
+++ b/libc/src/__support/OSUtil/baremetal/quick_exit.cpp
@@ -6,16 +6,13 @@
//
//===----------------------------------------------------------------------===//
-#ifndef LLVM_LIBC_SRC___SUPPORT_OSUTIL_BAREMETAL_QUICK_EXIT_H
-#define LLVM_LIBC_SRC___SUPPORT_OSUTIL_BAREMETAL_QUICK_EXIT_H
-
-namespace LIBC_NAMESPACE {
+#include "src/__support/OSUtil/quick_exit.h"
// This is intended to be provided by the vendor.
-extern "C" void __llvm_libc_quick_exit(int status);
+[[noreturn]] extern "C" void __llvm_libc_quick_exit(int status);
-void quick_exit(int status) { __llvm_libc_quick_exit(status); }
+namespace LIBC_NAMESPACE {
-} // namespace LIBC_NAMESPACE
+[[noreturn]] void quick_exit(int status) { __llvm_libc_quick_exit(status); }
-#endif // LLVM_LIBC_SRC___SUPPORT_OSUTIL_BAREMETAL_QUICK_EXIT_H
+} // namespace LIBC_NAMESPACE
diff --git a/libc/src/__support/OSUtil/darwin/CMakeLists.txt b/libc/src/__support/OSUtil/darwin/CMakeLists.txt
index a3e6b93c8e9925..4241bb37684f7c 100644
--- a/libc/src/__support/OSUtil/darwin/CMakeLists.txt
+++ b/libc/src/__support/OSUtil/darwin/CMakeLists.txt
@@ -8,7 +8,6 @@ add_header_library(
darwin_util
HDRS
io.h
- quick_exit.h
syscall.h
DEPENDS
.${LIBC_TARGET_ARCHITECTURE}.darwin_util
diff --git a/libc/src/__support/OSUtil/darwin/quick_exit.h b/libc/src/__support/OSUtil/darwin/quick_exit.h
deleted file mode 100644
index 71647f50def5e2..00000000000000
--- a/libc/src/__support/OSUtil/darwin/quick_exit.h
+++ /dev/null
@@ -1,26 +0,0 @@
-//===--------- Darwin implementation of a quick exit function ---*- 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
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef LLVM_LIBC_SRC___SUPPORT_OSUTIL_DARWIN_QUICK_EXIT_H
-#define LLVM_LIBC_SRC___SUPPORT_OSUTIL_DARWIN_QUICK_EXIT_H
-
-#include "syscall.h" // For internal syscall function.
-
-#include "src/__support/common.h"
-
-namespace LIBC_NAMESPACE {
-
-LIBC_INLINE void quick_exit(int status) {
- for (;;) {
- LIBC_NAMESPACE::syscall_impl<long>(1 /* SYS_exit */, status);
- }
-}
-
-} // namespace LIBC_NAMESPACE
-
-#endif // LLVM_LIBC_SRC___SUPPORT_OSUTIL_DARWIN_QUICK_EXIT_H
diff --git a/libc/src/__support/OSUtil/gpu/CMakeLists.txt b/libc/src/__support/OSUtil/gpu/CMakeLists.txt
index 8e892a7000c9b3..0c89f9223678b1 100644
--- a/libc/src/__support/OSUtil/gpu/CMakeLists.txt
+++ b/libc/src/__support/OSUtil/gpu/CMakeLists.txt
@@ -4,7 +4,6 @@ add_object_library(
quick_exit.cpp
io.cpp
HDRS
- quick_exit.h
io.h
DEPENDS
libc.src.__support.common
diff --git a/libc/src/__support/OSUtil/gpu/quick_exit.cpp b/libc/src/__support/OSUtil/gpu/quick_exit.cpp
index 1a03be0ace6728..af4795905e7868 100644
--- a/libc/src/__support/OSUtil/gpu/quick_exit.cpp
+++ b/libc/src/__support/OSUtil/gpu/quick_exit.cpp
@@ -6,17 +6,14 @@
//
//===----------------------------------------------------------------------===//
-#ifndef LLVM_LIBC_SRC___SUPPORT_OSUTIL_GPU_QUICK_EXIT_H
-#define LLVM_LIBC_SRC___SUPPORT_OSUTIL_GPU_QUICK_EXIT_H
-
-#include "quick_exit.h"
+#include "src/__support/OSUtil/quick_exit.h"
#include "src/__support/RPC/rpc_client.h"
#include "src/__support/macros/properties/architectures.h"
namespace LIBC_NAMESPACE {
-void quick_exit(int status) {
+[[noreturn]] void quick_exit(int status) {
// We want to first make sure the server is listening before we exit.
rpc::Client::Port port = rpc::client.open<RPC_EXIT>();
port.send_and_recv([](rpc::Buffer *) {}, [](rpc::Buffer *) {});
@@ -29,5 +26,3 @@ void quick_exit(int status) {
}
} // namespace LIBC_NAMESPACE
-
-#endif // LLVM_LIBC_SRC___SUPPORT_OSUTIL_GPU_QUICK_EXIT_H
diff --git a/libc/src/__support/OSUtil/gpu/quick_exit.h b/libc/src/__support/OSUtil/gpu/quick_exit.h
deleted file mode 100644
index b51385defbc0de..00000000000000
--- a/libc/src/__support/OSUtil/gpu/quick_exit.h
+++ /dev/null
@@ -1,18 +0,0 @@
-//===---------- GPU implementation of a quick exit function -----*- 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
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef LLVM_LIBC_SRC___SUPPORT_OSUTIL_GPU_QUICK_EXIT_H
-#define LLVM_LIBC_SRC___SUPPORT_OSUTIL_GPU_QUICK_EXIT_H
-
-namespace LIBC_NAMESPACE {
-
-void quick_exit(int status);
-
-} // namespace LIBC_NAMESPACE
-
-#endif // LLVM_LIBC_SRC___SUPPORT_OSUTIL_GPU_QUICK_EXIT_H
diff --git a/libc/src/__support/OSUtil/linux/CMakeLists.txt b/libc/src/__support/OSUtil/linux/CMakeLists.txt
index c27f9be7464894..239d115704927b 100644
--- a/libc/src/__support/OSUtil/linux/CMakeLists.txt
+++ b/libc/src/__support/OSUtil/linux/CMakeLists.txt
@@ -4,11 +4,12 @@ endif()
add_subdirectory(${LIBC_TARGET_ARCHITECTURE})
-add_header_library(
+add_object_library(
linux_util
+ SRCS
+ quick_exit.cpp
HDRS
io.h
- quick_exit.h
syscall.h
DEPENDS
.${LIBC_TARGET_ARCHITECTURE}.linux_${LIBC_TARGET_ARCHITECTURE}_util
diff --git a/libc/src/__support/OSUtil/linux/quick_exit.h b/libc/src/__support/OSUtil/linux/quick_exit.h
deleted file mode 100644
index 432395584d8467..00000000000000
--- a/libc/src/__support/OSUtil/linux/quick_exit.h
+++ /dev/null
@@ -1,35 +0,0 @@
-//===---------- Linux implementation of a quick exit function ---*- 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
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef LLVM_LIBC_SRC___SUPPORT_OSUTIL_LINUX_QUICK_EXIT_H
-#define LLVM_LIBC_SRC___SUPPORT_OSUTIL_LINUX_QUICK_EXIT_H
-
-#include "syscall.h" // For internal syscall function.
-
-#include "src/__support/common.h"
-
-#include <sys/syscall.h> // For syscall numbers.
-
-namespace LIBC_NAMESPACE {
-
-// mark as no_stack_protector for x86 since TLS can be torn down before calling
-// quick_exit so that the stack protector canary cannot be loaded.
-#ifdef LIBC_TARGET_ARCH_IS_X86
-__attribute__((no_stack_protector))
-#endif
-LIBC_INLINE void
-quick_exit(int status) {
- for (;;) {
- LIBC_NAMESPACE::syscall_impl<long>(SYS_exit_group, status);
- LIBC_NAMESPACE::syscall_impl<long>(SYS_exit, status);
- }
-}
-
-} // namespace LIBC_NAMESPACE
-
-#endif // LLVM_LIBC_SRC___SUPPORT_OSUTIL_LINUX_QUICK_EXIT_H
diff --git a/libc/src/__support/OSUtil/quick_exit.h b/libc/src/__support/OSUtil/quick_exit.h
index 6c59c1afcda254..e445917059c3e1 100644
--- a/libc/src/__support/OSUtil/quick_exit.h
+++ b/libc/src/__support/OSUtil/quick_exit.h
@@ -9,17 +9,10 @@
#ifndef LLVM_LIBC_SRC___SUPPORT_OSUTIL_QUICK_EXIT_H
#define LLVM_LIBC_SRC___SUPPORT_OSUTIL_QUICK_EXIT_H
-#include "src/__support/macros/properties/architectures.h"
+namespace LIBC_NAMESPACE {
-#if defined(LIBC_TARGET_ARCH_IS_GPU)
-#include "gpu/quick_exit.h"
-#elif defined(__APPLE__)
-#include "darwin/quick_exit.h"
-#elif defined(__linux__)
-#include "linux/quick_exit.h"
-#elif defined(__ELF__)
-// TODO: Ideally we would have LIBC_TARGET_OS_IS_BAREMETAL.
-#include "baremetal/quick_exit.h"
-#endif
+[[noreturn]] void quick_exit(int status);
+
+}
#endif // LLVM_LIBC_SRC___SUPPORT_OSUTIL_QUICK_EXIT_H
diff --git a/libc/src/stdlib/CMakeLists.txt b/libc/src/stdlib/CMakeLists.txt
index bd0bcffe0045d1..71449f9bfc9d5f 100644
--- a/libc/src/stdlib/CMakeLists.txt
+++ b/libc/src/stdlib/CMakeLists.txt
@@ -372,10 +372,11 @@ add_entrypoint_object(
CXX_STANDARD
20 # For constinit of the atexit callback list.
DEPENDS
- libc.src.__support.fixedvector
+ libc.src.__support.CPP.new
+ libc.src.__support.OSUtil.osutil
libc.src.__support.blockstore
+ libc.src.__support.fixedvector
libc.src.__support.threads.mutex
- libc.src.__support.CPP.new
)
add_entrypoint_object(
diff --git a/libc/src/stdlib/_Exit.cpp b/libc/src/stdlib/_Exit.cpp
index 85684d1e90879e..233af209739244 100644
--- a/libc/src/stdlib/_Exit.cpp
+++ b/libc/src/stdlib/_Exit.cpp
@@ -13,9 +13,8 @@
namespace LIBC_NAMESPACE {
-LLVM_LIBC_FUNCTION(void, _Exit, (int status)) {
+[[noreturn]] LLVM_LIBC_FUNCTION(void, _Exit, (int status)) {
quick_exit(status);
- __builtin_unreachable();
}
} // namespace LIBC_NAMESPACE
diff --git a/libc/src/stdlib/exit.cpp b/libc/src/stdlib/exit.cpp
index cc5ae6648d11f2..94cf2ae055fc30 100644
--- a/libc/src/stdlib/exit.cpp
+++ b/libc/src/stdlib/exit.cpp
@@ -16,10 +16,9 @@ namespace internal {
void call_exit_callbacks();
}
-LLVM_LIBC_FUNCTION(void, exit, (int status)) {
+[[noreturn]] LLVM_LIBC_FUNCTION(void, exit, (int status)) {
internal::call_exit_callbacks();
quick_exit(status);
- __builtin_unreachable();
}
} // namespace LIBC_NAMESPACE
|
nickdesaulniers
commented
Mar 20, 2024
jhuber6
approved these changes
Mar 20, 2024
Comment on lines
-9
to
-10
| #ifndef LLVM_LIBC_SRC___SUPPORT_OSUTIL_GPU_QUICK_EXIT_H | ||
| #define LLVM_LIBC_SRC___SUPPORT_OSUTIL_GPU_QUICK_EXIT_H |
There was a problem hiding this comment.
Wonder how these got here, probably some victim of copy paste.
nickdesaulniers
force-pushed
the
quick_exit
branch
from
fc6b569
to
04083c8
Compare
SchrodingerZhu
approved these changes
Mar 20, 2024
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
jhuber6
reviewed
Mar 20, 2024
Comment on lines
18
to
21
| __attribute__((no_stack_protector)) | ||
| #endif | ||
| LIBC_INLINE void | ||
| quick_exit(int status) { | ||
| __attribute__((noreturn)) | ||
| void quick_exit(int status) { |
There was a problem hiding this comment.
Any way we could use C++ [[]] syntax for these?
There was a problem hiding this comment.
I think [[clang::no_stack_protector]] or [[gnu::no_stack_protector]] should work.
There was a problem hiding this comment.
Actually, [[gnu::no_stack_protector]] was added to clang on our request for this.
There was a problem hiding this comment.
I see, adding attributes is fairly trivial, but unlike the GPU build we can't rely on an up-to-date compiler so I guess we'll just need to keep it as-is.
There was a problem hiding this comment.
yeah, I hit an issue with having these be repeated as separate attribute expressions, but I think I know how to fix that.
There was a problem hiding this comment.
done in 9ceeba4 PTAL
There was a problem hiding this comment.
the issue was, clang errors if you mix GNU C __attribute__s with C23/C++20 [[]] attributes.
There was a problem hiding this comment.
-- The C compiler identification is Clang 16.0.6
llvm-project/github-pull-requests/libc/src/__support/OSUtil/linux/quick_exit.cpp:18:3: error: unknown attribute 'no_stack_protector' ignored [-Werror,-Wunknown-attributes]
[[gnu::no_stack_protector]]
^~~~~~~~~~~~~~~~~~~~~~~
So ironically, no_stack_protector was a function attribute in clang first, then GCC added support, then clang had to match the gnu:: prefix later. GCC does not recognize the clang:: attribute prefix. All the issues with compiler version compatibility are avoided by just using the GNU C extension syntax for attributes.
So I'm going to revert 9ceeba4.
There was a problem hiding this comment.
Yeah, probably should've been more clear. This will work fine if you require runtimes builds of clang since it has the change, but not with the minimum clang version we support.
|
TODO: breaks bazel! |
nickdesaulniers
force-pushed
the
quick_exit
branch
from
04083c8
to
fb7baf7
Compare
nickdesaulniers
force-pushed
the
quick_exit
branch
from
a6d8bb8
to
1016b8a
Compare
michaelrj-google
approved these changes
Mar 22, 2024
petrhosek
reviewed
Mar 22, 2024
The usage of __builtin_unreachable after calls to quick_exit were distressing. If a function is properly marked [[noreturn]] then __builtin_unreachable is not necessary. Looking into this further, we seem to have header only implementations for CPU targets. The inline nature of these functions is curious; we're going to exit, it doesn't matter if we need to pay the call of a function or not. If we just make these functions have distinct TUs rather than be header only, we can clean up the cmake rules for quick_exit which were different between CPU and GPU. Remove darwin support for quick_exit. This isn't being tested, and we can bring it back when necessary.
nickdesaulniers
force-pushed
the
quick_exit
branch
from
1016b8a
to
f2ac0c3
Compare
|
✅ With the latest revision this PR passed the Python code formatter. |
petrhosek
approved these changes
Mar 25, 2024
|
Hi this patch breaks Failed build: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8752376881927918433/overview |
This should be trivial to fix-forward instead of revert, see https://godbolt.org/z/zv9oqnrd9. |
Let me know if 630283c fixes it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The usage of __builtin_unreachable after calls to quick_exit were distressing.
If a function is properly marked [[noreturn]] then __builtin_unreachable is not
necessary.
Looking into this further, we seem to have header only implementations for CPU
targets. The inline nature of these functions is curious; we're going to exit,
it doesn't matter if we need to pay the call of a function or not. If we just
make these functions have distinct TUs rather than be header only, we can clean
up the cmake rules for quick_exit which were different between CPU and GPU.
Remove darwin support for quick_exit. This isn't being tested, and we can bring
it back when necessary.