Details

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.

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".

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:

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.

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:

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.

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:

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:

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...