[Lldb-commits] [PATCH] D76009: [lldb/Target] Initialize new targets environment variables from target.env-vars

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D76009#1919554 <https://reviews.llvm.org/D76009#1919554>, @friss wrote:

> In D76009#1919103 <https://reviews.llvm.org/D76009#1919103>, @labath wrote:
>
> > 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76009/new/

https://reviews.llvm.org/D76009