-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[lldb][windows] fix copying instead of using a reference of STARTUPINFOW #170653
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
[lldb][windows] fix copying instead of using a reference of STARTUPINFOW #170653
Conversation
|
@llvm/pr-subscribers-lldb Author: Charles Zablit (charles-zablit) ChangesThis patch fixes a implicit copy instead of using a reference to a STARTUPINFOW instance. This is a followup to #170301. Full diff: https://github.com/llvm/llvm-project/pull/170653.diff 1 Files Affected:
diff --git a/lldb/source/Host/windows/ProcessLauncherWindows.cpp b/lldb/source/Host/windows/ProcessLauncherWindows.cpp
index 2f78ef80f385e..f22a5ffefc19a 100644
--- a/lldb/source/Host/windows/ProcessLauncherWindows.cpp
+++ b/lldb/source/Host/windows/ProcessLauncherWindows.cpp
@@ -198,7 +198,7 @@ llvm::ErrorOr<std::vector<HANDLE>> ProcessLauncherWindows::GetInheritedHandles(
const ProcessLaunchInfo &launch_info, STARTUPINFOEXW &startupinfoex,
HANDLE stdout_handle, HANDLE stderr_handle, HANDLE stdin_handle) {
std::vector<HANDLE> inherited_handles;
- STARTUPINFOW startupinfo = startupinfoex.StartupInfo;
+ STARTUPINFOW &startupinfo = startupinfoex.StartupInfo;
startupinfo.hStdError =
stderr_handle ? stderr_handle : GetStdHandle(STD_ERROR_HANDLE);
|
dzhidzhoev
left a comment
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.
Thank you!
compnerd
left a comment
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.
Would it make more sense to just reference the startupinfoex.StartupInfo locally? It is a bit unobvious that you are intending to mutate that member. Alternatively, we could simply take a STARTUPINFOW & instead of STARTUPINFOEXW & in the parameter list.
For what it's worth, we are doing something similar here: https://github.com/llvm/llvm-project/pull/170653/files#diff-e88204c0b3183c22166588028caaa5bc946c63fc9e90af696f60603325b1d08dR91
Happy to do that, it's going to increase verbosity a bit, but that's fine I think. |
|
Merging this to fix the bots ASAP, I will open up a follow up PR to address #170653 (review). |
This patch fixes a implicit copy instead of using a reference to a STARTUPINFOW instance.
This is a followup to #170301.