Re: [Lldb-commits] [lldb] b4a6e63 - [lldb/Target] Rework the way the inferior environment is created

[Pavel Labath via lldb-commits]

On 25/03/2020 01:04, Adrian McCarthy wrote:
> I took a stab at this, but I'm not seeing any new test failures. 

That is odd.

I was doing some stuff on windows today, so I figured I'd take a stab at
this. I was kind of right that the check in ProcessLauncher windows
prevents us from passing an empty environment.

However, the interesting part starts when I tried to remove that check.
Then the test started behaving nondeterministically -- sometimes passing
and sometimes failing due to ERROR_INVALID_PARAMETER being returned from
CreateProcessW. I can see how windows might need some environment
variables to start up a process correctly, but I do not understand why
this should be nondeterministic...

This is beyond my knowledge of windows. It might be interesting to
reduce this to a simple test case (independent of lldb) and show it to
some windows expert.

I am attaching a patch with the lldb changes I've made, in case you want
to play around with it.

> I assume
> the problem you're seeing is in TestSettings.py, but I've never figured
> out how to run individual Python-based lldb tests since all the
> dotest.py stuff was re-written.

These days we have multiple ways of achieving that. :)

One way would be via the "check-lldb-api-commands-settings" target which
would run all tests under api/commands/settings (i.e. TestSettings.py
and TestQuoting.py).

Another option would be via the lldb-dotest script.
"python bin\lldb-dotest -p TestSettings.py" would just run that single
file. That script passes all of its options to dotest, so you can use
any of the dotest options that you used to use.

pl
From 0e01f02b9b01c9700787c640ac96923acdf0cb0f Mon Sep 17 00:00:00 2001
From: Pavel Labath 
Date: Wed, 25 Mar 2020 13:39:05 +0100
Subject: [PATCH] [lldb] (Almost?) fix TestSettings.test_pass_host_env_vars

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.

After applying this fix, the relevant test starts to pass, but it does
so (at least on my machine) only sporadically. Sometimes, CreateProcessW
seems to fail with ERROR_INVALID_PARAMETER.

My windows-fu is not sufficient to understand what is going on here.
---
 lldb/source/Host/windows/ProcessLauncherWindows.cpp | 6 +-
 lldb/test/API/commands/settings/TestSettings.py | 1 -
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/lldb/source/Host/windows/ProcessLauncherWindows.cpp b/lldb/source/Host/windows/ProcessLauncherWindows.cpp
index 31101944d..77c7bc269 100644
--- a/lldb/source/Host/windows/ProcessLauncherWindows.cpp
+++ b/lldb/source/Host/windows/ProcessLauncherWindows.cpp
@@ -23,9 +23,6 @@ using namespace lldb_private;
 namespace {
 void CreateEnvironmentBuffer(const Environment &env,
  std::vector &buffer) {
-  if (env.size() == 0)
-return;
-
   // Environment buffer is a null terminated list of null terminated strings
   for (const auto &KV : env) {
 std::wstring warg;
@@ -94,8 +91,7 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
 
   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);
diff --git a/lldb/test/API/commands/settings/TestSettings.py b/lldb/test/API/commands/settings/TestSettings.py
index c0cdc085f..29360856a 100644
--- a/lldb/test/API/commands/settings/TestSettings.py
+++ b/lldb/test/API/commands/settings/TestSettings.py
@@ -286,7 +286,6 @@ class SettingsCommandTestCase(TestBase):
 "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()
-- 
2.19.2.windows.1

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [lldb] b4a6e63 - [lldb/Target] Rework the way the inferior environment is created

[Pavel Labath via lldb-commits]

On 25/03/2020 17:51, Adrian McCarthy via lldb-commits wrote:
> 
> 
> On Wed, Mar 25, 2020 at 9:49 AM Adrian McCarthy  > wrote:
> 
> 
> 
> On Wed, Mar 25, 2020 at 9:10 AM Pavel Labath  > wrote:
> 
> On 25/03/2020 01:04, Adrian McCarthy wrote:
> > I took a stab at this, but I'm not seeing any new test failures.
> 
> That is odd.
> 
> I was doing some stuff on windows today, so I figured I'd take a
> stab at
> this. I was kind of right that the check in ProcessLauncher windows
> prevents us from passing an empty environment.
> 
> However, the interesting part starts when I tried to remove that
> check.
> Then the test started behaving nondeterministically -- sometimes
> passing
> and sometimes failing due to ERROR_INVALID_PARAMETER being
> returned from
> CreateProcessW. I can see how windows might need some environment
> variables to start up a process correctly, but I do not
> understand why
> this should be nondeterministic...
> 
> 
> Oh, I have a guess.  CreateProcessW takes a pointer to an
> environment block.  If that pointer is null, the process will
> inherit the parent environment.  If you want to pass it an empty
> environment, you have to have a valid pointer to an empty string (or
> possibly to a string with TWO terminating '\0's).
> 
> 
> Scratch the "or possibly."  You definitely need two terminating zeros. 
> Since it's in UTF-16, it wants two L'\0', which is four consecutive zero
> bytes.
>  

Thanks. This is exactly what was needed. It's not what I would have
expected, as the documentation says this is "null-terminated block of
null-terminated strings" -- in my book that would mean that an empty
list of null-terminated strings ends with a single L'\0'. But I guess
this is a very weird corner case, as an empty environment is literally
the only way to *not* need a double null terminator.

Anyway, D76835 is the patch for that.

pl
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [lldb] b4a6e63 - [lldb/Target] Rework the way the inferior environment is created

[Adrian McCarthy via lldb-commits]

On Wed, Mar 25, 2020 at 9:49 AM Adrian McCarthy  wrote:

>
>
> On Wed, Mar 25, 2020 at 9:10 AM Pavel Labath  wrote:
>
>> On 25/03/2020 01:04, Adrian McCarthy wrote:
>> > I took a stab at this, but I'm not seeing any new test failures.
>>
>> That is odd.
>>
>> I was doing some stuff on windows today, so I figured I'd take a stab at
>> this. I was kind of right that the check in ProcessLauncher windows
>> prevents us from passing an empty environment.
>>
>> However, the interesting part starts when I tried to remove that check.
>> Then the test started behaving nondeterministically -- sometimes passing
>> and sometimes failing due to ERROR_INVALID_PARAMETER being returned from
>> CreateProcessW. I can see how windows might need some environment
>> variables to start up a process correctly, but I do not understand why
>> this should be nondeterministic...
>>
>
> Oh, I have a guess.  CreateProcessW takes a pointer to an environment
> block.  If that pointer is null, the process will inherit the parent
> environment.  If you want to pass it an empty environment, you have to have
> a valid pointer to an empty string (or possibly to a string with TWO
> terminating '\0's).
>

Scratch the "or possibly."  You definitely need two terminating zeros.
Since it's in UTF-16, it wants two L'\0', which is four consecutive zero
bytes.


>
>
>> This is beyond my knowledge of windows. It might be interesting to
>> reduce this to a simple test case (independent of lldb) and show it to
>> some windows expert.
>>
>> I am attaching a patch with the lldb changes I've made, in case you want
>> to play around with it.
>>
>> > I assume
>> > the problem you're seeing is in TestSettings.py, but I've never figured
>> > out how to run individual Python-based lldb tests since all the
>> > dotest.py stuff was re-written.
>>
>> These days we have multiple ways of achieving that. :)
>>
>> One way would be via the "check-lldb-api-commands-settings" target which
>> would run all tests under api/commands/settings (i.e. TestSettings.py
>> and TestQuoting.py).
>>
>> Another option would be via the lldb-dotest script.
>> "python bin\lldb-dotest -p TestSettings.py" would just run that single
>> file. That script passes all of its options to dotest, so you can use
>> any of the dotest options that you used to use.
>>
>
> I would be thrilled if either of those worked for me.  Somehow, check-lldb
> works, but the check-lldb-... doesn't.
>
> The lldb-dotest solution always fails for me because I can't figure out
> WTF it wants for PYTHONHOME and PYTHONPATH.
>
>
>>
>> pl
>>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [lldb] b4a6e63 - [lldb/Target] Rework the way the inferior environment is created

[Adrian McCarthy via lldb-commits]

On Wed, Mar 25, 2020 at 9:10 AM Pavel Labath  wrote:

> On 25/03/2020 01:04, Adrian McCarthy wrote:
> > I took a stab at this, but I'm not seeing any new test failures.
>
> That is odd.
>
> I was doing some stuff on windows today, so I figured I'd take a stab at
> this. I was kind of right that the check in ProcessLauncher windows
> prevents us from passing an empty environment.
>
> However, the interesting part starts when I tried to remove that check.
> Then the test started behaving nondeterministically -- sometimes passing
> and sometimes failing due to ERROR_INVALID_PARAMETER being returned from
> CreateProcessW. I can see how windows might need some environment
> variables to start up a process correctly, but I do not understand why
> this should be nondeterministic...
>

Oh, I have a guess.  CreateProcessW takes a pointer to an environment
block.  If that pointer is null, the process will inherit the parent
environment.  If you want to pass it an empty environment, you have to have
a valid pointer to an empty string (or possibly to a string with TWO
terminating '\0's).


> This is beyond my knowledge of windows. It might be interesting to
> reduce this to a simple test case (independent of lldb) and show it to
> some windows expert.
>
> I am attaching a patch with the lldb changes I've made, in case you want
> to play around with it.
>
> > I assume
> > the problem you're seeing is in TestSettings.py, but I've never figured
> > out how to run individual Python-based lldb tests since all the
> > dotest.py stuff was re-written.
>
> These days we have multiple ways of achieving that. :)
>
> One way would be via the "check-lldb-api-commands-settings" target which
> would run all tests under api/commands/settings (i.e. TestSettings.py
> and TestQuoting.py).
>
> Another option would be via the lldb-dotest script.
> "python bin\lldb-dotest -p TestSettings.py" would just run that single
> file. That script passes all of its options to dotest, so you can use
> any of the dotest options that you used to use.
>

I would be thrilled if either of those worked for me.  Somehow, check-lldb
works, but the check-lldb-... doesn't.

The lldb-dotest solution always fails for me because I can't figure out WTF
it wants for PYTHONHOME and PYTHONPATH.


>
> pl
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [lldb] b4a6e63 - [lldb/Target] Rework the way the inferior environment is created

[Frédéric Riss via lldb-commits]

Here’s the bot log: 
http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/15055/steps/test/logs/stdio

> On Mar 24, 2020, at 5:10 PM, Adrian McCarthy  wrote:
> 
> Oh, and in case I wasn't clear:  I re-enabled the test TestSettings.py 
> locally.
> 
> On Tue, Mar 24, 2020 at 5:04 PM Adrian McCarthy  > wrote:
> I took a stab at this, but I'm not seeing any new test failures.  Can you 
> point me to the specific test and provide a log showing the failures?
> 
> I've been using `ninja check-lldb`, which runs (almost everything) and none 
> of the failures I'm seeing are related to inherit-env.  I assume the problem 
> you're seeing is in TestSettings.py, but I've never figured out how to run 
> individual Python-based lldb tests since all the dotest.py stuff was 
> re-written.  I've started up lldb interactively and tried to emulate the 
> commands that test executes via SBAPI, but everything worked as expected.
> 
> Adrian.
> 
> On Tue, Mar 24, 2020 at 8:19 AM Adrian McCarthy  > wrote:
> I'll take a look this morning.
> 
> On Tue, Mar 24, 2020 at 7:00 AM Pavel Labath  > wrote:
> On 23/03/2020 17:17, Frédéric Riss via lldb-commits wrote:
> > The new testing for “inherit-env=false” is failing on Windows. I skipped 
> > the test for now.
> > 
> > Could it be that it never worked there? (In which case XFail would be a 
> > better resolution)
> > Does anyone have easy access to a Windows build to try it out?
> > 
> > Thanks,
> > Fred
> 
> My guess is that this "defensive check"
>   
> >
> prevents us from passing a completely blank environment. I am tempted to
> just delete it and see what happens, but maybe Adrian is able to do a
> quick test of this?
> 
> pl

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [lldb] b4a6e63 - [lldb/Target] Rework the way the inferior environment is created

[Adrian McCarthy via lldb-commits]

Oh, and in case I wasn't clear:  I re-enabled the test TestSettings.py
locally.

On Tue, Mar 24, 2020 at 5:04 PM Adrian McCarthy  wrote:

> I took a stab at this, but I'm not seeing any new test failures.  Can you
> point me to the specific test and provide a log showing the failures?
>
> I've been using `ninja check-lldb`, which runs (almost everything) and
> none of the failures I'm seeing are related to inherit-env.  I assume the
> problem you're seeing is in TestSettings.py, but I've never figured out how
> to run individual Python-based lldb tests since all the dotest.py stuff was
> re-written.  I've started up lldb interactively and tried to emulate the
> commands that test executes via SBAPI, but everything worked as expected.
>
> Adrian.
>
> On Tue, Mar 24, 2020 at 8:19 AM Adrian McCarthy 
> wrote:
>
>> I'll take a look this morning.
>>
>> On Tue, Mar 24, 2020 at 7:00 AM Pavel Labath  wrote:
>>
>>> On 23/03/2020 17:17, Frédéric Riss via lldb-commits wrote:
>>> > The new testing for “inherit-env=false” is failing on Windows. I
>>> skipped the test for now.
>>> >
>>> > Could it be that it never worked there? (In which case XFail would be
>>> a better resolution)
>>> > Does anyone have easy access to a Windows build to try it out?
>>> >
>>> > Thanks,
>>> > Fred
>>>
>>> My guess is that this "defensive check"
>>> <
>>> https://github.com/llvm/llvm-project/blob/master/lldb/source/Host/windows/ProcessLauncherWindows.cpp#L26
>>> >
>>> prevents us from passing a completely blank environment. I am tempted to
>>> just delete it and see what happens, but maybe Adrian is able to do a
>>> quick test of this?
>>>
>>> pl
>>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [lldb] b4a6e63 - [lldb/Target] Rework the way the inferior environment is created

[Adrian McCarthy via lldb-commits]

I took a stab at this, but I'm not seeing any new test failures.  Can you
point me to the specific test and provide a log showing the failures?

I've been using `ninja check-lldb`, which runs (almost everything) and none
of the failures I'm seeing are related to inherit-env.  I assume the
problem you're seeing is in TestSettings.py, but I've never figured out how
to run individual Python-based lldb tests since all the dotest.py stuff was
re-written.  I've started up lldb interactively and tried to emulate the
commands that test executes via SBAPI, but everything worked as expected.

Adrian.

On Tue, Mar 24, 2020 at 8:19 AM Adrian McCarthy  wrote:

> I'll take a look this morning.
>
> On Tue, Mar 24, 2020 at 7:00 AM Pavel Labath  wrote:
>
>> On 23/03/2020 17:17, Frédéric Riss via lldb-commits wrote:
>> > The new testing for “inherit-env=false” is failing on Windows. I
>> skipped the test for now.
>> >
>> > Could it be that it never worked there? (In which case XFail would be a
>> better resolution)
>> > Does anyone have easy access to a Windows build to try it out?
>> >
>> > Thanks,
>> > Fred
>>
>> My guess is that this "defensive check"
>> <
>> https://github.com/llvm/llvm-project/blob/master/lldb/source/Host/windows/ProcessLauncherWindows.cpp#L26
>> >
>> prevents us from passing a completely blank environment. I am tempted to
>> just delete it and see what happens, but maybe Adrian is able to do a
>> quick test of this?
>>
>> pl
>>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [lldb] b4a6e63 - [lldb/Target] Rework the way the inferior environment is created

[Adrian McCarthy via lldb-commits]

I'll take a look this morning.

On Tue, Mar 24, 2020 at 7:00 AM Pavel Labath  wrote:

> On 23/03/2020 17:17, Frédéric Riss via lldb-commits wrote:
> > The new testing for “inherit-env=false” is failing on Windows. I skipped
> the test for now.
> >
> > Could it be that it never worked there? (In which case XFail would be a
> better resolution)
> > Does anyone have easy access to a Windows build to try it out?
> >
> > Thanks,
> > Fred
>
> My guess is that this "defensive check"
> <
> https://github.com/llvm/llvm-project/blob/master/lldb/source/Host/windows/ProcessLauncherWindows.cpp#L26
> >
> prevents us from passing a completely blank environment. I am tempted to
> just delete it and see what happens, but maybe Adrian is able to do a
> quick test of this?
>
> pl
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [lldb] b4a6e63 - [lldb/Target] Rework the way the inferior environment is created

[Pavel Labath via lldb-commits]

On 23/03/2020 17:17, Frédéric Riss via lldb-commits wrote:
> The new testing for “inherit-env=false” is failing on Windows. I skipped the 
> test for now.
> 
> Could it be that it never worked there? (In which case XFail would be a 
> better resolution)
> Does anyone have easy access to a Windows build to try it out?
> 
> Thanks,
> Fred

My guess is that this "defensive check"

prevents us from passing a completely blank environment. I am tempted to
just delete it and see what happens, but maybe Adrian is able to do a
quick test of this?

pl
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [lldb] b4a6e63 - [lldb/Target] Rework the way the inferior environment is created

[Frédéric Riss via lldb-commits]

The new testing for “inherit-env=false” is failing on Windows. I skipped the 
test for now.

Could it be that it never worked there? (In which case XFail would be a better 
resolution)
Does anyone have easy access to a Windows build to try it out?

Thanks,
Fred

> On Mar 23, 2020, at 7:59 AM, Fred Riss via lldb-commits 
>  wrote:
> 
> 
> Author: Fred Riss
> Date: 2020-03-23T07:58:34-07:00
> New Revision: b4a6e63ea12309bf667d1569a20ec5b081cbf2a4
> 
> URL: 
> https://github.com/llvm/llvm-project/commit/b4a6e63ea12309bf667d1569a20ec5b081cbf2a4
> DIFF: 
> https://github.com/llvm/llvm-project/commit/b4a6e63ea12309bf667d1569a20ec5b081cbf2a4.diff
> 
> LOG: [lldb/Target] Rework the way the inferior environment is created
> 
> Summary:
> The interactions between the environment settings (`target.env-vars`,
> `target.inherit-env`) and the inferior life-cycle are non-obvious
> today. For example, if `target.inherit-env` is set, the `target.env-vars`
> setting will be augmented with the contents of the host environment
> the first time the launch environment is queried (usually at
> launch). After that point, toggling `target.inherit-env` will have no
> effect as there's no tracking of what comes from the host and what is
> a user setting.
> 
> This patch computes the environment every time it is queried rather
> than updating the contents of the `target.env-vars` property. This
> means that toggling the `target.inherit-env` property later will now
> have the intended effect.
> 
> This patch also adds a `target.unset-env-vars` settings that one can
> use to remove variables from the launch environment. Using this, you
> can inherit all but a few of the host environment.
> 
> The way the launch environment is constructed is:
>  1/ if `target.inherit-env` is set, then read the host environment
>  into the launch environment.
>  2/ Remove for the environment the variables listed in
>  `target.unset-env`.
>  3/ Augment the launch environment with the contents of
>  `target.env-vars`. This overrides any common values with the host
>  environment.
> 
> The one functional difference here that could be seen as a regression
> is that `target.env-vars` will not contain the inferior environment
> after launch. The patch implements a better alternative in the
> `target show-launch-environment` command which will return the
> environment computed through the above rules.
> 
> Reviewers: labath, jingham
> 
> Subscribers: lldb-commits
> 
> Tags: #lldb
> 
> Differential Revision: https://reviews.llvm.org/D76470
> 
> Added: 
> 
> 
> Modified: 
>lldb/include/lldb/Target/Target.h
>lldb/source/Commands/CommandObjectTarget.cpp
>lldb/source/Target/Target.cpp
>lldb/source/Target/TargetProperties.td
>lldb/test/API/commands/settings/TestSettings.py
> 
> Removed: 
> 
> 
> 
> 
> diff  --git a/lldb/include/lldb/Target/Target.h 
> b/lldb/include/lldb/Target/Target.h
> index 2e7932f49e6f..77cda4998192 100644
> --- a/lldb/include/lldb/Target/Target.h
> +++ b/lldb/include/lldb/Target/Target.h
> @@ -225,9 +225,12 @@ class TargetProperties : public Properties {
>   void DisableASLRValueChangedCallback();
>   void DisableSTDIOValueChangedCallback();
> 
> +  Environment ComputeEnvironment() const;
> +
>   // Member variables.
>   ProcessLaunchInfo m_launch_info;
>   std::unique_ptr m_experimental_properties_up;
> +  Target *m_target;
> };
> 
> class EvaluateExpressionOptions {
> 
> diff  --git a/lldb/source/Commands/CommandObjectTarget.cpp 
> b/lldb/source/Commands/CommandObjectTarget.cpp
> index c70117c7a80a..95f81fc6cd54 100644
> --- a/lldb/source/Commands/CommandObjectTarget.cpp
> +++ b/lldb/source/Commands/CommandObjectTarget.cpp
> @@ -682,6 +682,41 @@ class CommandObjectTargetDelete : public 
> CommandObjectParsed {
>   OptionGroupBoolean m_cleanup_option;
> };
> 
> +class CommandObjectTargetShowLaunchEnvironment : public CommandObjectParsed {
> +public:
> +  CommandObjectTargetShowLaunchEnvironment(CommandInterpreter &interpreter)
> +  : CommandObjectParsed(
> +interpreter, "target show-launch-environment",
> +"Shows the environment being passed to the process when 
> launched, "
> +"taking info account 3 settings: target.env-vars, "
> +"target.inherit-env and target.unset-env-vars.",
> +nullptr, eCommandRequiresTarget) {}
> +
> +  ~CommandObjectTargetShowLaunchEnvironment() override = default;
> +
> +protected:
> +  bool DoExecute(Args &args, CommandReturnObject &result) override {
> +Target *target = m_exe_ctx.GetTargetPtr();
> +Environment env = target->GetEnvironment();
> +
> +std::vector env_vector;
> +env_vector.reserve(env.size());
> +for (auto &KV : env)
> +  env_vector.push_back(&KV);
> +std::sort(env_vector.begin(), env_vector.end(),
> +  [](Environment::value_type *a, Environment::value_type *b) {
> +return a->first() <