[lldb][windows] make PseudoTerminal::GetPTY return a shared pointer #168730
Conversation
This file contains hidden or 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
Member
|
@llvm/pr-subscribers-lldb Author: Charles Zablit (charles-zablit) ChangesThis patch refactors the Full diff: https://github.com/llvm/llvm-project/pull/168730.diff 11 Files Affected:
diff --git a/lldb/include/lldb/Host/ProcessLaunchInfo.h b/lldb/include/lldb/Host/ProcessLaunchInfo.h
index 25762bc65295d..50a5af604ee26 100644
--- a/lldb/include/lldb/Host/ProcessLaunchInfo.h
+++ b/lldb/include/lldb/Host/ProcessLaunchInfo.h
@@ -118,7 +118,7 @@ class ProcessLaunchInfo : public ProcessInfo {
bool MonitorProcess() const;
- PseudoTerminal &GetPTY() { return *m_pty; }
+ std::shared_ptr<PseudoTerminal> GetPTY() const { return m_pty; }
void SetLaunchEventData(const char *data) { m_event_data.assign(data); }
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp b/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
index 47111c97927c1..8eeb3d61ba78b 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
@@ -215,7 +215,7 @@ PlatformAppleSimulator::DebugProcess(ProcessLaunchInfo &launch_info,
// been used where the secondary side was given as the file to open for
// stdin/out/err after we have already opened the primary so we can
// read/write stdin/out/err.
- int pty_fd = launch_info.GetPTY().ReleasePrimaryFileDescriptor();
+ int pty_fd = launch_info.GetPTY()->ReleasePrimaryFileDescriptor();
if (pty_fd != PseudoTerminal::invalid_fd) {
process_sp->SetSTDIOFileDescriptor(pty_fd);
}
diff --git a/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm b/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
index f3e79d3d56154..297fcd9b17f0a 100644
--- a/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
+++ b/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
@@ -399,18 +399,18 @@ static Status HandleFileAction(ProcessLaunchInfo &launch_info,
case FileAction::eFileActionOpen: {
FileSpec file_spec = file_action->GetFileSpec();
if (file_spec) {
- const int primary_fd = launch_info.GetPTY().GetPrimaryFileDescriptor();
+ const int primary_fd = launch_info.GetPTY()->GetPrimaryFileDescriptor();
if (primary_fd != PseudoTerminal::invalid_fd) {
// Check in case our file action open wants to open the secondary
- FileSpec secondary_spec(launch_info.GetPTY().GetSecondaryName());
+ FileSpec secondary_spec(launch_info.GetPTY()->GetSecondaryName());
if (file_spec == secondary_spec) {
int secondary_fd =
- launch_info.GetPTY().GetSecondaryFileDescriptor();
+ launch_info.GetPTY()->GetSecondaryFileDescriptor();
if (secondary_fd == PseudoTerminal::invalid_fd) {
- if (llvm::Error Err = launch_info.GetPTY().OpenSecondary(O_RDWR))
+ if (llvm::Error Err = launch_info.GetPTY()->OpenSecondary(O_RDWR))
return Status::FromError(std::move(Err));
}
- secondary_fd = launch_info.GetPTY().GetSecondaryFileDescriptor();
+ secondary_fd = launch_info.GetPTY()->GetSecondaryFileDescriptor();
assert(secondary_fd != PseudoTerminal::invalid_fd);
[options setValue:[NSNumber numberWithInteger:secondary_fd]
forKey:key];
diff --git a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
index befc28b09d185..193ea3ca5d219 100644
--- a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
+++ b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
@@ -488,7 +488,7 @@ lldb::ProcessSP PlatformPOSIX::DebugProcess(ProcessLaunchInfo &launch_info,
if (error.Success()) {
// Hook up process PTY if we have one (which we should for local debugging
// with llgs).
- int pty_fd = launch_info.GetPTY().ReleasePrimaryFileDescriptor();
+ int pty_fd = launch_info.GetPTY()->ReleasePrimaryFileDescriptor();
if (pty_fd != PseudoTerminal::invalid_fd) {
process_sp->SetSTDIOFileDescriptor(pty_fd);
LLDB_LOG(log, "hooked up STDIO pty to process");
diff --git a/lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp b/lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
index c182d3d862269..ced709aef6f8d 100644
--- a/lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
+++ b/lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
@@ -235,10 +235,10 @@ lldb::ProcessSP PlatformQemuUser::DebugProcess(ProcessLaunchInfo &launch_info,
if (error.Fail())
return nullptr;
- if (launch_info.GetPTY().GetPrimaryFileDescriptor() !=
+ if (launch_info.GetPTY()->GetPrimaryFileDescriptor() !=
PseudoTerminal::invalid_fd)
process_sp->SetSTDIOFileDescriptor(
- launch_info.GetPTY().ReleasePrimaryFileDescriptor());
+ launch_info.GetPTY()->ReleasePrimaryFileDescriptor());
return process_sp;
}
diff --git a/lldb/source/Plugins/Process/AIX/NativeProcessAIX.cpp b/lldb/source/Plugins/Process/AIX/NativeProcessAIX.cpp
index cd5e3458e60e8..bbf10960f55ae 100644
--- a/lldb/source/Plugins/Process/AIX/NativeProcessAIX.cpp
+++ b/lldb/source/Plugins/Process/AIX/NativeProcessAIX.cpp
@@ -86,8 +86,9 @@ NativeProcessAIX::Manager::Launch(ProcessLaunchInfo &launch_info,
LLDB_LOG(log, "inferior started, now in stopped state");
return std::unique_ptr<NativeProcessAIX>(new NativeProcessAIX(
- pid, launch_info.GetPTY().ReleasePrimaryFileDescriptor(), native_delegate,
- HostInfo::GetArchitecture(HostInfo::eArchKind64), *this, {pid}));
+ pid, launch_info.GetPTY()->ReleasePrimaryFileDescriptor(),
+ native_delegate, HostInfo::GetArchitecture(HostInfo::eArchKind64), *this,
+ {pid}));
}
llvm::Expected<std::unique_ptr<NativeProcessProtocol>>
diff --git a/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp b/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp
index a22083a8a0903..1b63b01693840 100644
--- a/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp
+++ b/lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp
@@ -108,8 +108,8 @@ NativeProcessFreeBSD::Manager::Launch(ProcessLaunchInfo &launch_info,
Info.GetArchitecture().GetArchitectureName());
std::unique_ptr<NativeProcessFreeBSD> process_up(new NativeProcessFreeBSD(
- pid, launch_info.GetPTY().ReleasePrimaryFileDescriptor(), native_delegate,
- Info.GetArchitecture(), m_mainloop));
+ pid, launch_info.GetPTY()->ReleasePrimaryFileDescriptor(),
+ native_delegate, Info.GetArchitecture(), m_mainloop));
status = process_up->SetupTrace();
if (status.Fail())
diff --git a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
index 7ef50da3641b4..c603c42cf09bf 100644
--- a/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ b/lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -304,8 +304,8 @@ NativeProcessLinux::Manager::Launch(ProcessLaunchInfo &launch_info,
return arch_or.takeError();
return std::unique_ptr<NativeProcessLinux>(new NativeProcessLinux(
- pid, launch_info.GetPTY().ReleasePrimaryFileDescriptor(), native_delegate,
- *arch_or, *this, {pid}));
+ pid, launch_info.GetPTY()->ReleasePrimaryFileDescriptor(),
+ native_delegate, *arch_or, *this, {pid}));
}
llvm::Expected<std::unique_ptr<NativeProcessProtocol>>
diff --git a/lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp b/lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
index 8fb15d83117f4..4f874d33f361a 100644
--- a/lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
+++ b/lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
@@ -94,8 +94,8 @@ NativeProcessNetBSD::Manager::Launch(ProcessLaunchInfo &launch_info,
Info.GetArchitecture().GetArchitectureName());
std::unique_ptr<NativeProcessNetBSD> process_up(new NativeProcessNetBSD(
- pid, launch_info.GetPTY().ReleasePrimaryFileDescriptor(), native_delegate,
- Info.GetArchitecture(), m_mainloop));
+ pid, launch_info.GetPTY()->ReleasePrimaryFileDescriptor(),
+ native_delegate, Info.GetArchitecture(), m_mainloop));
status = process_up->SetupTrace();
if (status.Fail())
diff --git a/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp
index 79dd46ba319d6..600f2203c63e1 100644
--- a/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp
@@ -47,9 +47,9 @@ namespace lldb_private {
NativeProcessWindows::NativeProcessWindows(ProcessLaunchInfo &launch_info,
NativeDelegate &delegate,
llvm::Error &E)
- : NativeProcessProtocol(LLDB_INVALID_PROCESS_ID,
- launch_info.GetPTY().ReleasePrimaryFileDescriptor(),
- delegate),
+ : NativeProcessProtocol(
+ LLDB_INVALID_PROCESS_ID,
+ launch_info.GetPTY()->ReleasePrimaryFileDescriptor(), delegate),
ProcessDebugger(), m_arch(launch_info.GetArchitecture()) {
ErrorAsOutParameter EOut(&E);
DebugDelegateSP delegate_sp(new NativeDebugDelegate(*this));
diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp
index 5b0930cf26b77..2c569039624e6 100644
--- a/lldb/source/Target/Platform.cpp
+++ b/lldb/source/Target/Platform.cpp
@@ -1054,7 +1054,7 @@ lldb::ProcessSP Platform::DebugProcess(ProcessLaunchInfo &launch_info,
// been used where the secondary side was given as the file to open for
// stdin/out/err after we have already opened the primary so we can
// read/write stdin/out/err.
- int pty_fd = launch_info.GetPTY().ReleasePrimaryFileDescriptor();
+ int pty_fd = launch_info.GetPTY()->ReleasePrimaryFileDescriptor();
if (pty_fd != PseudoTerminal::invalid_fd) {
process_sp->SetSTDIOFileDescriptor(pty_fd);
}
|
🐧 Linux x64 Test Results
|
| @@ -399,18 +399,18 @@ static Status HandleFileAction(ProcessLaunchInfo &launch_info, | |||
| case FileAction::eFileActionOpen: { | |||
| FileSpec file_spec = file_action->GetFileSpec(); | |||
| if (file_spec) { | |||
| const int primary_fd = launch_info.GetPTY().GetPrimaryFileDescriptor(); | |||
| const int primary_fd = launch_info.GetPTY()->GetPrimaryFileDescriptor(); | |||
Collaborator
There was a problem hiding this comment.
This looks like a worse API than before. The call sites now look like they forgot to check for a nullptr.
Could we keep the old API and have some callers use shared_froom_this()? Or have both APIs?
Member
|
Can you explain in the description why this needs to be a shared pointer? Needing it for another patch is good motivation but it doesn't explain why it's necessary and PRs/commit should be able to stand on their own. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This patch refactors the
PseudoTerminal::GetPTYto return anstd::shared_ptr. This is effectively an NFC patch and is needed for #168729.