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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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? | |
1414 | 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; } } |
lldb/tools/lldb-vscode/lldb-vscode.cpp | ||
---|---|---|
1414 | don't forget to apply Greg's suggestion |
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?
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...
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.
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. |
lldb/tools/lldb-vscode/lldb-vscode.cpp | ||
---|---|---|
1418 | 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. |
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 | ||
1392–1393 | 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. |
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.
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 | ||
---|---|---|
1362 | All other local variables use the same name, can we rename this to "inheritEnvironment"? | |
1393 | The other option here would be to add "inheritEnvironment" to the SBLaunchInfo? |
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. |
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.
This comment seems to need fixing, probably due to copying from a previous test case file.