[lldb-vscode] Add inheritEnvironment option

[wallace]

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.

[• diazhector98]

Add change

[• diazhector98]

Adding the test files

[• diazhector98]

Continued repair

[• diazhector98]

Continue repair

[• diazhector98]

Continue repair

[• diazhector98]

Continue repair

[• diazhector98]

Comment

[• diazhector98]

Adding test for priority

[• diazhector98]

Add Makefile

[• diazhector98]

Removing print

[JDevlieghere]

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.

[clayborg]

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

52

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?

1380

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;
  }
}

[• diazhector98]

Adding support for windows systems

[wallace]

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

1380

don't forget to apply Greg's suggestion

[• diazhector98]

Adding remote process check

[labath]

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?

[clayborg]

In D74636#1916356, @labath wrote:

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?

[wallace]

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?

[labath]

In D74636#1918110, @clayborg wrote:

In D74636#1916356, @labath wrote:

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

In D74636#1918682, @wallace wrote:

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

[wallace]

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]

Using the new SBEnvironment class

[wallace]

improve a comment

[labath]

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]

rebase

[clayborg]

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

1393

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]

Now using the latest SBEnvironment API

[labath]

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

1356–1357

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]

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]

improve comment

[wallace]

test pass now

[wallace]

simplify code

[wallace]

further simplify code. Sorry for the noise

[wallace]

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

[wallace]

fix test failing with python3

[labath]

In D74636#1934466, @wallace wrote:

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.

[wallace]

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?

[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

1336

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

1357

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

[labath]

In D74636#1938283, @clayborg wrote:

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]

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.

lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py

def attach(self, program=None, pid=None, waitFor=None, trace=None,
             self.assertTrue(response['success'],
                             'attach failed (%s)' % (response['message']))
 
     def launch(self, program=None, args=None, cwd=None, env=None,
                stopOnEntry=False, disableASLR=True,
                disableSTDIO=False, shellExpandArguments=False,
                trace=False, initCommands=None, preRunCommands=None,
                stopCommands=None, exitCommands=None,sourcePath= None,
-               debuggerRoot=None, launchCommands=None):
+               debuggerRoot=None, launchCommands=None, inheritEnvironment=False):
         '''Sending launch request to vscode
         '''
 
         # Make sure we disconnect and terminate the VSCode debug adapter,
         # if we throw an exception during the test case
         def cleanup():
             self.vscode.request_disconnect(terminateDebuggee=True)
             self.vscode.terminate()
def launch(self, program=None, args=None, cwd=None, env=None,
             shellExpandArguments=shellExpandArguments,
             trace=trace,
             initCommands=initCommands,
             preRunCommands=preRunCommands,
             stopCommands=stopCommands,
             exitCommands=exitCommands,
             sourcePath=sourcePath,
             debuggerRoot=debuggerRoot,
-            launchCommands=launchCommands)
+            launchCommands=launchCommands,
+            inheritEnvironment=inheritEnvironment)
         if not (response and response['success']):
             self.assertTrue(response['success'],
                             'launch failed (%s)' % (response['message']))
 
     def build_and_launch(self, program, args=None, cwd=None, env=None,
                          stopOnEntry=False, disableASLR=True,
                          disableSTDIO=False, shellExpandArguments=False,
                          trace=False, initCommands=None, preRunCommands=None,
                          stopCommands=None, exitCommands=None,
-                         sourcePath=None, debuggerRoot=None):
+                         sourcePath=None, debuggerRoot=None, inheritEnvironment=False):
         '''Build the default Makefile target, create the VSCode debug adaptor,
            and launch the process.
         '''
         self.build_and_create_debug_adaptor()
         self.assertTrue(os.path.exists(program), 'executable must exist')
 
         self.launch(program, args, cwd, env, stopOnEntry, disableASLR,
                     disableSTDIO, shellExpandArguments, trace,
                     initCommands, preRunCommands, stopCommands, exitCommands,
-                    sourcePath, debuggerRoot)
+                    sourcePath, debuggerRoot, inheritEnvironment=inheritEnvironment)

lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py

def request_initialize(self):
                 self.initialize_body = response['body']
         return response
 
     def request_launch(self, program, args=None, cwd=None, env=None,
                        stopOnEntry=False, disableASLR=True,
                        disableSTDIO=False, shellExpandArguments=False,
                        trace=False, initCommands=None, preRunCommands=None,
                        stopCommands=None, exitCommands=None, sourcePath=None,
-                       debuggerRoot=None, launchCommands=None):
+                       debuggerRoot=None, launchCommands=None, inheritEnvironment=False):
         args_dict = {
             'program': program
         }
         if args:
             args_dict['args'] = args
         if cwd:
             args_dict['cwd'] = cwd
         if env:
def request_launch(self, program, args=None, cwd=None, env=None,
         if exitCommands:
             args_dict['exitCommands'] = exitCommands
         if sourcePath:
             args_dict['sourcePath'] = sourcePath
         if debuggerRoot:
             args_dict['debuggerRoot'] = debuggerRoot
         if launchCommands:
             args_dict['launchCommands'] = launchCommands
+        if inheritEnvironment:
+            args_dict['inheritEnvironment'] = inheritEnvironment
         command_dict = {
             'command': 'launch',
             'type': 'request',
             'arguments': args_dict
         }
         response = self.send_recv(command_dict)
 
         # Wait for a 'process' and 'initialized' event in any order
else:
                                       args=args,
                                       env=options.envs,
                                       cwd=options.workingDir,
                                       debuggerRoot=options.debuggerRoot,
                                       sourcePath=options.sourcePath,
                                       initCommands=options.initCmds,
                                       preRunCommands=options.preRunCmds,
                                       stopCommands=options.stopCmds,
-                                      exitCommands=options.exitCmds)
+                                      exitCommands=options.exitCmds,
+                                      inheritEnvironment=options.inheritEnvironment)
 
     if response['success']:
         if options.sourceBreakpoints:
             source_to_lines = {}
             for file_line in options.sourceBreakpoints:
                 (path, line) = file_line.split(':')
                 if len(path) == 0 or len(line) == 0:
                     print('error: invalid source with line "%s"' %

lldb/test/API/tools/lldb-vscode/completions/main.cpp

int main(int argc, char const *argv[]) {
   std::string str1 = "a";
   std::string str2 = "b";
   std::vector<std::string> vec;
   fun(vec);
   bar bar1 = {2};
   bar* bar2 = &bar1; 
   foo foo1 = {3,&bar1, bar1, NULL};
   return 0; // breakpoint 2
-}
+}
 No newline at end of file

lldb/test/API/tools/lldb-vscode/environmentVariables/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

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

+"""
+Test lldb-vscode environment variables
+"""
+
+
+import lldbvscode_testcase
+import unittest2
+import vscode
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+class TestVSCode_variables(lldbvscode_testcase.VSCodeTestCaseBase):
+    
+    mydir = TestBase.compute_mydir(__file__)
+
+    def checkIfPathIsSame(self,lines, path_env_variable):
+        for line in lines:
+            if line.startswith('PATH='):
+                self.assertTrue(path_env_variable == line, "PATH environment variable not the same")
+                return
+        
+    @skipIfWindows
+    #@skipIfDarwin # Skip this test for now until we can figure out why things aren't working on build bots
+    def test_environment_variable(self):
+        """
+            Tests the environment variables
+        """
+        program = self.getBuildArtifact("a.out")
+        path_env_variable = 'PATH='+os.environ['PATH']
+        self.build_and_launch(program, inheritEnvironment=True)
+        self.continue_to_exit()
+        output = self.get_stdout().encode('utf-8')
+        lines = output.splitlines()
+        self.checkIfPathIsSame(lines, path_env_variable)
+    
+    @skipIfWindows
+    #@skipIfDarwin
+    def test_environment_variable_priority(self):
+        """
+            Tests the environment variables priority
+        """
+        program = self.getBuildArtifact("a.out")
+        path_env_variable = 'PATH=./'
+        self.build_and_launch(program, inheritEnvironment=True, env=[path_env_variable])
+        self.continue_to_exit()
+        output = self.get_stdout().encode('utf-8')
+        lines = output.splitlines()
+        self.checkIfPathIsSame(lines, path_env_variable)
+
+

lldb/test/API/tools/lldb-vscode/environmentVariables/main.cpp

+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+
+extern char **environ;
+
+int main(int argc, char const *argv[], char const *envp[]) {
+    char** env_var_pointer = environ;
+    for (char* env_variable = *env_var_pointer; env_variable; env_variable=*++env_var_pointer) {
+        printf("%s\n", env_variable);
+    }
+    return 0;
+}

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

 #include "llvm/Support/Errno.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include "JSONUtils.h"
 #include "LLDBUtils.h"
 #include "VSCode.h"
 
-#if defined(_WIN32)
+#  if defined(_WIN32)
+extern __declspec(dllimport) char** environ;
+#  else
+extern char** environ;
+#  endif
+
+#if defined(_WIN32)
 #ifndef PATH_MAX
 #define PATH_MAX MAX_PATH
 #endif
 typedef int socklen_t;
 constexpr const char *dev_null_path = "nul";
 
 #else
 constexpr const char *dev_null_path = "/dev/null";
void request_launch(const llvm::json::Object &request) {
   auto arguments = request.getObject("arguments");
   g_vsc.init_commands = GetStrings(arguments, "initCommands");
   g_vsc.pre_run_commands = GetStrings(arguments, "preRunCommands");
   g_vsc.stop_commands = GetStrings(arguments, "stopCommands");
   g_vsc.exit_commands = GetStrings(arguments, "exitCommands");
   auto launchCommands = GetStrings(arguments, "launchCommands");
   g_vsc.stop_at_entry = GetBoolean(arguments, "stopOnEntry", false);
   const auto debuggerRoot = GetString(arguments, "debuggerRoot");
+  bool launchWithDebuggerEnvironment = GetBoolean(arguments, "inheritEnvironment", false);
+
 
   // This is a hack for loading DWARF in .o files on Mac where the .o files
   // in the debug map of the main executable have relative paths which require
   // the lldb-vscode binary to have its working directory set to that relative
   // root for the .o files in order to be able to load debug info.
   if (!debuggerRoot.empty()) {
     llvm::sys::fs::set_current_path(debuggerRoot.data());
   }
 
   SetSourceMapFromArguments(*arguments);
 
   // Run any initialize LLDB commands the user specified in the launch.json.
   // This is run before target is created, so commands can't do anything with
   // the targets - preRunCommands are run with the target.
   g_vsc.RunInitCommands();
 
   lldb::SBError status;
   g_vsc.SetTarget(g_vsc.CreateTargetFromArguments(*arguments, status));
   if (status.Fail()) {
     response["success"] = llvm::json::Value(false);
     EmplaceSafeString(response, "message", status.GetCString());
     g_vsc.SendJSON(llvm::json::Value(std::move(response)));
     return;
   }
 
   // Instantiate a launch info instance for the target.
   g_vsc.launch_info = g_vsc.target.GetLaunchInfo();
 
   // Grab the current working directory if there is one and set it in the
   // launch info.
   const auto cwd = GetString(arguments, "cwd");
   if (!cwd.empty())
     g_vsc.launch_info.SetWorkingDirectory(cwd.data());
 
   // Extract any extra arguments and append them to our program arguments for
   // when we launch
   auto args = GetStrings(arguments, "args");
   if (!args.empty())
     g_vsc.launch_info.SetArguments(MakeArgv(args).data(), true);
 
   // Pass any environment variables along that the user specified.
   auto envs = GetStrings(arguments, "env");
+  if (launchWithDebuggerEnvironment) {
+    std::vector<std::string> vscode_env_variables;
+    int cc;
+    for (cc = 0; environ[cc]; ++cc) {
+      vscode_env_variables.push_back(environ[cc]);
+    }
+    envs.insert(std::end(envs), std::begin(vscode_env_variables), std::end(vscode_env_variables));
+  }
   if (!envs.empty())
     g_vsc.launch_info.SetEnvironmentEntries(MakeArgv(envs).data(), true);
 
   auto flags = g_vsc.launch_info.GetLaunchFlags();
 
   if (GetBoolean(arguments, "disableASLR", true))
     flags |= lldb::eLaunchFlagDisableASLR;
   if (GetBoolean(arguments, "disableSTDIO", false))
     flags |= lldb::eLaunchFlagDisableSTDIO;
   if (GetBoolean(arguments, "shellExpandArguments", false))
     flags |= lldb::eLaunchFlagShellExpandArguments;
   const bool detatchOnError = GetBoolean(arguments, "detachOnError", false);
   g_vsc.launch_info.SetDetachOnError(detatchOnError);
   g_vsc.launch_info.SetLaunchFlags(flags | lldb::eLaunchFlagDebug |

lldb/tools/lldb-vscode/package.json

"debuggers": [
                 "description": "Program working directory.",
                 "default": "${workspaceRoot}"
               },
               "env": {
                 "type": "array",
                 "description": "Additional environment variables.",
                 "default": []
               },
+              "inheritEnvironment": {
+                "type": "boolean",
+                "description": "Inherit the debugger environment when launching a process. Only works for binaries launched directly by LLDB.",
+                "default": false
+              },
               "stopOnEntry": {
                 "type": "boolean",
                 "description": "Automatically stop after launch.",
                 "default": false
               },
               "disableASLR": {
                 "type": "boolean",
                 "description": "Enable or disable Address space layout randomization if the debugger supports it.",