Delete LLDB_DEFAULT_SHELL, and determine the default shell at runtime

[zturner]

Summary

This patch removes the #define for LLDB_DEFAULT_SHELL, instead relying on a runtime call to determine the default shell.

Since we return the default shell as a FileSpec instead of as a const char*, this patch opened the door for a further improvement of propagating this FileSpec usage to other places that had previously stored the shell as a char*, so we do that as well.

[clayborg]

You can't add members to any lldb::SB class, you will need to remove m_cached_shell and either put it into lldb_private::ProcessLaunchInfo. Why? Because we expect the lldb::SB API to be stable for people to link against and adding a new ivar changes the layout of the class. All lldb::SB classes must have a single member that is either a pointer, unique_ptr or shared_ptr and these can't change. Other rules for lldb::SB API classes include: no virtual functions and no inheritance.

[zturner]

In D5805#4, @clayborg wrote:

You can't add members to any lldb::SB class, you will need to remove m_cached_shell and either put it into lldb_private::ProcessLaunchInfo. Why? Because we expect the lldb::SB API to be stable for people to link against and adding a new ivar changes the layout of the class. All lldb::SB classes must have a single member that is either a pointer, unique_ptr or shared_ptr and these can't change. Other rules for lldb::SB API classes include: no virtual functions and no inheritance.

Can I make it a function local static? The issue is that the function returns a raw char*, so after my change it can't just return the result of m_opaque_sp->GetShell(). And I need to make sure its life extends beyond the scope of the function.

[zturner]

Moves the member variable of SBTarget to a function local static, to avoid changing the ABI of the class.

[zturner]

The differential between patch 1 and patch 2 doesn't show up correctly since I used a different value for git diff -U, and I forgot the original value. The change in this patchset is only deleting the member variable from SBTarget and moving it to a function local static though. The full diff should still show correctly.

[clayborg]

In SBLaunchInfo::GetShell() just make a local ConstString from the shell and return GetCString(). Once a string has been const-ified, it will remain in the string pool forever. This will keep this function thread safe.

[zturner]

Make a ConstString out of the path before returning it.

[clayborg]

Looks good.

[zturner]

Closed by commit rL220217 (authored by @zturner).

lldb/trunk/include/lldb/Host/Host.h

#endif
 
     static Error
     RunShellCommand (const char *command,           // Shouldn't be NULL
                      const char *working_dir,       // Pass NULL to use the current working directory
                      int *status_ptr,               // Pass NULL if you don't want the process exit status
                      int *signo_ptr,                // Pass NULL if you don't want the signal that caused the process to exit
                      std::string *command_output,   // Pass NULL if you don't want the command output
                      uint32_t timeout_sec,
-                     const char *shell = LLDB_DEFAULT_SHELL);
+                     bool run_in_default_shell = true);
     
     static lldb::DataBufferSP
     GetAuxvData (lldb_private::Process *process);
 
     static lldb::DataBufferSP
     GetAuxvData (lldb::pid_t pid);
 
     static lldb::TargetSP

lldb/trunk/include/lldb/Host/posix/HostInfoPosix.h

 //===-- HostInfoPosix.h -----------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef lldb_Host_posix_HostInfoPosix_h_
 #define lldb_Host_posix_HostInfoPosix_h_
 
+#include "lldb/Host/FileSpec.h"
 #include "lldb/Host/HostInfoBase.h"
 
 namespace lldb_private
 {
 
 class HostInfoPosix : public HostInfoBase
 {
     friend class HostInfoBase;
 
   public:
     static size_t GetPageSize();
     static bool GetHostname(std::string &s);
     static const char *LookupUserName(uint32_t uid, std::string &user_name);
     static const char *LookupGroupName(uint32_t gid, std::string &group_name);
 
     static uint32_t GetUserID();
     static uint32_t GetGroupID();
     static uint32_t GetEffectiveUserID();
     static uint32_t GetEffectiveGroupID();
 
+    static FileSpec GetDefaultShell();
+
   protected:
     static bool ComputeSupportExeDirectory(FileSpec &file_spec);
     static bool ComputeHeaderDirectory(FileSpec &file_spec);
     static bool ComputePythonDirectory(FileSpec &file_spec);
 };
 }
 
 #endif

lldb/trunk/include/lldb/Host/windows/HostInfoWindows.h

class HostInfoWindows : public HostInfoBase
   public:
     static size_t GetPageSize();
 
     static bool GetOSVersion(uint32_t &major, uint32_t &minor, uint32_t &update);
     static bool GetOSBuildString(std::string &s);
     static bool GetOSKernelDescription(std::string &s);
     static bool GetHostname(std::string &s);
     static FileSpec GetProgramFileSpec();
+    static FileSpec GetDefaultShell();
 
   protected:
     static bool ComputePythonDirectory(FileSpec &file_spec);
 
   private:
     static FileSpec m_program_filespec;
 };
 }
 
 #endif

lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h

 #ifndef liblldb_ProcessLaunch_Info_h
 #define liblldb_ProcessLaunch_Info_h
 
 // C++ Headers
 #include <string>
 
 // LLDB Headers
 #include "lldb/Core/Flags.h"
+#include "lldb/Host/FileSpec.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Target/FileAction.h"
 #include "lldb/Target/ProcessInfo.h"
 #include "lldb/Utility/PseudoTerminal.h"
 
 namespace lldb_private
 {
 
public:
         }
 
         const char *
         GetProcessPluginName () const;
 
         void
         SetProcessPluginName (const char *plugin);
 
-        const char *
+        const FileSpec &
         GetShell () const;
 
         void
-        SetShell (const char * path);
+        SetShell (const FileSpec &shell);
 
         uint32_t
         GetResumeCount () const
         {
             return m_resume_count;
         }
 
         void
public:
         GetDetachOnError () const
         {
             return m_flags.Test(lldb::eLaunchFlagDetachOnError);
         }
 
     protected:
         std::string m_working_dir;
         std::string m_plugin_name;
-        std::string m_shell;
+        FileSpec m_shell;
         Flags m_flags;       // Bitwise OR of bits from lldb::LaunchFlags
         std::vector<FileAction> m_file_actions; // File actions for any other files
         std::shared_ptr<lldb_utility::PseudoTerminal> m_pty;
         uint32_t m_resume_count; // How many times do we resume after launching
         Host::MonitorChildProcessCallback m_monitor_callback;
         void *m_monitor_callback_baton;
         bool m_monitor_signals;
         std::string m_event_data; // A string passed to the plugin launch, having no meaning to the upper levels of lldb.
         lldb::ListenerSP m_hijack_listener_sp;
     };
 }
 
 #endif // liblldb_ProcessLaunch_Info_h

lldb/trunk/include/lldb/lldb-defines.h

 //#define LLDB_VERSION
 //#define LLDB_REVISION
 //#define LLDB_VERSION_STRING
 
 //----------------------------------------------------------------------
 // LLDB defines
 //----------------------------------------------------------------------
 #define LLDB_GENERIC_ERROR              UINT32_MAX
-#if defined(_WIN32)
-#define LLDB_DEFAULT_SHELL              "cmd.exe"
-#else
-#define LLDB_DEFAULT_SHELL              "/bin/sh"
-#endif
 
 //----------------------------------------------------------------------
 // Breakpoints
 //----------------------------------------------------------------------
 #define LLDB_INVALID_BREAK_ID           0
 #define LLDB_DEFAULT_BREAK_SIZE         0
 #define LLDB_BREAK_ID_IS_VALID(bid)     ((bid) != (LLDB_INVALID_BREAK_ID))
 #define LLDB_BREAK_ID_IS_INTERNAL(bid)  ((bid) < 0)

lldb/trunk/source/API/SBTarget.cpp

 SBLaunchInfo::SetProcessPluginName (const char *plugin_name)
 {
     return m_opaque_sp->SetProcessPluginName (plugin_name);
 }
 
 const char *
 SBLaunchInfo::GetShell ()
 {
-    return m_opaque_sp->GetShell();
+    // Constify this string so that it is saved in the string pool.  Otherwise
+    // it would be freed when this function goes out of scope.
+    ConstString shell(m_opaque_sp->GetShell().GetPath().c_str());
+    return shell.AsCString();
 }
 
 void
 SBLaunchInfo::SetShell (const char * path)
 {
-    m_opaque_sp->SetShell (path);
+    m_opaque_sp->SetShell (FileSpec(path, false));
 }
 
 uint32_t
 SBLaunchInfo::GetResumeCount ()
 {
     return m_opaque_sp->GetResumeCount();
 }
 

lldb/trunk/source/Host/common/Host.cpp

 
 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,
-                       const char *shell)
+                       bool run_in_default_shell)
 {
     Error error;
     ProcessLaunchInfo launch_info;
-    if (shell && shell[0])
+    if (run_in_default_shell)
     {
         // Run the command in a shell
-        launch_info.SetShell(shell);
+        launch_info.SetShell(HostInfo::GetDefaultShell());
         launch_info.GetArguments().AppendArgument(command);
         const bool localhost = true;
         const bool will_debug = false;
         const bool first_arg_is_full_shell_command = true;
         launch_info.ConvertArgumentsForLaunchingInShell (error,
                                                          localhost,
                                                          will_debug,
                                                          first_arg_is_full_shell_command,

lldb/trunk/source/Host/posix/HostInfoPosix.cpp

 }
 
 uint32_t
 HostInfoPosix::GetEffectiveGroupID()
 {
     return getegid();
 }
 
+FileSpec
+HostInfoPosix::GetDefaultShell()
+{
+    return FileSpec("/bin/sh", false);
+}
+
 bool
 HostInfoPosix::ComputeSupportExeDirectory(FileSpec &file_spec)
 {
     Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST);
 
     FileSpec lldb_file_spec;
     if (!GetLLDBPath(lldb::ePathTypeLLDBShlibDir, lldb_file_spec))
         return false;

lldb/trunk/source/Host/windows/HostInfoWindows.cpp

if (!is_initialized)
 
         std::vector<char> buffer(PATH_MAX);
         ::GetModuleFileName(NULL, &buffer[0], buffer.size());
         m_program_filespec.SetFile(&buffer[0], false);
     }
     return m_program_filespec;
 }
 
+FileSpec
+HostInfoWindows::GetDefaultShell()
+{
+    return FileSpec(::getenv("ComSpec"), false);
+}
+
 bool
 HostInfoWindows::ComputePythonDirectory(FileSpec &file_spec)
 {
     FileSpec lldb_file_spec;
     if (!GetLLDBPath(lldb::ePathTypeLLDBShlibDir, lldb_file_spec))
         return false;
 
     char raw_path[PATH_MAX];
     lldb_file_spec.AppendPathComponent("../lib/site-packages");
     lldb_file_spec.GetPath(raw_path, sizeof(raw_path));
 
     file_spec.GetDirectory().SetCString(raw_path);
     return true;
 }

lldb/trunk/source/Interpreter/CommandInterpreter.cpp

CommandInterpreter::Initialize ()
     
     cmd_obj_sp = GetCommandSPExact ("process launch", false);
     if (cmd_obj_sp)
     {
         alias_arguments_vector_sp.reset (new OptionArgVector);
 #if defined (__arm__) || defined (__arm64__) || defined (__aarch64__)
         ProcessAliasOptionsArgs (cmd_obj_sp, "--", alias_arguments_vector_sp);
 #else
-        ProcessAliasOptionsArgs (cmd_obj_sp, "--shell=" LLDB_DEFAULT_SHELL " --", alias_arguments_vector_sp);
+        std::string shell_option;
+        shell_option.append("--shell=");
+        shell_option.append(HostInfo::GetDefaultShell().GetPath());
+        shell_option.append(" --");
+        ProcessAliasOptionsArgs (cmd_obj_sp, shell_option.c_str(), alias_arguments_vector_sp);
 #endif
         AddAlias ("r", cmd_obj_sp);
         AddAlias ("run", cmd_obj_sp);
         AddOrReplaceAliasOptions ("r", alias_arguments_vector_sp);
         AddOrReplaceAliasOptions ("run", alias_arguments_vector_sp);
     }
     
     cmd_obj_sp = GetCommandSPExact ("target symbols add", false);

lldb/trunk/source/Plugins/Platform/Linux/PlatformLinux.cpp

PlatformLinux::GetResumeCountForLaunchInfo (ProcessLaunchInfo &launch_info)
     // Always resume past the initial stop when we use eLaunchFlagDebug
     if (launch_info.GetFlags ().Test (eLaunchFlagDebug))
     {
         // Resume past the stop for the final exec into the true inferior.
         ++resume_count;
     }
 
     // If we're not launching a shell, we're done.
-    const char *shell = launch_info.GetShell();
-    if (shell == NULL)
+    const FileSpec &shell = launch_info.GetShell();
+    if (!shell)
         return resume_count;
 
+    std::string shell_string = shell.GetPath();
     // We're in a shell, so for sure we have to resume past the shell exec.
     ++resume_count;
 
     // Figure out what shell we're planning on using.
-    const char *shell_name = strrchr (shell, '/');
+    const char *shell_name = strrchr (shell_string.c_str(), '/');
     if (shell_name == NULL)
-        shell_name = shell;
+        shell_name = shell_string.c_str();
     else
         shell_name++;
 
     if (strcmp (shell_name, "csh") == 0
              || strcmp (shell_name, "tcsh") == 0
              || strcmp (shell_name, "zsh") == 0
              || strcmp (shell_name, "sh") == 0)
     {

lldb/trunk/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp

PlatformDarwin::SetThreadCreationBreakpoint (Target &target)
 
     return bp_sp;
 }
 
 
 int32_t
 PlatformDarwin::GetResumeCountForLaunchInfo (ProcessLaunchInfo &launch_info)
 {
-    const char *shell = launch_info.GetShell();
-    if (shell == NULL)
+    const FileSpec &shell = launch_info.GetShell();
+    if (!shell)
         return 1;
         
-    const char *shell_name = strrchr (shell, '/');
+    std::string shell_string = shell.GetPath();
+    const char *shell_name = strrchr (shell_string.c_str(), '/');
     if (shell_name == NULL)
-        shell_name = shell;
+        shell_name = shell_string.c_str();
     else
         shell_name++;
     
     if (strcmp (shell_name, "sh") == 0)
     {
         // /bin/sh re-exec's itself as /bin/bash requiring another resume.
         // But it only does this if the COMMAND_MODE environment variable
         // is set to "legacy".

lldb/trunk/source/Target/Platform.cpp

if (IsHost())
         
         if (launch_info.GetFlags().Test (eLaunchFlagLaunchInShell))
         {
             const bool is_localhost = true;
             const bool will_debug = launch_info.GetFlags().Test(eLaunchFlagDebug);
             const bool first_arg_is_full_shell_command = false;
             uint32_t num_resumes = GetResumeCountForLaunchInfo (launch_info);
             if (log)
+            {
+                const FileSpec &shell = launch_info.GetShell();
+                const char *shell_str = (shell) ? shell.GetPath().c_str() : "<null>";
                 log->Printf ("Platform::%s GetResumeCountForLaunchInfo() returned %" PRIu32 ", shell is '%s'",
                              __FUNCTION__,
                              num_resumes,
-                             launch_info.GetShell () ? launch_info.GetShell () : "<null>");
+                             shell_str);
+            }
 
             if (!launch_info.ConvertArgumentsForLaunchingInShell (error,
                                                                   is_localhost,
                                                                   will_debug,
                                                                   first_arg_is_full_shell_command,
                                                                   num_resumes))
                 return error;
         }

lldb/trunk/source/Target/Process.cpp

switch (short_option)
                 disable_aslr = disable_aslr_arg ? eLazyBoolYes : eLazyBoolNo;
             else
                 error.SetErrorStringWithFormat ("Invalid boolean value for disable-aslr option: '%s'", option_arg ? option_arg : "<null>");
             break;
         }
 
         case 'c':
             if (option_arg && option_arg[0])
-                launch_info.SetShell (option_arg);
+                launch_info.SetShell (FileSpec(option_arg, false));
             else
-                launch_info.SetShell (LLDB_DEFAULT_SHELL);
+                launch_info.SetShell (HostInfo::GetDefaultShell());
             break;
             
         case 'v':
             launch_info.GetEnvironmentEntries().AppendArgument(option_arg);
             break;
 
         default:
             error.SetErrorStringWithFormat("unrecognized short option character '%c'", short_option);

lldb/trunk/source/Target/ProcessLaunchInfo.cpp

 //----------------------------------------------------------------------------
 // ProcessLaunchInfo member functions
 //----------------------------------------------------------------------------
 
 ProcessLaunchInfo::ProcessLaunchInfo () :
     ProcessInfo(),
     m_working_dir (),
     m_plugin_name (),
-    m_shell (),
     m_flags (0),
     m_file_actions (),
     m_pty (new lldb_utility::PseudoTerminal),
     m_resume_count (0),
     m_monitor_callback (NULL),
     m_monitor_callback_baton (NULL),
     m_monitor_signals (false),
     m_hijack_listener_sp ()
 {
 }
 
 ProcessLaunchInfo::ProcessLaunchInfo(const char *stdin_path, const char *stdout_path, const char *stderr_path,
                                      const char *working_directory, uint32_t launch_flags) :
     ProcessInfo(),
     m_working_dir(),
     m_plugin_name(),
-    m_shell(),
     m_flags(launch_flags),
     m_file_actions(),
     m_pty(new lldb_utility::PseudoTerminal),
     m_resume_count(0),
     m_monitor_callback(NULL),
     m_monitor_callback_baton(NULL),
     m_monitor_signals(false),
     m_hijack_listener_sp()

 ProcessLaunchInfo::SetProcessPluginName (const char *plugin)
 {
     if (plugin && plugin[0])
         m_plugin_name.assign (plugin);
     else
         m_plugin_name.clear();
 }
 
-const char *
+const FileSpec &
 ProcessLaunchInfo::GetShell () const
 {
-    if (m_shell.empty())
-        return NULL;
-    return m_shell.c_str();
+    return m_shell;
 }
 
 void
-ProcessLaunchInfo::SetShell (const char * path)
+ProcessLaunchInfo::SetShell (const FileSpec &shell)
 {
-    if (path && path[0])
+    m_shell = shell;
+    if (m_shell)
     {
-        m_shell.assign (path);
+        m_shell.ResolveExecutableLocation();
         m_flags.Set (lldb::eLaunchFlagLaunchInShell);
     }
     else
-    {
-        m_shell.clear();
         m_flags.Clear (lldb::eLaunchFlagLaunchInShell);
-    }
-}
 
 void
 ProcessLaunchInfo::SetLaunchInSeparateProcessGroup (bool separate)
 {
     if (separate)
         m_flags.Set(lldb::eLaunchFlagLaunchInSeparateProcessGroup);
     else
         m_flags.Clear (lldb::eLaunchFlagLaunchInSeparateProcessGroup);
 
 }
 
 void
 ProcessLaunchInfo::Clear ()
 {
     ProcessInfo::Clear();
     m_working_dir.clear();
     m_plugin_name.clear();
-    m_shell.clear();
+    m_shell.Clear();
     m_flags.Clear();
     m_file_actions.clear();
     m_resume_count = 0;
     m_hijack_listener_sp.reset();
 }
 
 void
 ProcessLaunchInfo::SetMonitorProcessCallback (Host::MonitorChildProcessCallback callback,
ProcessLaunchInfo::ConvertArgumentsForLaunchingInShell (Error &error,
                                                         bool will_debug,
                                                         bool first_arg_is_full_shell_command,
                                                         int32_t num_resumes)
 {
     error.Clear();
 
     if (GetFlags().Test (eLaunchFlagLaunchInShell))
     {
-        const char *shell_executable = GetShell();
-        if (shell_executable)
+        if (m_shell)
         {
             char shell_resolved_path[PATH_MAX];
-
-            if (localhost)
-            {
-                FileSpec shell_filespec (shell_executable, true);
-
-                if (!shell_filespec.Exists())
-                {
-                    // Resolve the path in case we just got "bash", "sh" or "tcsh"
-                    if (!shell_filespec.ResolveExecutableLocation ())
-                    {
-                        error.SetErrorStringWithFormat("invalid shell path '%s'", shell_executable);
-                        return false;
-                    }
-                }
-                shell_filespec.GetPath (shell_resolved_path, sizeof(shell_resolved_path));
-                shell_executable = shell_resolved_path;
-            }
+            std::string shell_executable = m_shell.GetPath();
 
             const char **argv = GetArguments().GetConstArgumentVector ();
             if (argv == NULL || argv[0] == NULL)
                 return false;
             Args shell_arguments;
             std::string safe_arg;
-            shell_arguments.AppendArgument (shell_executable);
+            shell_arguments.AppendArgument (shell_executable.c_str());
             shell_arguments.AppendArgument ("-c");
             StreamString shell_command;
             if (will_debug)
             {
                 // Add a modified PATH environment variable in case argv[0]
                 // is a relative path
                 const char *argv0 = argv[0];
                 if (argv0 && (argv0[0] != '/' && argv0[0] != '~'))
if (GetFlags().Test (eLaunchFlagLaunchInShell))
             {
                 for (size_t i=0; argv[i] != NULL; ++i)
                 {
                     const char *arg = Args::GetShellSafeArgument (argv[i], safe_arg);
                     shell_command.Printf(" %s", arg);
                 }
             }
             shell_arguments.AppendArgument (shell_command.GetString().c_str());
-            m_executable.SetFile(shell_executable, false);
+            m_executable = m_shell;
             m_arguments = shell_arguments;
             return true;
         }
         else
         {
             error.SetErrorString ("invalid shell path");
         }
     }
     else
     {
         error.SetErrorString ("not launching in shell");
     }
     return false;
 }

lldb/trunk/source/Target/Target.cpp

 #include "lldb/Core/SourceManager.h"
 #include "lldb/Core/State.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Core/StreamString.h"
 #include "lldb/Core/Timer.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Expression/ClangASTSource.h"
 #include "lldb/Expression/ClangUserExpression.h"
+#include "lldb/Host/FileSpec.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Interpreter/OptionGroupWatchpoint.h"
 #include "lldb/Interpreter/OptionValues.h"
 #include "lldb/Interpreter/Property.h"
 #include "lldb/lldb-private-log.h"
 #include "lldb/Symbol/ObjectFile.h"
if (error.Success())
                 {
                     Error error2;
                     error2.SetErrorStringWithFormat("process resume at entry point failed: %s", error.AsCString());
                     error = error2;
                 }
             }
             else if (state == eStateExited)
             {
-                bool with_shell = launch_info.GetShell();
+                bool with_shell = !!launch_info.GetShell();
                 const int exit_status = m_process_sp->GetExitStatus();
                 const char *exit_desc = m_process_sp->GetExitDescription();
 #define LAUNCH_SHELL_MESSAGE "\n'r' and 'run' are aliases that default to launching through a shell.\nTry launching without going through a shell by using 'process launch'."
                 if (exit_desc && exit_desc[0])
                 {
                     if (with_shell)
                         error.SetErrorStringWithFormat ("process exited with status %i (%s)" LAUNCH_SHELL_MESSAGE, exit_status, exit_desc);
                     else