-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Support: don't block signals around close if it can be avoided #73009
base: main
Are you sure you want to change the base?
Conversation
Signal blocking originally showed up in 51c2afc ("Support: Don't call close again if we get EINTR"), but it was overzealous -- there are systems where the error is known to be fine. This commit elides signal blocking for said systems (the list is incomplete though). Note close() can still fail for other reasons (like ENOSPC), in which case an error will be returned while the fd slot is cleared up.
@llvm/pr-subscribers-llvm-support Author: Mateusz Guzik (mjguzik) ChangesSignal blocking originally showed up in 51c2afc ("Support: Don't call close again if we get EINTR"), but it was overzealous -- there are systems where the error is known to be fine. This commit elides signal blocking for said systems (the list is incomplete though). Note close() can still fail for other reasons (like ENOSPC), in which case an error will be returned while the fd slot is cleared up. Full diff: https://github.com/llvm/llvm-project/pull/73009.diff 1 Files Affected:
diff --git a/llvm/lib/Support/Unix/Process.inc b/llvm/lib/Support/Unix/Process.inc
index d3d9fb7d71878e1..bc638439754c71e 100644
--- a/llvm/lib/Support/Unix/Process.inc
+++ b/llvm/lib/Support/Unix/Process.inc
@@ -234,6 +234,25 @@ std::error_code Process::FixupStandardFileDescriptors() {
return std::error_code();
}
+// Close a file descriptor while being mindful of EINTR.
+//
+// On Unix systems closing a file descriptor usually starts with removing it
+// from the fd table (or an equivalent structure). This means any error
+// generated past that point will still result in the entry being cleared.
+//
+// Make sure to not bubble up EINTR as there is nothing to do in that case.
+// XXX what about other errors?
+#if defined(__linux__) || defined(__DragonFly__) || defined(__FreeBSD__) || \
+ defined(__NetBSD__) || defined(__OpenBSD__)
+std::error_code Process::SafelyCloseFileDescriptor(int FD) {
+ int EC = 0;
+ if (::close(FD) < 0) {
+ if (errno != EINTR)
+ EC = errno;
+ }
+ return std::error_code(EC, std::generic_category());
+}
+#else
std::error_code Process::SafelyCloseFileDescriptor(int FD) {
// Create a signal set filled with *all* signals.
sigset_t FullSet, SavedSet;
@@ -268,6 +287,7 @@ std::error_code Process::SafelyCloseFileDescriptor(int FD) {
return std::error_code(ErrnoFromClose, std::generic_category());
return std::error_code(EC, std::generic_category());
}
+#endif
bool Process::StandardInIsUserInput() {
return FileDescriptorIsDisplayed(STDIN_FILENO);
|
cc @majnemer |
Is this really a hot path? |
It's not a hot path, it is just an example of numerous artificial problems (at least in terms of syscalls which get invoked). Here is something else to give you a flavor:
For example not only there are clearly avoidable stat calls, but the last one comes after llvm itself unlinked the file. And so on. truss/strace -f output is really bad. |
Who can I prod here? perhaps @MaskRay ? |
// generated past that point will still result in the entry being cleared. | ||
// | ||
// Make sure to not bubble up EINTR as there is nothing to do in that case. | ||
// XXX what about other errors? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should probably explain that signals do not need to be blocked on these systems and that therefore EINTR can be ignored (is it even possible to get eintr?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least on FreeBSD, EINTR
is a documented return value of close(2)
(https://man.freebsd.org/cgi/man.cgi?query=close&sektion=2):
The close() system call will fail if:
[EBADF] The fd argument is not an active descriptor.
[EINTR] An interrupt was received.
[ENOSPC] The underlying object did not fit, cached data was
lost.
[ECONNRESET] The underlying object was a stream socket that was
shut down by the peer before all pending data was
delivered.
In case of any error except EBADF, the supplied file descriptor is
deallocated and therefore is no longer valid.
As noted, in all possible error cases, the file descriptor will no longer be valid afterwards. In case of EINTR
, nothing more can be done anyway, so it does not make sense to retry, for example. So I guess the assumption is that the file will be closed in the background, somehow?
@mjguzik might know more about this logic :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking a various sources it looks like this change to ignore EINTR is correct: Linux manpage says it will be closed https://man7.org/linux/man-pages/man2/close.2.html and https://lwn.net/Articles/576478/ says EINTR will (almost?) never be returned.
On the FreeBSD side, @mjguzik's assertion that it will also close the FD even if EINTR is returned sounds good to me. Not sure about the other BSD implementations but maybe they do the same?
So this change looks good to me but I'd ask that the comment is updated to explain why EINTR can be ignored in more detail (and maybe add a reference to e.g. the Linux manpage).
Signal blocking originally showed up in 51c2afc ("Support: Don't call close again if we get EINTR"), but it was overzealous -- there are systems where the error is known to be fine.
This commit elides signal blocking for said systems (the list is incomplete though).
Note close() can still fail for other reasons (like ENOSPC), in which case an error will be returned while the fd slot is cleared up.