[Lldb-commits] [PATCH] D76835: [lldb] Fix TestSettings.test_pass_host_env_vars on windows
This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.
Closed by commit rG908f78f3c198: [lldb] Fix
TestSettings.test_pass_host_env_vars on windows (authored by labath).
Changed prior to commit:
https://reviews.llvm.org/D76835?vs=252785=253594#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76835/new/
https://reviews.llvm.org/D76835
Files:
lldb/source/Host/windows/ProcessLauncherWindows.cpp
lldb/test/API/commands/settings/TestSettings.py
Index: lldb/test/API/commands/settings/TestSettings.py
===
--- lldb/test/API/commands/settings/TestSettings.py
+++ lldb/test/API/commands/settings/TestSettings.py
@@ -286,7 +286,6 @@
"Environment variable 'MY_ENV_VAR' successfully passed."])
@skipIfRemote # it doesn't make sense to send host env to remote target
-@skipIf(oslist=["windows"])
def test_pass_host_env_vars(self):
"""Test that the host env vars are passed to the launched process."""
self.build()
Index: lldb/source/Host/windows/ProcessLauncherWindows.cpp
===
--- lldb/source/Host/windows/ProcessLauncherWindows.cpp
+++ lldb/source/Host/windows/ProcessLauncherWindows.cpp
@@ -23,10 +23,9 @@
namespace {
void CreateEnvironmentBuffer(const Environment ,
std::vector ) {
- if (env.size() == 0)
-return;
-
- // Environment buffer is a null terminated list of null terminated strings
+ // The buffer is a list of null-terminated UTF-16 strings, followed by an
+ // extra L'\0' (two bytes of 0). An empty environment must have one
+ // empty string, followed by an extra L'\0'.
for (const auto : env) {
std::wstring warg;
if (llvm::ConvertUTF8toWide(Environment::compose(KV), warg)) {
@@ -38,6 +37,9 @@
// One null wchar_t (to end the block) is two null bytes
buffer.push_back(0);
buffer.push_back(0);
+ // Insert extra two bytes, just in case the environment was empty.
+ buffer.push_back(0);
+ buffer.push_back(0);
}
bool GetFlattenedWindowsCommandString(Args args, std::string ) {
@@ -94,8 +96,7 @@
LPVOID env_block = nullptr;
::CreateEnvironmentBuffer(launch_info.GetEnvironment(), environment);
- if (!environment.empty())
-env_block = environment.data();
+ env_block = environment.data();
executable = launch_info.GetExecutableFile().GetPath();
GetFlattenedWindowsCommandString(launch_info.GetArguments(), commandLine);
Index: lldb/test/API/commands/settings/TestSettings.py
===
--- lldb/test/API/commands/settings/TestSettings.py
+++ lldb/test/API/commands/settings/TestSettings.py
@@ -286,7 +286,6 @@
"Environment variable 'MY_ENV_VAR' successfully passed."])
@skipIfRemote # it doesn't make sense to send host env to remote target
-@skipIf(oslist=["windows"])
def test_pass_host_env_vars(self):
"""Test that the host env vars are passed to the launched process."""
self.build()
Index: lldb/source/Host/windows/ProcessLauncherWindows.cpp
===
--- lldb/source/Host/windows/ProcessLauncherWindows.cpp
+++ lldb/source/Host/windows/ProcessLauncherWindows.cpp
@@ -23,10 +23,9 @@
namespace {
void CreateEnvironmentBuffer(const Environment ,
std::vector ) {
- if (env.size() == 0)
-return;
-
- // Environment buffer is a null terminated list of null terminated strings
+ // The buffer is a list of null-terminated UTF-16 strings, followed by an
+ // extra L'\0' (two bytes of 0). An empty environment must have one
+ // empty string, followed by an extra L'\0'.
for (const auto : env) {
std::wstring warg;
if (llvm::ConvertUTF8toWide(Environment::compose(KV), warg)) {
@@ -38,6 +37,9 @@
// One null wchar_t (to end the block) is two null bytes
buffer.push_back(0);
buffer.push_back(0);
+ // Insert extra two bytes, just in case the environment was empty.
+ buffer.push_back(0);
+ buffer.push_back(0);
}
bool GetFlattenedWindowsCommandString(Args args, std::string ) {
@@ -94,8 +96,7 @@
LPVOID env_block = nullptr;
::CreateEnvironmentBuffer(launch_info.GetEnvironment(), environment);
- if (!environment.empty())
-env_block = environment.data();
+ env_block = environment.data();
executable = launch_info.GetExecutableFile().GetPath();
GetFlattenedWindowsCommandString(launch_info.GetArguments(), commandLine);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D76835: [lldb] Fix TestSettings.test_pass_host_env_vars on windows
labath marked 4 inline comments as done.
labath added inline comments.
Comment at: lldb/source/Host/windows/ProcessLauncherWindows.cpp:41
+ // double null.
+ if (env.empty()) {
+buffer.push_back(0);
amccarth wrote:
> There would be no harm in always adding the extra terminator.
Yeah, I wondered myself what would be less confusing... inserting a seemingly
needless terminator, or cluttering the code with an extra if...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76835/new/
https://reviews.llvm.org/D76835
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D76835: [lldb] Fix TestSettings.test_pass_host_env_vars on windows
amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.
The code is correct, so I'm approving, but I suggest a couple fixes to the
comments before committing..
Comment at: lldb/source/Host/windows/ProcessLauncherWindows.cpp:27
+ // Environment buffer is a list of null-terminated list of strings. It ends
+ // with a double null.
for (const auto : env) {
You have an extra "list of" between null-terminated and "strings". And I think
we should point out that it's UTF-16 encoded, so maybe:
```
// The buffer is a list of null-terminated UTF-16 strings, followed by an
// extra L'\0' (two bytes of 0). An empty environment must have one
// empty string, followed by an extra L'\0'.
```
Comment at: lldb/source/Host/windows/ProcessLauncherWindows.cpp:39
buffer.push_back(0);
+ // If the environment was empty, we must insert an extra byte to ensure a
+ // double null.
"... an extra **two** bytes ..."
Comment at: lldb/source/Host/windows/ProcessLauncherWindows.cpp:41
+ // double null.
+ if (env.empty()) {
+buffer.push_back(0);
There would be no harm in always adding the extra terminator.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76835/new/
https://reviews.llvm.org/D76835
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D76835: [lldb] Fix TestSettings.test_pass_host_env_vars on windows
friss added a comment. Thanks for fixing this Pavel! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76835/new/ https://reviews.llvm.org/D76835 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D76835: [lldb] Fix TestSettings.test_pass_host_env_vars on windows
labath created this revision.
labath added reviewers: amccarth, friss.
Herald added a project: LLDB.
A defensive check in ProcessLauncherWindows meant that we would never
attempt to launch a process with a completely empty environment -- the
host environment would be used instead. Insted, I make function add an
extra null wchar_t at the end of an empty environment. The documentation
on this is a bit fuzzy, but it seems to be what is needed to make
windows accept these kinds of environments.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D76835
Files:
lldb/source/Host/windows/ProcessLauncherWindows.cpp
lldb/test/API/commands/settings/TestSettings.py
Index: lldb/test/API/commands/settings/TestSettings.py
===
--- lldb/test/API/commands/settings/TestSettings.py
+++ lldb/test/API/commands/settings/TestSettings.py
@@ -286,7 +286,6 @@
"Environment variable 'MY_ENV_VAR' successfully passed."])
@skipIfRemote # it doesn't make sense to send host env to remote target
-@skipIf(oslist=["windows"])
def test_pass_host_env_vars(self):
"""Test that the host env vars are passed to the launched process."""
self.build()
Index: lldb/source/Host/windows/ProcessLauncherWindows.cpp
===
--- lldb/source/Host/windows/ProcessLauncherWindows.cpp
+++ lldb/source/Host/windows/ProcessLauncherWindows.cpp
@@ -23,10 +23,8 @@
namespace {
void CreateEnvironmentBuffer(const Environment ,
std::vector ) {
- if (env.size() == 0)
-return;
-
- // Environment buffer is a null terminated list of null terminated strings
+ // Environment buffer is a list of null-terminated list of strings. It ends
+ // with a double null.
for (const auto : env) {
std::wstring warg;
if (llvm::ConvertUTF8toWide(Environment::compose(KV), warg)) {
@@ -38,6 +36,12 @@
// One null wchar_t (to end the block) is two null bytes
buffer.push_back(0);
buffer.push_back(0);
+ // If the environment was empty, we must insert an extra byte to ensure a
+ // double null.
+ if (env.empty()) {
+buffer.push_back(0);
+buffer.push_back(0);
+ }
}
bool GetFlattenedWindowsCommandString(Args args, std::string ) {
@@ -94,8 +98,7 @@
LPVOID env_block = nullptr;
::CreateEnvironmentBuffer(launch_info.GetEnvironment(), environment);
- if (!environment.empty())
-env_block = environment.data();
+ env_block = environment.data();
executable = launch_info.GetExecutableFile().GetPath();
GetFlattenedWindowsCommandString(launch_info.GetArguments(), commandLine);
Index: lldb/test/API/commands/settings/TestSettings.py
===
--- lldb/test/API/commands/settings/TestSettings.py
+++ lldb/test/API/commands/settings/TestSettings.py
@@ -286,7 +286,6 @@
"Environment variable 'MY_ENV_VAR' successfully passed."])
@skipIfRemote # it doesn't make sense to send host env to remote target
-@skipIf(oslist=["windows"])
def test_pass_host_env_vars(self):
"""Test that the host env vars are passed to the launched process."""
self.build()
Index: lldb/source/Host/windows/ProcessLauncherWindows.cpp
===
--- lldb/source/Host/windows/ProcessLauncherWindows.cpp
+++ lldb/source/Host/windows/ProcessLauncherWindows.cpp
@@ -23,10 +23,8 @@
namespace {
void CreateEnvironmentBuffer(const Environment ,
std::vector ) {
- if (env.size() == 0)
-return;
-
- // Environment buffer is a null terminated list of null terminated strings
+ // Environment buffer is a list of null-terminated list of strings. It ends
+ // with a double null.
for (const auto : env) {
std::wstring warg;
if (llvm::ConvertUTF8toWide(Environment::compose(KV), warg)) {
@@ -38,6 +36,12 @@
// One null wchar_t (to end the block) is two null bytes
buffer.push_back(0);
buffer.push_back(0);
+ // If the environment was empty, we must insert an extra byte to ensure a
+ // double null.
+ if (env.empty()) {
+buffer.push_back(0);
+buffer.push_back(0);
+ }
}
bool GetFlattenedWindowsCommandString(Args args, std::string ) {
@@ -94,8 +98,7 @@
LPVOID env_block = nullptr;
::CreateEnvironmentBuffer(launch_info.GetEnvironment(), environment);
- if (!environment.empty())
-env_block = environment.data();
+ env_block = environment.data();
executable = launch_info.GetExecutableFile().GetPath();
GetFlattenedWindowsCommandString(launch_info.GetArguments(), commandLine);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
