[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

labath added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd9c5613e856c: Update logic to close inherited file descriptors. (authored by rdhindsa). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

rdhindsa updated this revision to Diff 367311. rdhindsa added a comment. Updated comments as suggested. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105732/new/ https://reviews.llvm.org/D105732 Files:

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

JDevlieghere accepted this revision. JDevlieghere added a comment. Small nit about the comments, but otherwise LGTM Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:160 +// Don't close first three entries since they are +// stdin/stdout/stderr +

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

rdhindsa updated this revision to Diff 367286. rdhindsa marked 7 inline comments as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105732/new/ https://reviews.llvm.org/D105732 Files: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

JDevlieghere added inline comments. Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:148 + +std::string proc_fd_path = "/proc/self/fd"; +std::error_code EC; MaskRay wrote: > Can this be a StringRef? Comment at:

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

MaskRay added inline comments. Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:148 + +std::string proc_fd_path = "/proc/self/fd"; +std::error_code EC; Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

MaskRay accepted this revision. MaskRay added a comment. LGTM. Some nits Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:161 +// stdin/stdout/stderr +if ((fd > 2) && !info.GetFileActionForFD(fd) && fd != error_fd) +

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

clayborg added a comment. Herald added a subscriber: JDevlieghere. LGTM. Anyone else have any issues? Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:148 +std::string proc_fd_path = "/proc/self/fd"; +std::filesystem::path fp(proc_fd_path);

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

rdhindsa updated this revision to Diff 367067. rdhindsa added a comment. Herald added a project: LLDB. Updated it to use llvm's filesystem. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105732/new/ https://reviews.llvm.org/D105732 Files:

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

MaskRay added inline comments. Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:148 +std::string proc_fd_path = "/proc/self/fd"; +std::filesystem::path fp(proc_fd_path); /proc/self/fd is generally unavailable on non-Linux OSes. CHANGES

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

MaskRay added a comment. 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` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105732/new/ https://reviews.llvm.org/D105732

Re: [Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

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

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

dblaikie added a comment. In D105732#2942716 , @jingham wrote: > 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

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

jingham added a comment. 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. CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

dblaikie added a comment. In D105732#2942355 , @clayborg wrote: > 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

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

rdhindsa added a comment. fork is being called in LaunchProcess function in this file(ProcessLauncherPosixFork.cpp) itself. Thank you for the review! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105732/new/ https://reviews.llvm.org/D105732

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. 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

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

dblaikie added a comment. 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. CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

rdhindsa added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105732/new/ https://reviews.llvm.org/D105732 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

rdhindsa updated this revision to Diff 358429. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105732/new/ https://reviews.llvm.org/D105732 Files: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp Index: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp