Move the shell expansion code to Host/common

[zturner]

Summary

This moves the shell expansion code from different implementation files for every Host to Host/common. The reason for this is that the method for expanding the shell arguments is the same for a majority of platforms, so there is no reason to leave it unimplemented for everyone.

Additionally, this generalizes the Host method a little to be useful even if you don't have a ProcessLaunchInfo handy by changing the arguments to accept only the information that it needs, instead of the entire ProcessLaunchInfo. As a side effect, this also fixes a redundant test of the process launch flag, which was being checked once in the Platform and another time in Host::ShellExpandArguments.

[zturner]

Anyone have thoughts on this? If there's no suggestions I'd like to commit, but I'll give another day or two for comments.

[clayborg]

A few changes, see inlined comments.

include/lldb/Host/Host.h

263

Use lldb_private::Args instead of std::vector<std::string>.

include/lldb/Target/Platform.h

395

Use lldb_private::Args instead of std::vector<std::string>.

source/Host/common/Host.cpp

529

Use lldb_private::Args instead of std::vector<std::string>.

source/Target/Platform.cpp

1119

Remove this and use launch_info.GetArguments() below, or change to Args type then copy back into launch_info.GetArguments() if all goes well.

1122

Pass launch_info.GetArguments() instead of expanded_args, or a temp Args variable.

1141

Change "vector<string" over to Args.

[zturner]

Well, I agree in principle that the API is meant to support process
launching. But if its utility can be trivially extended to support other
use cases, then why not?

FWIW, when I decided to do this, it was only to enable the functionality
for other platforms (i.e. moving from specific dirs to common), because
only Windows is going to have a different codepath here so it didn't make
sense to increase the code debt for everyone just because of Windows. Then
changing of the signature was just something I noticed on the side. I can
change it back if people feel strongly, but at the same time, I really like
making code as general as possible, as long as the generality doesn't
hinder the readability or usability for the primary use case (which in this
case I don't think it does)

[clayborg]

I agree with Enrico's email comments that it would be nice to still use ProcessLaunchInfo so we can pass the environment variables down to the argdumper program so that it could us the env vars to expand command line stuff if needed.

[zturner]

Understood. I will update the patch when I have time and commit the
result. Thanks for the review!

include/lldb/API/SBLaunchInfo.h

     
     bool
     GetShellExpandArguments ();
-    
-    void
-    SetShellExpandArguments (bool glob);
-    
+    void SetShellExpandArguments(bool expand);
+
     uint32_t
     GetResumeCount ();
 

include/lldb/Host/Host.h

     //  argument magic the platform defines as part of its typical
     //  user experience
     //------------------------------------------------------------------
-    static Error
-    ShellExpandArguments (ProcessLaunchInfo &launch_info);
-    
+    static Error ShellExpandArguments(llvm::StringRef input, llvm::StringRef working_dir, std::vector<std::string> &expanded);
+
     static Error
     RunShellCommand (const char *command,           // Shouldn't be NULL
                      const char *working_dir,       // Pass NULL to use the current working directory

include/lldb/Target/Platform.h

         //  argument magic the platform defines as part of its typical
         //  user experience
         //------------------------------------------------------------------
-        virtual Error
-        ShellExpandArguments (ProcessLaunchInfo &launch_info);
-        
+        virtual Error ShellExpandArguments(llvm::StringRef original, llvm::StringRef working_dir, std::vector<std::string> &expanded);
+
         //------------------------------------------------------------------
         /// Kill process on a platform.
         //------------------------------------------------------------------

source/Host/common/Host.cpp

 #include "lldb/Core/ArchSpec.h"
 #include "lldb/Core/Error.h"
 #include "lldb/Core/Log.h"
+#include "lldb/Core/StreamString.h"
+#include "lldb/Core/StructuredData.h"
 #include "lldb/Host/FileSpec.h"
 #include "lldb/Host/HostProcess.h"
 #include "lldb/Host/MonitoringProcessLauncher.h"

 }
 
 Error
-Host::RunShellCommand (const char *command,
-                       const char *working_dir,
-                       int *status_ptr,
-                       int *signo_ptr,
-                       std::string *command_output_ptr,
-                       uint32_t timeout_sec,
-                       bool run_in_default_shell)
+Host::ShellExpandArguments(llvm::StringRef original, llvm::StringRef working_dir, std::vector<std::string> &expanded)
+{
+    Error error;
+    FileSpec expand_tool_spec;
+    if (!HostInfo::GetLLDBPath(lldb::ePathTypeSupportExecutableDir, expand_tool_spec))
+    {
+        error.SetErrorString("could not find argdumper tool");
+        return error;
+    }
+    expand_tool_spec.AppendPathComponent("argdumper");
+    if (!expand_tool_spec.Exists())
+    {
+        error.SetErrorString("could not find argdumper tool");
+        return error;
+    }
+
+    StreamString expand_command;
+    expand_command.Printf("%s %s", expand_tool_spec.GetPath().c_str(), original.data());
+
+    int status;
+    std::string output;
+
+    RunShellCommand(expand_command.GetData(), working_dir.data(), &status, nullptr, &output, 10);
+
+    if (status != 0)
+    {
+        error.SetErrorStringWithFormat("argdumper exited with error %d", status);
+        return error;
+    }
+
+    auto data_sp = StructuredData::ParseJSON(output);
+    if (!data_sp)
+    {
+        error.SetErrorString("invalid JSON");
+        return error;
+    }
+
+    auto dict_sp = data_sp->GetAsDictionary();
+    if (!data_sp)
+    {
+        error.SetErrorString("invalid JSON");
+        return error;
+    }
+
+    auto args_sp = dict_sp->GetObjectForDotSeparatedPath("arguments");
+    if (!args_sp)
+    {
+        error.SetErrorString("invalid JSON");
+        return error;
+    }
+
+    auto args_array_sp = args_sp->GetAsArray();
+    if (!args_array_sp)
+    {
+        error.SetErrorString("invalid JSON");
+        return error;
+    }
+
+    expanded.clear();
+    for (size_t i = 0; i < args_array_sp->GetSize(); i++)
+    {
+        auto item_sp = args_array_sp->GetItemAtIndex(i);
+        if (!item_sp)
+            continue;
+        auto str_sp = item_sp->GetAsString();
+        if (!str_sp)
+            continue;
+
+        expanded.push_back(str_sp->GetValue());
+    }
+
+    return error;
+}
+
+Error
+Host::RunShellCommand(const char *command, const char *working_dir, int *status_ptr, int *signo_ptr, std::string *command_output_ptr,
+                      uint32_t timeout_sec, bool run_in_default_shell)
 {
     Error error;
     ProcessLaunchInfo launch_info;

source/Host/freebsd/Host.cpp

     static const lldb_private::UnixSignalsSP s_unix_signals_sp (new FreeBSDSignals ());
     return s_unix_signals_sp;
 }
-
-Error
-Host::ShellExpandArguments (ProcessLaunchInfo &launch_info)
-{
-    return Error("unimplemented");
-}
-

source/Host/linux/Host.cpp

     static const lldb_private::UnixSignalsSP s_unix_signals_sp (new process_linux::LinuxSignals ());
     return s_unix_signals_sp;
 }
-
-Error
-Host::ShellExpandArguments (ProcessLaunchInfo &launch_info)
-{
-    return Error("unimplemented");
-}

source/Host/macosx/Host.mm

 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Core/StreamString.h"
-#include "lldb/Core/StructuredData.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/Endian.h"
 #include "lldb/Host/FileSpec.h"

     return error;
 }
 
-Error
-Host::ShellExpandArguments (ProcessLaunchInfo &launch_info)
-{
-    Error error;
-    if (launch_info.GetFlags().Test(eLaunchFlagShellExpandArguments))
-    {
-        FileSpec expand_tool_spec;
-        if (!HostInfo::GetLLDBPath(lldb::ePathTypeSupportExecutableDir, expand_tool_spec))
-        {
-            error.SetErrorString("could not find argdumper tool");
-            return error;
-        }
-        expand_tool_spec.AppendPathComponent("argdumper");
-        if (!expand_tool_spec.Exists())
-        {
-            error.SetErrorString("could not find argdumper tool");
-            return error;
-        }
-        
-        std::string quoted_cmd_string;
-        launch_info.GetArguments().GetQuotedCommandString(quoted_cmd_string);
-        StreamString expand_command;
-        
-        expand_command.Printf("%s %s",
-                              expand_tool_spec.GetPath().c_str(),
-                              quoted_cmd_string.c_str());
-        
-        int status;
-        std::string output;
-        RunShellCommand(expand_command.GetData(), launch_info.GetWorkingDirectory(), &status, nullptr, &output, 10);
-        
-        if (status != 0)
-        {
-            error.SetErrorStringWithFormat("argdumper exited with error %d", status);
-            return error;
-        }
-        
-        auto data_sp = StructuredData::ParseJSON(output);
-        if (!data_sp)
-        {
-            error.SetErrorString("invalid JSON");
-            return error;
-        }
-        
-        auto dict_sp = data_sp->GetAsDictionary();
-        if (!data_sp)
-        {
-            error.SetErrorString("invalid JSON");
-            return error;
-        }
-        
-        auto args_sp = dict_sp->GetObjectForDotSeparatedPath("arguments");
-        if (!args_sp)
-        {
-            error.SetErrorString("invalid JSON");
-            return error;
-        }
-
-        auto args_array_sp = args_sp->GetAsArray();
-        if (!args_array_sp)
-        {
-            error.SetErrorString("invalid JSON");
-            return error;
-        }
-        
-        launch_info.GetArguments().Clear();
-        
-        for (size_t i = 0;
-             i < args_array_sp->GetSize();
-             i++)
-        {
-            auto item_sp = args_array_sp->GetItemAtIndex(i);
-            if (!item_sp)
-                continue;
-            auto str_sp = item_sp->GetAsString();
-            if (!str_sp)
-                continue;
-            
-            launch_info.GetArguments().AppendArgument(str_sp->GetValue().c_str());
-        }
-    }
-    
-    return error;
-}
-
 HostThread
 Host::StartMonitoringChildProcess(Host::MonitorChildProcessCallback callback, void *callback_baton, lldb::pid_t pid, bool monitor_signals)
 {

source/Host/windows/Host.cpp

 {
     return HostThread();
 }
-
-Error
-Host::ShellExpandArguments (ProcessLaunchInfo &launch_info)
-{
-    Error error;
-    if (launch_info.GetFlags().Test(eLaunchFlagShellExpandArguments))
-    {
-        FileSpec expand_tool_spec;
-        if (!HostInfo::GetLLDBPath(lldb::ePathTypeSupportExecutableDir, expand_tool_spec))
-        {
-            error.SetErrorString("could not find argdumper tool");
-            return error;
-        }
-        expand_tool_spec.AppendPathComponent("argdumper.exe");
-        if (!expand_tool_spec.Exists())
-        {
-            error.SetErrorString("could not find argdumper tool");
-            return error;
-        }
-        
-        std::string quoted_cmd_string;
-        launch_info.GetArguments().GetQuotedCommandString(quoted_cmd_string);
-        std::replace(quoted_cmd_string.begin(), quoted_cmd_string.end(), '\\', '/');
-        StreamString expand_command;
-        
-        expand_command.Printf("%s %s",
-                              expand_tool_spec.GetPath().c_str(),
-                              quoted_cmd_string.c_str());
-        
-        int status;
-        std::string output;
-        RunShellCommand(expand_command.GetData(), launch_info.GetWorkingDirectory(), &status, nullptr, &output, 10);
-        
-        if (status != 0)
-        {
-            error.SetErrorStringWithFormat("argdumper exited with error %d", status);
-            return error;
-        }
-        
-        auto data_sp = StructuredData::ParseJSON(output);
-        if (!data_sp)
-        {
-            error.SetErrorString("invalid JSON");
-            return error;
-        }
-        
-        auto dict_sp = data_sp->GetAsDictionary();
-        if (!data_sp)
-        {
-            error.SetErrorString("invalid JSON");
-            return error;
-        }
-        
-        auto args_sp = dict_sp->GetObjectForDotSeparatedPath("arguments");
-        if (!args_sp)
-        {
-            error.SetErrorString("invalid JSON");
-            return error;
-        }
-        
-        auto args_array_sp = args_sp->GetAsArray();
-        if (!args_array_sp)
-        {
-            error.SetErrorString("invalid JSON");
-            return error;
-        }
-        
-        launch_info.GetArguments().Clear();
-        
-        for (size_t i = 0;
-             i < args_array_sp->GetSize();
-             i++)
-        {
-            auto item_sp = args_array_sp->GetItemAtIndex(i);
-            if (!item_sp)
-                continue;
-            auto str_sp = item_sp->GetAsString();
-            if (!str_sp)
-                continue;
-            
-            launch_info.GetArguments().AppendArgument(str_sp->GetValue().c_str());
-        }
-    }
-    
-    return error;
-}

source/Target/Platform.cpp

         }
         else if (launch_info.GetFlags().Test(eLaunchFlagShellExpandArguments))
         {
-            error = ShellExpandArguments(launch_info);
+            std::vector<std::string> expanded_args;
+            std::string original_args;
+            launch_info.GetArguments().GetQuotedCommandString(original_args);
+            error = ShellExpandArguments(original_args.c_str(), launch_info.GetWorkingDirectory(), expanded_args);
             if (error.Fail())
                 return error;
+            launch_info.GetArguments().Clear();
+            for (auto arg : expanded_args)
+                launch_info.GetArguments().AppendArgument(arg.c_str());
         }
 
         if (log)

 }
 
 Error
-Platform::ShellExpandArguments (ProcessLaunchInfo &launch_info)
+Platform::ShellExpandArguments(llvm::StringRef original, llvm::StringRef working_dir, std::vector<std::string> &expanded)
 {
     if (IsHost())
-        return Host::ShellExpandArguments(launch_info);
+        return Host::ShellExpandArguments(original, working_dir, expanded);
     return Error("base lldb_private::Platform class can't expand arguments");
 }