Fix some posixy assumptions that were used during RunShellCommand

[zturner]

Remove ClangExpressionParser changes from this patch, that was for an unrelated fix.

[clayborg]

The only thing I don't know about is the:

launch_info.SetArchitecture(HostInfo::GetArchitecture());

Line... You might specify a tool on MacOSX that is i386 like "/path/to/i386/a.out" and if you set this to the system architecture x86_64-apple-macosx, it won't execute the shell tool. This should probably be removed. Did you need to do this for windows or did you just add be because you thought it would always be correct?

[zturner]

If you take a look at ConvertArgumentsForLaunchingInShell, I had to make a
decision about whether to use -c or /C as an argument to the shell. I
could have string checked the shell name against "cmd.exe", but that seemed
more hackish than asking "Is this going to run in a shell on windows"?

[zturner]

Also, there's some code in ConvertArgumentsForLaunchingInShell that checks
the architecture. If you get into that function through
Host::RunShellCommand, the architecture would have been uninitialized, so
it looked like in that case the code that was checking for it would have
been buggy.

[clayborg]

The architecture check is in case you have a universal file that contains both x86_64 and i386, you might set the architecture to specify you want to run (debug more like it) the i386 slice even though you are on a 64 bit system (MacOSX supports this). For running a shell command, just let the system do what it needs to do and launch it using the best slice it can and don't set the architecture in the launch info.

So please remove "launch_info.SetArchitecture(HostInfo::GetArchitecture());" from Host::RunShellCommand().

[zturner]

In ConvertArgumentsForLaunchingInShell(), do you suggest I change the
conditional to use /C if the shell name is "cmd.exe"? Seems a little
hackish, so if there's a better way then that would be preferable.

What about setting the Architecture as I'm currently doing, but modify the
triple so it only contains OS and Environment, but not architecture?

[clayborg]

Seems like we should either get this from a call like the one that gets "cmd.exe" (HostInfoWindows::GetDefaultShell()). In Host::RunShellCommand() we already do:

launch_info.SetShell(HostInfo::GetDefaultShell());

We should be asking the HostInfo class to be setting the shell arguments for us. Then we HostInfoPosix and HostInfoWindows could "do the right thing".

So I vote to remove ProcessLaunchInfo::ConvertArgumentsForLaunchingInShell(), then create a function in the HostInfo class that can do this correctly for each system. This function is only used in Platform::LaunchProcess() when "IsHost()" is true and in Host::RunShellCommand().

[zturner]

Actually wait. Currently the architecture is completely uninitialized, so
anything that would break as a result of setting it in
Host::RunShellCommand would already break, because it would be making an
assumption that it could use an uninitialized ArchSpec. So this actually
seems a little better than before.

A more robust fix might involve breaking this function up into smaller
pieces.

[zturner]

In D6553#13, @clayborg wrote:

Seems like we should either get this from a call like the one that gets "cmd.exe" (HostInfoWindows::GetDefaultShell()). In Host::RunShellCommand() we already do:

launch_info.SetShell(HostInfo::GetDefaultShell());

We should be asking the HostInfo class to be setting the shell arguments for us. Then we HostInfoPosix and HostInfoWindows could "do the right thing".

So I vote to remove ProcessLaunchInfo::ConvertArgumentsForLaunchingInShell(), then create a function in the HostInfo class that can do this correctly for each system. This function is only used in Platform::LaunchProcess() when "IsHost()" is true and in Host::RunShellCommand().

Looks like we crossed paths, but we basically said the same thing, that the function should be reworked. Can I commit the fix as-is (since it doesn't break anything, due to the existing Architecture being uninitialized), and then follow up with the re-work into HostInfo?

The reason for comitting as-is first is because this patch contains some other stuff, like the changes in FileSpec, and the temp file paths, that would be unrelated to a refactor of this function.

[clayborg]

Fine to commit as long as you don't set the architecture in the launch_info in Host::RunShellCommand() since that can break MacOSX.

[zturner]

In D6553#16, @clayborg wrote:

Fine to commit as long as you don't set the architecture in the launch_info in Host::RunShellCommand() since that can break MacOSX.

Ok, I will take out the checks I added against the triple in ConvertArgumentsForLaunchingInShell for now and commit, then re-add them once the logic is properly separated in HostInfo.

Just so I can understand the code better, can you point me to the code that would break if the Architecture were set? I see some in ConvertArgumentsForLaunchingInShell, but it's only exercised if will_debug == true, and RunShellCommand() uses will_debug == false. So I'm assuming its something deep inside of Host::LaunchProcess.

[zturner]

Closed by commit rL223548 (authored by @zturner).

[clayborg]

You are correct, since it is protected by "will_debug" it won't break MacOSX. Sorry about the noise.

Greg

[clayborg]

This breaks the entire test suite (Jim Ingham informed me), "run" which is aliased to "process launch --shell" no longer works. Please revert this.

[jingham]

Yes, as checked in it didn't even build (fixed in r223559.) But then "run" fails with:

/bin/sh: exec /usr/bin/arch -arch x86_64 /Volumes/ThePlayground/Users/jingham/Projects/Sketch/build/Debug/Sketch.app/Contents/MacOS/Sketch: No such file or directory
error: process exited with status 127

But "process launch" with the same binary works correctly.

Jim

[zturner]

Can you revert for me? sorry about the break. I will have to fix it Monday.

[zturner]

If its not urgent i can revert in a couple hours, but I don't have access
to a machine at the moment.

[jingham]

I did it (r223568.)

Jim

[zturner]

Thanks, before resubmitting I'll run the test suite on mac and linux.

[zturner]

I resubmitted this in r223695. My original patch was actually ok, the
issue happened when I removed the code for setting the Architecture in the
ProcessLaunchInfo, I inadvertently removed the code that added -c to the
command line of /bin/sh. Since toward the end of the thread we determined
that setting the architecture was actually ok (since it's checked inside of
a will_debug block, I added this code back in.

I built and ran on both Linux and Mac, and did not see any failures. On
the off chance it does fail again though, let me know.

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

if (extension)
         return g_source_file_regex.Execute (extension.GetCString());
     }
     return false;
 }
 
 bool
 FileSpec::IsRelativeToCurrentWorkingDirectory () const
 {
-    const char *directory = m_directory.GetCString();
-    if (directory && directory[0])
+    const char *dir = m_directory.GetCString();
+    llvm::StringRef directory(dir ? dir : "");
+
+    if (directory.size() > 0)
+    {
+        if (m_syntax == ePathSyntaxWindows)
+        {
+            if (directory.size() >= 2 && directory[1] == ':')
+                return false;
+            if (directory[0] == '/')
+                return false;
+            return true;
+        }
+        else
         {
             // If the path doesn't start with '/' or '~', return true
             switch (directory[0])
             {
                 case '/':
                 case '~':
                     return false;
                 default:
                     return true;
             }
         }
+    }
     else if (m_filename)
     {
         // No directory, just a basename, return true
         return true;
     }
     return false;
 }

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

 
 #if defined (__FreeBSD__)
 #include <pthread_np.h>
 #endif
 
 // C++ includes
 #include <limits>
 
+#include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Core/ArchSpec.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Error.h"
 #include "lldb/Core/Log.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Host/FileSpec.h"
else
         // No shell, just run it
         Args args (command);
         const bool first_arg_is_executable = true;
         launch_info.SetArguments(args, first_arg_is_executable);
     }
     
     if (working_dir)
         launch_info.SetWorkingDirectory(working_dir);
+    llvm::SmallString<MAX_PATH> output_file_path;
     char output_file_path_buffer[PATH_MAX];
-    const char *output_file_path = NULL;
     
     if (command_output_ptr)
     {
         // Create a temporary file to get the stdout/stderr and redirect the
         // output of the command into this file. We will later read this file
         // if all goes well and fill the data into "command_output_ptr"
         FileSpec tmpdir_file_spec;
         if (HostInfo::GetLLDBPath(ePathTypeLLDBTempSystemDir, tmpdir_file_spec))
         {
-            tmpdir_file_spec.AppendPathComponent("lldb-shell-output.XXXXXX");
-            strncpy(output_file_path_buffer, tmpdir_file_spec.GetPath().c_str(), sizeof(output_file_path_buffer));
+            tmpdir_file_spec.AppendPathComponent("lldb-shell-output.%%%%%%");
+            llvm::sys::fs::createUniqueFile(tmpdir_file_spec.GetPath().c_str(), output_file_path);
         }
         else
         {
-            strncpy(output_file_path_buffer, "/tmp/lldb-shell-output.XXXXXX", sizeof(output_file_path_buffer));
+            llvm::sys::fs::createTemporaryFile("lldb-shell-output.%%%%%%", "", output_file_path);
         }
-        
-        output_file_path = ::mktemp(output_file_path_buffer);
     }
     
     launch_info.AppendSuppressFileAction (STDIN_FILENO, true, false);
-    if (output_file_path)
+    if (!output_file_path.empty())
     {
-        launch_info.AppendOpenFileAction(STDOUT_FILENO, output_file_path, false, true);
+        launch_info.AppendOpenFileAction(STDOUT_FILENO, output_file_path.c_str(), false, true);
         launch_info.AppendDuplicateFileAction(STDOUT_FILENO, STDERR_FILENO);
     }
     else
     {
         launch_info.AppendSuppressFileAction (STDOUT_FILENO, false, true);
         launch_info.AppendSuppressFileAction (STDERR_FILENO, false, true);
     }
     
if (error.Success())
                 *status_ptr = shell_info->status;
 
             if (signo_ptr)
                 *signo_ptr = shell_info->signo;
 
             if (command_output_ptr)
             {
                 command_output_ptr->clear();
-                FileSpec file_spec(output_file_path, File::eOpenOptionRead);
+                FileSpec file_spec(output_file_path.c_str(), File::eOpenOptionRead);
                 uint64_t file_size = file_spec.GetByteSize();
                 if (file_size > 0)
                 {
                     if (file_size > command_output_ptr->max_size())
                     {
                         error.SetErrorStringWithFormat("shell command output is too large to fit into a std::string");
                     }
                     else
                     {
                         command_output_ptr->resize(file_size);
                         file_spec.ReadFileContents(0, &((*command_output_ptr)[0]), command_output_ptr->size(), &error);
                     }
                 }
             }
         }
         shell_info->can_delete.SetValue(true, eBroadcastAlways);
     }
 
-    if (output_file_path)
-        ::unlink (output_file_path);
+    FileSpec output_file_spec(output_file_path.c_str(), false);
+    if (FileSystem::GetFileExists(output_file_spec))
+        FileSystem::Unlink(output_file_path.c_str());
     // Handshake with the monitor thread, or just let it know in advance that
     // it can delete "shell_info" in case we timed out and were not able to kill
     // the process...
     return error;
 }
 
 
 // LaunchProcessPosixSpawn for Apple, Linux, FreeBSD and other GLIBC

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

 #include "lldb/Core/Log.h"
 #include "lldb/Core/StreamString.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Host/HostInfoBase.h"
 
 #include "llvm/ADT/Triple.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Host.h"
+#include "llvm/Support/raw_ostream.h"
 
 #include <thread>
 
 using namespace lldb;
 using namespace lldb_private;
 
 namespace
 {
HostInfoBase::ComputeTempFileDirectory(FileSpec &file_spec)
     {
         tmpdir_cstr = getenv("TMP");
         if (tmpdir_cstr == NULL)
             tmpdir_cstr = getenv("TEMP");
     }
     if (!tmpdir_cstr)
         return false;
 
-    StreamString pid_tmpdir;
-    pid_tmpdir.Printf("%s/lldb", tmpdir_cstr);
-    if (!FileSystem::MakeDirectory(pid_tmpdir.GetString().c_str(), eFilePermissionsDirectoryDefault).Success())
+    FileSpec temp_file_spec(tmpdir_cstr, false);
+    temp_file_spec.AppendPathComponent("lldb");
+    if (!FileSystem::MakeDirectory(temp_file_spec.GetPath().c_str(), eFilePermissionsDirectoryDefault).Success())
         return false;
 
-    pid_tmpdir.Printf("/%" PRIu64, Host::GetCurrentProcessID());
-    if (!FileSystem::MakeDirectory(pid_tmpdir.GetString().c_str(), eFilePermissionsDirectoryDefault).Success())
+    std::string pid_str;
+    llvm::raw_string_ostream pid_stream(pid_str);
+    pid_stream << Host::GetCurrentProcessID();
+    temp_file_spec.AppendPathComponent(pid_stream.str().c_str());
+    std::string final_path = temp_file_spec.GetPath();
+    if (!FileSystem::MakeDirectory(final_path.c_str(), eFilePermissionsDirectoryDefault).Success())
         return false;
 
     // Make an atexit handler to clean up the process specify LLDB temp dir
     // and all of its contents.
     ::atexit(CleanupProcessSpecificLLDBTempDir);
-    file_spec.GetDirectory().SetCStringWithLength(pid_tmpdir.GetString().c_str(), pid_tmpdir.GetString().size());
+    file_spec.GetDirectory().SetCStringWithLength(final_path.c_str(), final_path.size());
     return true;
 }
 
 bool
 HostInfoBase::ComputeHeaderDirectory(FileSpec &file_spec)
 {
     // TODO(zturner): Figure out how to compute the header directory for all platforms.
     return false;

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

 }
 
 Error
 FileSystem::MakeDirectory(const char *path, uint32_t file_permissions)
 {
     // On Win32, the mode parameter is ignored, as Windows files and directories support a
     // different permission model than POSIX.
     Error error;
-    if (!::CreateDirectory(path, NULL))
+    if (!::CreateDirectory(path, NULL) && GetLastError() != ERROR_ALREADY_EXISTS)
         error.SetError(::GetLastError(), lldb::eErrorTypeWin32);
     return error;
 }
 
 Error
 FileSystem::DeleteDirectory(const char *path, bool recurse)
 {
     Error error;

lldb/trunk/source/Target/ProcessLaunchInfo.cpp

if (GetFlags().Test (eLaunchFlagLaunchInShell))
             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.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
+                // is a relative path.
                 const char *argv0 = argv[0];
-                if (argv0 && (argv0[0] != '/' && argv0[0] != '~'))
+                FileSpec arg_spec(argv0, false);
+                if (arg_spec.IsRelativeToCurrentWorkingDirectory())
                 {
                     // We have a relative path to our executable which may not work if
                     // we just try to run "a.out" (without it being converted to "./a.out")
                     const char *working_dir = GetWorkingDirectory();
                     // Be sure to put quotes around PATH's value in case any paths have spaces...
                     std::string new_path("PATH=\"");
                     const size_t empty_path_len = new_path.size();
 
if (GetFlags().Test (eLaunchFlagLaunchInShell))
                         if (new_path.size() > empty_path_len)
                             new_path += ':';
                         new_path += curr_path;
                     }
                     new_path += "\" ";
                     shell_command.PutCString(new_path.c_str());
                 }
 
-                shell_command.PutCString ("exec");
+                shell_command.PutCString("exec");
 
                 // Only Apple supports /usr/bin/arch being able to specify the architecture
                 if (GetArchitecture().IsValid() &&                                          // Valid architecture
                     GetArchitecture().GetTriple().getVendor() == llvm::Triple::Apple &&     // Apple only
                     GetArchitecture().GetCore() != ArchSpec::eCore_x86_64_x86_64h)          // Don't do this for x86_64h
                 {
                     shell_command.Printf(" /usr/bin/arch -arch %s", GetArchitecture().GetArchitectureName());
                     // Set the resume count to 2: