This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Target] Rework the way the inferior environment is created
ClosedPublic

Authored by friss on Mar 19 2020, 6:32 PM.

Details

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/ Augment the launch environment with the contents of
`target.env-vars`. This overrides any common values with the host
environment.
3/ Remove for the environment the variables listed in
`target.unset-env`.

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. It's unclear that this is what any user would have
expected anyway, and we can make it clear(er) in the help that this
setting only contains the values specified by the user.

Diff Detail

Event Timeline

friss created this revision.Mar 19 2020, 6:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2020, 6:32 PM
friss added a comment.Mar 19 2020, 6:33 PM

This needs some tests, but I wanted to put it out there for comments.

I think this is a good change, and is in line with what we have discussed previously. I have just one question about the exact variable interactions here.

Since the raison d'être of the new setting is to be able to "un-inherit" a particular environment variable, I'm wondering if it should only take effect if inherit-env is true (as otherwise you can achieve the same effect by deleting stuff from env-vars)? So instead of the current flow, the sequence would be:

Environment env;
if (inherit-env) {
  env = platform.GetEnvironment();
  for (var : unset-env-vars)
    env.erase(var);
}
for (kv : env-vars)
  env[kv.first] = kv.second;

I'm not insisting on anything, I just want to see which flow would be least surprising.

friss updated this revision to Diff 251670.Mar 20 2020, 9:16 AM

Add tests and update m_launch_info when undet-vars change.
Improve docs.

friss added a comment.Mar 20 2020, 9:18 AM

I think this is a good change, and is in line with what we have discussed previously. I have just one question about the exact variable interactions here.

Since the raison d'être of the new setting is to be able to "un-inherit" a particular environment variable, I'm wondering if it should only take effect if inherit-env is true (as otherwise you can achieve the same effect by deleting stuff from env-vars)? So instead of the current flow, the sequence would be:

I find the logic easier to explain if unset-env-vars applies generally. I don't see a reason other the the host environment to use it, but I also don't see the harm in letting it override env-vars.

Is there any command-based way to see the entire environment that a process will get when it launches? By populating target.env-vars with the inherited environment there was a way to mostly do that, but I don't see that anymore. It seems a shame not to be able to see that...

Also, it does seem weird to me that unset-env-vars would override setting a target.env-var explicitly. What I'm likely to do is say: environment variable 'PUT_HERE_TO_ANNOY_JIM' is always getting set, and I don't want it to be when I'm debugging, so I put:

settings set target.unset-env-vars PUT_HERE_TO_ANNOY_JIM

in my .lldbinit and then after a couple month of debugging happiness, I forget that I put it there.

Then for some reason in a debugging session I want to set it. So I do:

(lldb) env PUT_HERE_TO_ANNOY_JIM="Okay, This One Time..."

But then when I run my process, it doesn't get set. Now I have to go back and remember that I had done the unset thing...

It really seems to me the automatic unsets should happen before the explicit sets. And given the only way you get anything before the automatic sets is the inherited environment, it doesn't make sense to me to apply the unsets if inherit is off.

Is there any command-based way to see the entire environment that a process will get when it launches? By populating target.env-vars with the inherited environment there was a way to mostly do that, but I don't see that anymore. It seems a shame not to be able to see that...

No, there is no way to do this, but it's not really a regression. If you do settings show target.env-vars today before running then you won't see what is going to be passed. Only after the first run will it be populated. This was a surprise to me and I find it highly inconsistent. Maybe I can add another property that would get updated with the computed environment. Do we have anything like read-only properties?

Also, it does seem weird to me that unset-env-vars would override setting a target.env-var explicitly. What I'm likely to do is say: environment variable 'PUT_HERE_TO_ANNOY_JIM' is always getting set, and I don't want it to be when I'm debugging, so I put:

settings set target.unset-env-vars PUT_HERE_TO_ANNOY_JIM

in my .lldbinit and then after a couple month of debugging happiness, I forget that I put it there.

Then for some reason in a debugging session I want to set it. So I do:

(lldb) env PUT_HERE_TO_ANNOY_JIM="Okay, This One Time..."

But then when I run my process, it doesn't get set. Now I have to go back and remember that I had done the unset thing...

It really seems to me the automatic unsets should happen before the explicit sets. And given the only way you get anything before the automatic sets is the inherited environment, it doesn't make sense to me to apply the unsets if inherit is off.

This is pretty far-fetched to me, but I also don't really care. I'm fine with having env-vars override unset-env-vars.

Is there any command-based way to see the entire environment that a process will get when it launches? By populating target.env-vars with the inherited environment there was a way to mostly do that, but I don't see that anymore. It seems a shame not to be able to see that...

No, there is no way to do this, but it's not really a regression. If you do settings show target.env-vars today before running then you won't see what is going to be passed. Only after the first run will it be populated. This was a surprise to me and I find it highly inconsistent. Maybe I can add another property that would get updated with the computed environment. Do we have anything like read-only properties?

No, but I don't think I'd do this with a property anyway. You are asking the target to compute the result of the various settings that affect the environment of a process it might launch. That sounds more like the result of a command ("target show-environment" or something like that.) If we knew how to get the environment from a running process, the same command could show the current environment in a running process, which would sometimes be handy.

I agree the behavior before was pretty unhelpful, so I don't think you need to add a new command to access this in this change set. But we should put it on our list of things to do.

Also, it does seem weird to me that unset-env-vars would override setting a target.env-var explicitly. What I'm likely to do is say: environment variable 'PUT_HERE_TO_ANNOY_JIM' is always getting set, and I don't want it to be when I'm debugging, so I put:

settings set target.unset-env-vars PUT_HERE_TO_ANNOY_JIM

in my .lldbinit and then after a couple month of debugging happiness, I forget that I put it there.

Then for some reason in a debugging session I want to set it. So I do:

(lldb) env PUT_HERE_TO_ANNOY_JIM="Okay, This One Time..."

But then when I run my process, it doesn't get set. Now I have to go back and remember that I had done the unset thing...

It really seems to me the automatic unsets should happen before the explicit sets. And given the only way you get anything before the automatic sets is the inherited environment, it doesn't make sense to me to apply the unsets if inherit is off.

This is pretty far-fetched to me, but I also don't really care. I'm fine with having env-vars override unset-env-vars.

If a variable FOO exists in lldb's environment with the value BAR, and then I have done env FOO=BAZ, when the process launches, FOO will be BAZ, not BAR. So it would be inconsistent to have env-vars override the values of lldb environment variables, but not the entries in unset-env-vars. So I do think that way makes more sense.

friss updated this revision to Diff 251794.Mar 20 2020, 4:12 PM

Add a new target command to dump the launch environment
Make env-vars override unset-env-vars

friss added a comment.Mar 20 2020, 4:15 PM

Is there any command-based way to see the entire environment that a process will get when it launches? By populating target.env-vars with the inherited environment there was a way to mostly do that, but I don't see that anymore. It seems a shame not to be able to see that...

No, there is no way to do this, but it's not really a regression. If you do settings show target.env-vars today before running then you won't see what is going to be passed. Only after the first run will it be populated. This was a surprise to me and I find it highly inconsistent. Maybe I can add another property that would get updated with the computed environment. Do we have anything like read-only properties?

No, but I don't think I'd do this with a property anyway. You are asking the target to compute the result of the various settings that affect the environment of a process it might launch. That sounds more like the result of a command ("target show-environment" or something like that.) If we knew how to get the environment from a running process, the same command could show the current environment in a running process, which would sometimes be handy.

I agree the behavior before was pretty unhelpful, so I don't think you need to add a new command to access this in this change set. But we should put it on our list of things to do.

The command was not hard to add, so I just did it. I used "target show-launch-environment" because I thought "show-environment" would imply that it fetches the inferior environment. I also made env-vars take precedence over unset-env-vars

Except for the suggestion to use eCommandRequiresTarget for the new command, this looks good.

lldb/source/Commands/CommandObjectTarget.cpp
690

"passed to the process when launched"

might be clearer.

693

If you pass eCommandRequiresTarget to the optional last argument here, then you don't have to check in the DoExecute, and you will get consistent error reporting.

701

Pass eCommandRequiresTarget when you make the command and you can omit this checking.

friss updated this revision to Diff 251813.Mar 20 2020, 6:34 PM

Simplify command definition

labath accepted this revision.Mar 23 2020, 12:44 AM

looks good to me.

This revision is now accepted and ready to land.Mar 23 2020, 12:44 AM
This revision was automatically updated to reflect the committed changes.