Fix TestProcessLaunch.test_environment_with_special_char for Python 3

[zturner]

Summary

There were a number of problems preventing this from working:

1. The SWIG typemaps for converting Python lists to and from C++
   arrays were not updated for Python 3.  So they were doing things
   like PyString_Check instead of using the PythonString from
   PythonDataObjects.
2. ProcessLauncherWindows was ignoring the environment completely.
   So any test that involved launching an inferior with any kind
   of environment variable would have failed.
3. The test itself was using process.GetSTDOUT(), which isn't
   implemented on Windows.  So this was changed to redirect
   stdout to a file, and then the test reads the file.

[amccarth]

I don't know enough about SWIG to give that a thorough review.

source/Host/windows/ProcessLauncherWindows.cpp

38

I would explicitly set

*cur_entry = '\0';

after the loop to make it clear that this is a null-terminated block of null-terminated strings.

I get that buffer.resize(bytes) is going to zero out the bytes initially, _if_ it grows, so it's not technically necessary in the current use case. But if a future caller calls it with a junk buffer that's already long enough (or longer), then you could end up with a non-zero in that last byte.

[labath]

This will make the test fail on remote platforms, as the redirected file needs to be transferred in before you can check it (see TestQuoting for an example).

It sounds like we should come up with some guidelines wrt. using stdio in tests. I would like to use it as it is much simpler than using files, but I guess supporting that for windows, where you launch the inferior in a separate console window (which i very like btw) could be difficult (?). If we get rid of it, we might even kill two birds with one stone, as we have some stdio issues on linux as well (https://llvm.org/bugs/show_bug.cgi?id=25652). My favourite replacement for that would be inferior sticking whatever output in needs in a char[] variable and debugger reading it from there. This should hopefully work in all scenarios, as it does not depend on any external concepts.

Thoughts ?

[zturner]

Yea, supporting GetStdio is really difficult on Windows. We might try to do it again someday. Putting it in a char[] variable and reading the variable seems like a decent solution, I will try that.

There's lots of tests currently that redirect stdio, are those all broken on Linux currently?

[labath]

In D16128#325804, @zturner wrote:

Yea, supporting GetStdio is really difficult on Windows. We might try to do it again someday. Putting it in a char[] variable and reading the variable seems like a decent solution, I will try that.

Thanks.

There's lots of tests currently that redirect stdio, are those all broken on Linux currently?

The redirection works on linux, it's just not 100% reliable in tests. The thing is that inferior stdio arrives asynchronously wrt. everything else, so you really cannot be sure that by the time you do GetSTDOUT(), all output will be available. It works most of the time, but it fails in like ~0.1% of cases -- not much, but enough to be annoying when scrutinizing buildbot failures.

[labath]

In D16128#325814, @labath wrote:

There's lots of tests currently that redirect stdio, are those all broken on Linux currently?

The redirection works on linux, it's just not 100% reliable in tests. The thing is that inferior stdio arrives asynchronously wrt. everything else, so you really cannot be sure that by the time you do GetSTDOUT(), all output will be available. It works most of the time, but it fails in like ~0.1% of cases -- not much, but enough to be annoying when scrutinizing buildbot failures.

Or, if you meant the other kind of redirection, in all tests that redirect stdio to a file, we have added a if(remote) { platform.GetFile(...)}, to retrieve the file from the remote side -- not really elegant and I would prefer using the char[] approach going forward.

[labath]

There has been no activity here for 6 months. Shall we close it?

packages/Python/lldbsuite/test/functionalities/process_launch/TestProcessLaunch.py

 
 from __future__ import print_function
 
+import copy
+import os
+import time
 
-
-import os, time
 import lldb
 from lldbsuite.test.lldbtest import *
 
+import six
+
 class ProcessLaunchTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)

     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
+        self.stdout_redirect_file = 'lldb-stdout-redirect.txt'
         # disable "There is a running process, kill it and restart?" prompt
         self.runCmd("settings set auto-confirm true")
-        self.addTearDownHook(lambda: self.runCmd("settings clear auto-confirm"))
+    def tearDown(self):
+        self.runCmd("settings clear auto-confirm")
+
+        try:
+            os.unlink(self.stdout_redirect_file)
+        except:
+            pass
+
+        super().tearDown()
 
     @not_remote_testsuite_ready
     def test_io (self):

 
         evil_var = 'INIT*MIDDLE}TAIL'
 
-        target = self.dbg.CreateTarget(exe)
-        process = target.LaunchSimple(None, ['EVIL=' + evil_var], self.get_process_working_directory())
-        self.assertEqual(process.GetState(), lldb.eStateExited, PROCESS_EXITED)
+        error = lldb.SBError()
 
-        out = process.GetSTDOUT(len(evil_var))
+        target = self.dbg.CreateTarget(exe)
+        launch_info = lldb.SBLaunchInfo([])
+        launch_info.SetWorkingDirectory(self.get_process_working_directory())
+        environment = dict(os.environ)
+        environment['EVIL'] = evil_var
+        environment = [x + "=" + y for (x, y) in six.iteritems(environment)]
+
+        launch_info.SetEnvironmentEntries(environment, True)
+        launch_info.AddOpenFileAction(1, self.stdout_redirect_file, False, True)
+        process = target.Launch(launch_info, error)
+
+        state = process.GetState()
+        self.assertEqual(state, lldb.eStateExited, PROCESS_EXITED)
+
+        out = None
+        with open(self.stdout_redirect_file) as output:
+            out = output.read().rstrip()
         self.assertIsNotNone(out, "Encountered an error reading the process's output")
 
-        out = out[:len(evil_var)]
         if out != evil_var:
             self.fail('The environment variable was mis-coded: %s\n' % repr(out))
         
-        newline = process.GetSTDOUT(1)
-        self.assertIsNotNone(newline, "Encountered an error reading the process's output")
-
-        newline = newline[0]
-        if newline != '\r' and newline != '\n':
-            self.fail('Garbage at end of environment variable')

packages/Python/lldbsuite/test/functionalities/process_launch/print_env.cpp

 int main (int argc, char **argv)
 {
   char *evil = getenv("EVIL");
-  puts(evil);
+  if (evil == nullptr)
+      printf("Environment variable EVIL is not set\n");
+  else
+      puts(evil);
 
   return 0;
 }

scripts/Python/python-typemaps.swig

 /* Typemap definitions, to allow SWIG to properly handle 'char**' data types. */
 
 %typemap(in) char ** {
+  using namespace lldb_private;
   /* Check if is a list  */
-  if (PyList_Check($input)) {
-    int size = PyList_Size($input);
+  if (PythonList::Check($input)) {
+    PythonList list(PyRefType::Borrowed, $input);
+    int size = list.GetSize();
     int i = 0;
-    $1 = (char **) malloc((size+1) * sizeof(char*));
+    $1 = (char**)malloc((size+1)*sizeof(char*));
     for (i = 0; i < size; i++) {
-      PyObject *o = PyList_GetItem($input,i);
-      if (PyString_Check(o))
-        $1[i] = PyString_AsString(o);
-      else {
+      PythonString py_str = list.GetItemAtIndex(i).AsType<PythonString>();
+      if (!py_str.IsAllocated()) {
         PyErr_SetString(PyExc_TypeError,"list must contain strings");
         free($1);
-        return NULL;
+        return nullptr;
       }
+
+      $1[i] = const_cast<char*>(py_str.GetString().data());
     }
     $1[i] = 0;
   } else if ($input == Py_None) {

 %typemap(typecheck) char ** {
   /* Check if is a list  */
   $1 = 1;
-  if (PyList_Check($input)) {
-    int size = PyList_Size($input);
+  using namespace lldb_private;
+  if (PythonList::Check($input)) {
+    PythonList list(PyRefType::Borrowed, $input);
+    int size = list.GetSize();
     int i = 0;
     for (i = 0; i < size; i++) {
-      PyObject *o = PyList_GetItem($input,i);
-      if (!PyString_Check(o)) { $1 = 0; }
+      PythonString s = list.GetItemAtIndex(i).AsType<PythonString>();
+      if (!s.IsAllocated()) { $1 = 0; }
     }
   }
   else

 
     $1 = (char**)malloc((size+1)*sizeof(char*));
     for (int i = 0; i < size; i++) {
-      PythonObject o = py_list.GetItemAtIndex(i);
-      if (!PythonString::Check(o.get())) {
+      auto py_str = py_list.GetItemAtIndex(i).AsType<PythonString>();
+      if (!py_str.IsAllocated()) {
         PyErr_SetString(PyExc_TypeError,"list must contain strings");
         free($1);
         return nullptr;
       }
-      auto py_str = o.AsType<PythonString>();
       $1[i] = const_cast<char*>(py_str.GetString().data());
     }
 

 }
 
 %typemap(typecheck) char const ** {
+  using namespace lldb_private;
   /* Check if is a list  */
   $1 = 1;
-  if (PyList_Check($input)) {
-    int size = PyList_Size($input);
+  if (PythonList::Check($input)) {
+    PythonList list(PyRefType::Borrowed, $input);
+    int size = list.GetSize();
     int i = 0;
     for (i = 0; i < size; i++) {
-      PyObject *o = PyList_GetItem($input,i);
-      if (!PyString_Check(o)) { $1 = 0; }
+      PythonString s = list.GetItemAtIndex(i).AsType<PythonString>();
+      if (!s.IsAllocated()) { $1 = 0; }
     }
   }
   else

   int i;
   len = 0;
   while ($1[len]) len++;
-  $result = PyList_New(len);
+  using namespace lldb_private;
+  PythonList list(len);
   for (i = 0; i < len; i++) {
-    PyList_SetItem($result, i, PyString_FromString($1[i]));
+    PythonString str($1[i]);
+    list.SetItemAtIndex(i, str);
   }
+  $result = list.release();
 }
 
 /* Typemap definitions to allow SWIG to properly handle char buffer. */

source/Host/windows/ProcessLauncherWindows.cpp

 using namespace lldb;
 using namespace lldb_private;
 
+namespace
+{
+void
+CreateEnvironmentBuffer(const Args &env, std::vector<char> &buffer)
+{
+    if (env.GetArgumentCount() == 0)
+        return;
+
+    int bytes = 0;
+    for (int i = 0; i < env.GetArgumentCount(); ++i)
+        bytes += strlen(env.GetArgumentAtIndex(i)) + sizeof(char);
+    bytes += sizeof(char);
+    buffer.resize(bytes);
+    char *cur_entry = &buffer[0];
+    for (int i = 0; i < env.GetArgumentCount(); ++i)
+    {
+        ::strcpy(cur_entry, env.GetArgumentAtIndex(i));
+        cur_entry += strlen(cur_entry) + sizeof(char);
+    }
+}
+}
+
 HostProcess
 ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info, Error &error)
 {

     if (launch_info.GetFlags().Test(eLaunchFlagDebug))
         flags |= DEBUG_ONLY_THIS_PROCESS;
 
+    auto &env = const_cast<Args &>(launch_info.GetEnvironmentEntries());
+    LPVOID env_block = nullptr;
+    ::CreateEnvironmentBuffer(env, environment);
+    if (!environment.empty())
+        env_block = environment.data();
+
     executable = launch_info.GetExecutableFile().GetPath();
     launch_info.GetArguments().GetQuotedCommandString(commandLine);
-    BOOL result = ::CreateProcessA(executable.c_str(), const_cast<char *>(commandLine.c_str()), NULL, NULL, TRUE, flags, NULL,
-                                   launch_info.GetWorkingDirectory().GetCString(), &startupinfo, &pi);
+    BOOL result = ::CreateProcessA(executable.c_str(), const_cast<char *>(commandLine.c_str()), NULL, NULL, TRUE, flags,
+                                   env_block, launch_info.GetWorkingDirectory().GetCString(), &startupinfo, &pi);
     if (result)
     {
         // Do not call CloseHandle on pi.hProcess, since we want to pass that back through the HostProcess.