Page MenuHomePhabricator

[lldb/Target] Initialize new targets environment variables from target.env-vars
ClosedPublic

Authored by friss on Mar 11 2020, 10:43 AM.

Details

Summary

The TargetProperties constructor invokes a series of callbacks to
prime the properties from the default ones. The one callback in
charge of updating the inferior environment was commented out
because it crashed.

The reason for the crash is that TargetProperties is a parent class
of Target and the callbacks were invoked using a Target that was
not fully initialized. This patch moves the initial callback
invocations to a separate function that can be called at the end
the Target constructor, thus preventing the crash.

One existing test had to be modified, because the initialization of
the environment properties now take place at the time the target is
created, not at the first use of the environment (usually launch
time).

The added test checks that the LaunchInfo object returned by
the target has been primed with the values from the settings.

Diff Detail

Event Timeline

friss created this revision.Mar 11 2020, 10:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2020, 10:43 AM
labath accepted this revision.Mar 12 2020, 3:43 AM

Another option might be to make TargetProperties a member instead of a base class. I don't think the current setup makes for a particularly clean design. If TargetProperties is among the last Target members being initialized, then calling the callbacks from the constructor should "just work".

lldb/include/lldb/Target/Target.h
214

This name is very nondescript. I think it would be better to name this according to what it does, instead of how it does it. LoadDefaultPropertyValues() ?

This revision is now accepted and ready to land.Mar 12 2020, 3:43 AM
labath removed a reviewer: labath.Mar 12 2020, 4:05 AM

Actually, hang on.

One existing test had to be modified, because the initialization of
the environment properties now take place at the time the target is
created, not at the first use of the environment (usually launch
time).

Does this mean that the target-local value of the target.inherit-env setting no longer has any effect? Currently I can set it after creating a target (but before launching) to stop the process inheriting the default environment:

(lldb) file /usr/bin/env
Current executable set to '/usr/bin/env' (x86_64).
(lldb) set set target.inherit-env false
(lldb) pr la
Process 26684 launched: '/usr/bin/env' (x86_64)
Process 26684 exited with status = 0 (0x00000000)

I take it that after this, that will no longer be possible? It would still be possible to do that by setting the global value of the setting before creating a target, but the target-local setting would be useless (?)

I'm not really sure how these settings are supposed to work, but this does not seem ideal to me.

This revision now requires review to proceed.Mar 12 2020, 4:05 AM
friss added a comment.Mar 12 2020, 8:19 AM

Actually, hang on.

One existing test had to be modified, because the initialization of
the environment properties now take place at the time the target is
created, not at the first use of the environment (usually launch
time).

Just to be clear, in that test the host environment was modified between the creation of the target and the launch which I think is pretty unlikely to matter in practice.

Does this mean that the target-local value of the target.inherit-env setting no longer has any effect? Currently I can set it after creating a target (but before launching) to stop the process inheriting the default environment:

(lldb) file /usr/bin/env
Current executable set to '/usr/bin/env' (x86_64).
(lldb) set set target.inherit-env false
(lldb) pr la
Process 26684 launched: '/usr/bin/env' (x86_64)
Process 26684 exited with status = 0 (0x00000000)

I take it that after this, that will no longer be possible? It would still be possible to do that by setting the global value of the setting before creating a target, but the target-local setting would be useless (?)

The inherited environment is collected the first time the property is accessed and running the callback triggers this. For some reason just doing set show target.env-vars doesn't trigger it though, I'd need to debug this to understand. So yes, with this change, the target.inherit-env setting will take effect at target creation time and not at whatever next point in time the lazy logic is triggered.

One way to fix this is to add another callback for the target.inherit-env property and remove the variables that are in the environment.

I'm not really sure how these settings are supposed to work, but this does not seem ideal to me.

Jim would certainly be able to explain the intent here.

Actually, hang on.

One existing test had to be modified, because the initialization of
the environment properties now take place at the time the target is
created, not at the first use of the environment (usually launch
time).

Just to be clear, in that test the host environment was modified between the creation of the target and the launch which I think is pretty unlikely to matter in practice.

Does this mean that the target-local value of the target.inherit-env setting no longer has any effect? Currently I can set it after creating a target (but before launching) to stop the process inheriting the default environment:

(lldb) file /usr/bin/env
Current executable set to '/usr/bin/env' (x86_64).
(lldb) set set target.inherit-env false
(lldb) pr la
Process 26684 launched: '/usr/bin/env' (x86_64)
Process 26684 exited with status = 0 (0x00000000)

I take it that after this, that will no longer be possible? It would still be possible to do that by setting the global value of the setting before creating a target, but the target-local setting would be useless (?)

The inherited environment is collected the first time the property is accessed and running the callback triggers this. For some reason just doing set show target.env-vars doesn't trigger it though, I'd need to debug this to understand. So yes, with this change, the target.inherit-env setting will take effect at target creation time and not at whatever next point in time the lazy logic is triggered.

One way to fix this is to add another callback for the target.inherit-env property and remove the variables that are in the environment.

I'm not really sure how these settings are supposed to work, but this does not seem ideal to me.

Jim would certainly be able to explain the intent here.

I don't think the current version of the code works very well. You can actually only change the value of target.inherit-env BEFORE the first run. For instance:

 > lldb printenv
warning: Overwriting existing definition for 'p'.
warning: Overwriting existing definition for 't'.
(lldb) target create "printenv"
Current executable set to 'printenv' (x86_64).
(lldb) set set target.in
Available completions:
	target.inherit-env               
	target.input-path                
	target.inline-breakpoint-strategy
(lldb) set set target.inherit-env 0
(lldb) set show target.env-vars
target.env-vars (dictionary of strings) =
(lldb) run
Process 65813 launched: '/tmp/printenv' (x86_64)
0  :OS_ACTIVITY_DT_MODE=enable:
Process 65813 exited with status = 0 (0x00000000) 
(lldb) set set target.inherit-env 1
(lldb) run
Process 65816 launched: '/tmp/printenv' (x86_64)
0  :OS_ACTIVITY_DT_MODE=enable:
Process 65816 exited with status = 0 (0x00000000)

lldb had way more environment variables than that...

And conversely, if you run with inherit-env set to 1 and then set it to 0, you still get all the environment variables.

For this to behave correctly, we would as Fred suggests have to add a callback to setting inherit-env and add or remove the host's variables as appropriate when it changes.

IMO there should really also be a "target create" option for inherit-env. This is usually something you want to do depending on the type of target, and you don't generally change it after the fact. It's awkward to have to change a setting to create this or that target in a certain mode. It would be better if we had predicates for the settings, then you could do something like settings set target[platform=remote].inherit-env 0. But we haven't gotten there yet...

Note, for this to be really useful for remote debugging, lldb should be able to fetch the monitor's environment and use that to prime the target's environment when inherit-env is on. That would be a much more useful thing to do. But that's way outside the scope of this patch.

BTW, don't worry about the OS_ACTIVITY_DT_MODE env var. That's something you have to set when debugging or the Foundation loggging method (NSLog) will go to the system console not to the current terminal, so we force it on all processes on macOS...

friss added a comment.Mar 12 2020, 3:37 PM

I put up a "fix" for the inherit-env issue mentioned here: https://reviews.llvm.org/D76105
It is mostly orthogonal to this patch as Jim showed the behavior today is already broken.

I put up a "fix" for the inherit-env issue mentioned here: https://reviews.llvm.org/D76105
It is mostly orthogonal to this patch as Jim showed the behavior today is already broken.

Yeah, I agree the current behavior is pretty bad, and this patch at least cleans up one aspect of it.

I don't have an immediate use case for the current env-vars behavior. If this helps you in any way, then I don't want to block it just because of that.

friss updated this revision to Diff 251675.Mar 20 2020, 9:37 AM

Make helper function name more descriptive.

friss marked an inline comment as done.Mar 20 2020, 9:37 AM
labath accepted this revision.Mar 23 2020, 1:37 AM
This revision is now accepted and ready to land.Mar 23 2020, 1:37 AM
This revision was automatically updated to reflect the committed changes.