Page MenuHomePhabricator

[lldb] Update logic to close inherited file descriptors.
ClosedPublic

Authored by rdhindsa on Jul 9 2021, 1:56 PM.

Details

Summary

This patch adds the support to close all inherited fds into the child process by iterating over /proc/self/fd entries. 

Currently, it iterates over all the files, for file descriptors from 3 to _SC_OPEN_MAX, which causes lldb to be slower. In comparison, the number of file descriptors held by the child process are quite small. Hence this patch makes the child process faster.

Diff Detail

Event Timeline

rdhindsa requested review of this revision.Jul 9 2021, 1:56 PM
rdhindsa created this revision.
rdhindsa updated this revision to Diff 358429.Jul 13 2021, 2:37 PM

Might be worth checking who wrote some of this/nearby code and whether they're still active in the community and see if they can review this - not sure if Pavel is around/has the bandwidth to review this.

clayborg accepted this revision.Aug 12 2021, 1:40 PM

Sorry for the delay. This seems like it should work just fine. Where is this actually used? I thought most clients should be using posix_spawn() as this is the preferred launching mechanism over using fork/exec

This revision is now accepted and ready to land.Aug 12 2021, 1:40 PM

fork is being called in LaunchProcess function in this file(ProcessLauncherPosixFork.cpp) itself.

Thank you for the review!

Sorry for the delay. This seems like it should work just fine. Where is this actually used? I thought most clients should be using posix_spawn() as this is the preferred launching mechanism over using fork/exec

Looks like posix_spawn is used in the macosx host, but not in the posix host (where, as @rdhindsa mentioned, fork is used), for whatever that's worth.

Do modern Linux's not have posix_spawn? If it exists that's a better interface, and lets the system handle a lot of the complicated machinations you have to do by hand if you roll it yourself out of fork and exec.

Do modern Linux's not have posix_spawn? If it exists that's a better interface, and lets the system handle a lot of the complicated machinations you have to do by hand if you roll it yourself out of fork and exec.

https://man7.org/linux/man-pages/man3/posix_spawn.3.html - looks like it's there. I was reporting on the current implementation/adding some detail on the current state of things.

Sure, thanks for the context!

Actually, the posix_spawn usage is only in the darwin Host code, probably because for posix_spawn to work for debugging
you need a couple of non-standard flags that were added on Darwin for us (the CoreOS folks really wanted us to use posix_spawn).
So you probably can't use it on Linux w/o some kernel support.

Jim

MaskRay added a subscriber: MaskRay.EditedAug 17 2021, 4:39 PM

Drive-by: std::filesystem is unavailable for GCC<8. The minimum GCC version is 5, so std::filesystem isn't suitable. You may use include/llvm/Support/FileSystem.h

MaskRay added inline comments.Aug 17 2021, 4:41 PM
lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
174

/proc/self/fd is generally unavailable on non-Linux OSes.

rdhindsa updated this revision to Diff 367067.Aug 17 2021, 5:19 PM

Updated it to use llvm's filesystem.

Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2021, 5:19 PM

LGTM. Anyone else have any issues?

lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
174

which is why the "else" clause is still intact. If the file fails to open, we do what we did before.

MaskRay accepted this revision.Aug 17 2021, 6:26 PM

LGTM. Some nits

lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
148
161

drop parens around fd > 2

164

auto & -> int

168

Cache sysconf(_SC_OPEN_MAX) to avoid repeated calls.

169
JDevlieghere added inline comments.Aug 17 2021, 7:51 PM
lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
148

Can this be a StringRef?

149
155
rdhindsa updated this revision to Diff 367286.Aug 18 2021, 11:42 AM
rdhindsa marked 7 inline comments as done.
rdhindsa marked an inline comment as done.
JDevlieghere accepted this revision.Aug 18 2021, 12:17 PM

Small nit about the comments, but otherwise LGTM

lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
160

nit: Comments should be sentences ending with a period.

167

nit: Same as above.

MaskRay accepted this revision.Aug 18 2021, 12:18 PM
rdhindsa updated this revision to Diff 367311.Aug 18 2021, 12:52 PM

Updated comments as suggested.

This revision was landed with ongoing or failed builds.Aug 19 2021, 2:42 PM
This revision was automatically updated to reflect the committed changes.
labath added a comment.Sep 6 2021, 2:44 AM

BTW, since linux-5.9, there's a close_range(2) syscall. Given that's a pretty recent kernel, it may not be very useful right now, but it sounds like a way to go for the future.