Page MenuHomePhabricator

[lldb-vscode] Add inheritEnvironment option
AbandonedPublic

Authored by wallace on Feb 14 2020, 12:06 PM.

Details

Summary

If no custom launching is used, lldb-vscode launches a program with an empty environment by default. In some scenarios, the user might want to simply use the same environment as the IDE to have a set of working environment variables (e.g. PATH wouldn't be empty). In fact, most DAPs in VSCode have this behavior by default. In other cases the user definitely needs to set their custom environment, which is already supported. To make the first case easier for the user (e.g. not having to copy the PATH to the launch.json every time they want to debug simple programs that rely on PATH), a new option is now offered. inheritEnvironment will launch the program copying its own environment, and it's just a boolean flag.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Adding test for priority

diazhector98 edited the summary of this revision. (Show Details)Feb 14 2020, 3:29 PM
Harbormaster completed remote builds in B46567: Diff 244781.
Harbormaster completed remote builds in B46568: Diff 244782.

Hey Héctor, this is generating a lot of traffic on the lldb-commits mailing list. Phabricator is not really designed to iterate quickly. It's not really a problem per se, but I figured I'd let you know.

wallace retitled this revision from [DO NOT REVIEW] Add inheritEnvironment option to [lldb-vscode] Add inheritEnvironment option.Feb 19 2020, 12:50 PM
wallace edited the summary of this revision. (Show Details)
wallace added a reviewer: clayborg.
wallace added a reviewer: aadsm.
clayborg requested changes to this revision.Feb 19 2020, 3:13 PM

Very close, just need to check for the "host" platform and possibly add windows support, or return an error on windows, if LLVM doesn't have a OS abstracted wrapper that allows access to the current environment.

lldb/test/API/tools/lldb-vscode/environmentVariables/TestVSCode_environmentVariables.py
3

This comment seems to need fixing, probably due to copying from a previous test case file.

lldb/tools/lldb-vscode/lldb-vscode.cpp
57

Will this work on windows or non unix systems? Are there any llvm functions that get the environment that we might be able to use?

1408

I think we need to grab the platform form the target and see if the platform is the "host" platform here and error out if we catch someone trying to forward an environment to another remote platform. Maybe something like:

if (launchWithDebuggerEnvironment) {
  const char *platform_name = g_vsc.target.GetPlatform().GetName();
  if (platform_name && strcmp(platform_name, "host") != 0) {
    response["success"] = llvm::json::Value(false);
    EmplaceSafeString(response, "message", "can't use inheritEnvironment on a remote process");
    g_vsc.SendJSON(llvm::json::Value(std::move(response)));
    return;
  }
}
This revision now requires changes to proceed.Feb 19 2020, 3:13 PM

Adding support for windows systems

wallace requested changes to this revision.Tue, Mar 10, 6:35 PM
wallace added inline comments.
lldb/tools/lldb-vscode/lldb-vscode.cpp
1408

don't forget to apply Greg's suggestion

This revision now requires changes to proceed.Tue, Mar 10, 6:35 PM

Adding remote process check

labath added a subscriber: labath.Wed, Mar 11, 1:21 AM

There's a target.inherit-env setting in lldb (which I believe also works correctly for remote launches). Could you use that instead of reimplementing the feature in vscode?

There's a target.inherit-env setting in lldb (which I believe also works correctly for remote launches). Could you use that instead of reimplementing the feature in vscode?

The real question is if we want launching to fail when the user tries to enable this for remote targets? If we don't care if it fails, then we can use "target.inherit-env", but I kind of like that it does fail as it clearly conveys to the user that their environment won't be passed along instead of just ignoring the request. Thoughts?

Regarding implementation:

The target.inherit-env setting is only effectively used by CommandObjectProcess when launching the target from the command line. lldb-vscode is not following the same codepath and not using that property.

What about exposing the Platform's environment in the API and merging SBPlatform->GetEnvironment() with the launch.json's environment if inheritEnvironment is True? That would be very similar to what is doing CommandObjectProcess


If the user creates the target on their own using custom launchCommands, I imagine these commands would go through CommandObjectProcess, and if target.inherit-env is also set to true, then this would work also for remote cases.

Is that right?

There's a target.inherit-env setting in lldb (which I believe also works correctly for remote launches). Could you use that instead of reimplementing the feature in vscode?

The real question is if we want launching to fail when the user tries to enable this for remote targets? If we don't care if it fails, then we can use "target.inherit-env", but I kind of like that it does fail as it clearly conveys to the user that their environment won't be passed along instead of just ignoring the request. Thoughts?

The real question for me is what do we expect this setting to do. If the intention is to pass the host environment, then that doesn't generally make sense for remote targets, and failing would be good. But if we just want to ensure it inherits "a suitable" environment, then there's no reason to fail. I believe this is what target.inherit-env will do for remote launches, where it will take the environment from the remote platform, and "inherit" that. I think it would make sense if this setting and target.inherit-env behaved the same way...

Regarding implementation:

The target.inherit-env setting is only effectively used by CommandObjectProcess when launching the target from the command line. lldb-vscode is not following the same codepath and not using that property.

What about exposing the Platform's environment in the API and merging SBPlatform->GetEnvironment() with the launch.json's environment if inheritEnvironment is True? That would be very similar to what is doing CommandObjectProcess

SBPlatform::GetEnvironment sounds perfectly reasonable to me.

I am also wondering if D76009 is relevant in any way here...

The original intention of this is to have any working environment, as quite often complex programs require many environment variables that are common to most processes. Having the user specify each of those is a bit too much to ask for, and they are complaining because of this. That said, just using the platform's environment would be enough to have something working. And it the extreme case when other specific env vars are needed, then the user can specify them by hand.

wallace commandeered this revision.Wed, Mar 18, 5:57 PM
wallace edited reviewers, added: diazhector98; removed: wallace.
wallace updated this revision to Diff 251235.Wed, Mar 18, 5:58 PM

Using the new SBEnvironment class

wallace marked 2 inline comments as done.Wed, Mar 18, 6:00 PM
wallace updated this revision to Diff 251236.Wed, Mar 18, 6:01 PM

improve a comment

Harbormaster completed remote builds in B49670: Diff 251236.

Thanks for updating this patch. This seems ok to me (modulo comments here and the other patch), but I think it would be better to move all of the SB changes into that other patch. (the reason I requested this update was to see whether the other patch has all that's needed to implement the given functionality -- and I suspected there would be some bits missing :))

lldb/bindings/interface/SBLaunchInfo.i
65 ↗(On Diff #251236)

This is going to change the mangled name of the function. Either add a new overload or const_cast at the call site. (But if you implement the other comment, neither of those will be needed).

lldb/include/lldb/API/SBEnvironment.h
62 ↗(On Diff #251236)

Usually, these functions are called get() (when they return a pointer) or ref() (for a reference).

lldb/source/API/SBLaunchInfo.cpp
198 ↗(On Diff #251236)

This is a pretty long-winded way of implementing this functionality, as the first thing the other function will do is recreate an Environment object. It would be better to have the other function redirect to this one.

wallace marked an inline comment as done.Thu, Mar 19, 2:37 PM
clayborg added inline comments.Thu, Mar 19, 6:24 PM
lldb/tools/lldb-vscode/lldb-vscode.cpp
1412

We want to have a function in SBEnvironment that takes the "name=value" format:

env.SetEntry("FOO=bar");

We don't want people to have to split this themselves.

wallace updated this revision to Diff 251527.Thu, Mar 19, 6:58 PM

Now using the latest SBEnvironment API

labath added inline comments.Fri, Mar 20, 3:15 AM
lldb/test/API/tools/lldb-vscode/environmentVariables/main.cpp
9

I guess you don't need the envp argument if you're going to use environ. (I'm not sure which one is more portable -- I don't think either of them is fully standardized)

lldb/tools/lldb-vscode/lldb-vscode.cpp
1379–1380

Is there a way to avoid fiddling with this setting? E.g. if we get the initial environment via target.GetPlatform().GetEnvironment() (or not, depending on the value of launchWithDebuggerEnvironment), then I'd expect we should be able to get the right behavior regardless of the value that the user has set for target.inherit-env, and without overwriting the user-set value.

wallace updated this revision to Diff 251754.Fri, Mar 20, 2:01 PM

I added some tests cases to show why I used "settings set target.inherit-env".

There are currently two ways to launch a process. Either with the plain "program" argument,
or with the "launchCommands" argument. The latter is assumed to create a target by executing
arbitrary commands, which may go through CommandObjectProcess.

As by default target.inherit-env is true, if we first set its value to what we got from the
inheritEnvironment argument, then both kinds of launchers would behave the same way.

I haven't found an API for changing this setting, so I ended up invoking the command.

wallace updated this revision to Diff 251756.Fri, Mar 20, 2:05 PM

improve comment

wallace planned changes to this revision.Fri, Mar 20, 2:10 PM
wallace updated this revision to Diff 251760.Fri, Mar 20, 2:14 PM

test pass now

wallace updated this revision to Diff 251762.Fri, Mar 20, 2:17 PM

simplify code

wallace updated this revision to Diff 251766.Fri, Mar 20, 2:23 PM

further simplify code. Sorry for the noise

Harbormaster failed remote builds in B49959: Diff 251760!
Harbormaster failed remote builds in B49961: Diff 251762!
This revision was not accepted when it landed; it landed in state Needs Review.Fri, Mar 20, 7:00 PM
Closed by commit rG4ec6ebabfc3e: [lldb-vscode] Add inheritEnvironment option (authored by Hector Diaz <hdiaz@fb.com>, committed by Walter Erquinigo <waltermelon@fb.com>). · Explain Why
This revision was automatically updated to reflect the committed changes.
wallace reopened this revision.Fri, Mar 20, 7:28 PM

I pushed this by mistake, but I've just reverted the commit.

wallace updated this revision to Diff 251821.Fri, Mar 20, 8:10 PM

fix test failing with python3

I added some tests cases to show why I used "settings set target.inherit-env".

There are currently two ways to launch a process. Either with the plain "program" argument,
or with the "launchCommands" argument. The latter is assumed to create a target by executing
arbitrary commands, which may go through CommandObjectProcess.

As by default target.inherit-env is true, if we first set its value to what we got from the
inheritEnvironment argument, then both kinds of launchers would behave the same way.

Thanks for the explanation. That makes sort of sense, but it does raise the question of the handling of other launch command arguments. What happens if the user specifies the environment (env) in the launch command, but uses the launchCommands method of launching? Will that environment still be applied? And what about the rest of the launch arguments (disable-aslr, stop-on-entry, disable-stdio)? It seems odd to have this special treatment for only a single property.

I haven't found an API for changing this setting, so I ended up invoking the command.

Yeah, I'm afraid we don't have an API for getting/setting settings right now.

TBH I'd prefer to have consistency across all these options regardless of how the target is launched. That means modifying the defaults of the target settings that can be specified via launch.json arguments.

What do you think, @clayborg?

I would either add the option to SBLaunchInfo so it can be specified, or execute the command. If the target is created, it is setting a target specific setting. If had to pick I would add the API to SBLaunchInfo. Whenever I see something that can't be done through the API, I like to add that API if it is warranted. In our case the value in the SBLaunchInfo should probably be stored as a lldb_private::LazyBool which can have the following values:

enum LazyBool { eLazyBoolCalculate = -1, eLazyBoolNo = 0, eLazyBoolYes = 1 };

It would eLazyBoolCalculate to in the launch info, and if it gets set to true or false, then we use that, if it is set to eLazyBoolCalculate we use the target setting.

lldb/tools/lldb-vscode/lldb-vscode.cpp
1359

All other local variables use the same name, can we rename this to "inheritEnvironment"?

1380

The other option here would be to add "inheritEnvironment" to the SBLaunchInfo?

I would either add the option to SBLaunchInfo so it can be specified, or execute the command. If the target is created, it is setting a target specific setting. If had to pick I would add the API to SBLaunchInfo. Whenever I see something that can't be done through the API, I like to add that API if it is warranted. In our case the value in the SBLaunchInfo should probably be stored as a lldb_private::LazyBool which can have the following values:

enum LazyBool { eLazyBoolCalculate = -1, eLazyBoolNo = 0, eLazyBoolYes = 1 };

It would eLazyBoolCalculate to in the launch info, and if it gets set to true or false, then we use that, if it is set to eLazyBoolCalculate we use the target setting.

As of https://reviews.llvm.org/D76045, one can pass nullptr/None as the environment to Launch/LaunchSimple in order to get the "default" environment handling. This is does not work with the SBLaunchInfo, but the same thing can be achieved there (after https://reviews.llvm.org/D76111) by doing launch_info.SetEnvironment(target.GetEnvironment()). So I don't think that another way of inheriting the default environment is needed (or even desired).

The question here isn't really about what can or cannot be done via SBLaunchInfo api (all of this can be done). It's more of the opposite. I.e., what to do if we cannot use SBLaunchInfo because the user wants to do the launch itself (via launchCommands => CLI). I can see how applying the other settings would be useful, but given that launchCommands is a fairly advanced feature, I think it would also be acceptable to leave this up to the user. But in either case, I think we should be consistent, and not apply a random subset of settings.

So, I think it would be best to leave the target.inherit-env alone here, and create a separate patch for the handling of settings in the launchCommands scenario.

lldb/tools/lldb-vscode/package.json
90

Why is this false? It seems true is a much more reasonable default here, and it would be consistent with the normal lldb behavior.

wallace abandoned this revision.Tue, Mar 24, 1:34 PM

With https://reviews.llvm.org/D76470, targets created by lldb-vscode by default are inheriting the debugger's environment. I don't need this change anymore.
We can work on providing a flag that disables that default behavior, but that can also be set by an initCommand if someone really needs it.