Remove FileSystem::MakeDirectory

[zturner]

Summary

Use the LLVM function instead.

There are two subtle behavioral changes here which I want to make clear so someone can determine whether this matters on their platform.

1. Previously all LLDB callers were passing eFilePermissionsDirectoryDefault which is equal to eFilePermissionsUserRWX (0o700). The LLVM default is equivalent to eFilePermissionsUserRWX | eFilePermissionsGroupRWX (0o770). If this is a problem it's easy to update all callsites to explicitly pass 0o700, but I don't think it is.

2. The implementation of MakeDirectory would first try to create the directory, then if it failed due to ENOENT would try to create the parent directory. But it only went up one level. So if /foo existed but not /foo/bar, and you tried to create /foo/bar/baz, it would create /foo/bar first and then /foo/bar/baz. On the other hand, if not even /foo existed, the function would fail.

This seems like very strange behavior to me, but I don't know if anyone depends on it. I imagine if the tests pass everywhere, then nobody does. They pass on Windows. If someone could confirm that they pass on other platforms after this patch it would be helpful.

[labath]

I think you missed one occurence in ModuleCache.cpp. This one creates a full directory structure, so you will probably need recursion there (unlike in the rest of these cases, which seem fine). Also unlike the rest of these cases, that one is actually covered by a unit test, so you should be able to verify that. :)

[zturner]

That one is calling a file static function MakeDirectory, not FileSystem::MakeDirectory, and the implementation of that function already calls llvm::sys::fs::create_directories() to create the whole tree, so it should be fine.

[labath]

In D31086#704122, @zturner wrote:

That one is calling a file static function MakeDirectory, not FileSystem::MakeDirectory, and the implementation of that function already calls llvm::sys::fs::create_directories() to create the whole tree, so it should be fine.

Right, sorry about that. I was looking at an old checkout. I'm not sure about the PlatformDarwin stuff, but the rest seems fine.

[sas]

The second behavioral change seems good but the first thing you described is a bit odd. Creating folders with 770 does not seem like a common behavior, and 700 or 755 is usually more standard.

Either way, I think this change is good.

[labath]

In D31086#704518, @sas wrote:

The second behavioral change seems good but the first thing you described is a bit odd. Creating folders with 770 does not seem like a common behavior, and 700 or 755 is usually more standard.

Don't forget that the real permissions will also reflect the user's umask(2). The completely standard way would be to use 777 or 666 for the permissions (except perhaps for some temporary/sensitive files), and then let the user choose what are the real permissions he wants by setting umask in his bashrc. I guess we are being a bit paranoid here and disallowing the "others" permissions just in case...

[sas]

In D31086#705159, @labath wrote:

In D31086#704518, @sas wrote:

The second behavioral change seems good but the first thing you described is a bit odd. Creating folders with 770 does not seem like a common behavior, and 700 or 755 is usually more standard.

Don't forget that the real permissions will also reflect the user's umask(2). The completely standard way would be to use 777 or 666 for the permissions (except perhaps for some temporary/sensitive files), and then let the user choose what are the real permissions he wants by setting umask in his bashrc. I guess we are being a bit paranoid here and disallowing the "others" permissions just in case...

Didn't know that. If these respect the user's umask(2) then this is good.

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

 namespace lldb_private {
 class FileSystem {
 public:
   static const char *DEV_NULL;
   static const char *PATH_CONVERSION_ERROR;
 
   static FileSpec::PathSyntax GetNativePathSyntax();
 
-  static Error MakeDirectory(const FileSpec &file_spec, uint32_t mode);
-
   static Error GetFilePermissions(const FileSpec &file_spec,
                                   uint32_t &file_permissions);
   static Error SetFilePermissions(const FileSpec &file_spec,
                                   uint32_t file_permissions);
   static lldb::user_id_t GetFileSize(const FileSpec &file_spec);
   static bool GetFileExists(const FileSpec &file_spec);
 
   static Error Hardlink(const FileSpec &src, const FileSpec &dst);

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

 #include <iomanip>
 #include <iostream>
 #include <limits.h>
 
 #include "lldb/Core/StringList.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/Editline.h"
 #include "lldb/Host/FileSpec.h"
-#include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Utility/Error.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/SelectHelper.h"
 #include "lldb/Utility/StreamString.h"
 
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
 
 using namespace lldb_private;
 using namespace lldb_private::line_editor;
 
 // Workaround for what looks like an OS X-specific issue, but other platforms
 // may benefit from something similar if issues arise.  The libedit library
 // doesn't explicitly initialize the curses termcap library, which it gets away
EditlineHistory(const std::string &prefix, uint32_t size, bool unique_entries)
     if (unique_entries)
       history_w(m_history, &m_event, H_SETUNIQUE, 1);
   }
 
   const char *GetHistoryFilePath() {
     if (m_path.empty() && m_history && !m_prefix.empty()) {
       FileSpec parent_path{"~/.lldb", true};
       char history_path[PATH_MAX];
-      if (FileSystem::MakeDirectory(parent_path,
-                                    lldb::eFilePermissionsDirectoryDefault)
-              .Success()) {
+      if (!llvm::sys::fs::create_directory(parent_path.GetPath()) {
         snprintf(history_path, sizeof(history_path), "~/.lldb/%s-history",
                  m_prefix.c_str());
       } else {
         snprintf(history_path, sizeof(history_path), "~/%s-widehistory",
                  m_prefix.c_str());
       }
       m_path = FileSpec(history_path, true).GetPath();
     }

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

 
 bool HostInfoBase::ComputeProcessTempFileDirectory(FileSpec &file_spec) {
   FileSpec temp_file_spec;
   if (!HostInfo::ComputeGlobalTempFileDirectory(temp_file_spec))
     return false;
 
   std::string pid_str{llvm::to_string(Host::GetCurrentProcessID())};
   temp_file_spec.AppendPathComponent(pid_str);
-  if (!FileSystem::MakeDirectory(temp_file_spec,
-                                 eFilePermissionsDirectoryDefault)
-           .Success())
+  if (llvm::sys::fs::create_directory(temp_file_spec.GetPath()))
     return false;
 
   file_spec.GetDirectory().SetCString(temp_file_spec.GetCString());
   return true;
 }
 
 bool HostInfoBase::ComputeTempFileBaseDirectory(FileSpec &file_spec) {
   llvm::SmallVector<char, 16> tmpdir;
   llvm::sys::path::system_temp_directory(/*ErasedOnReboot*/ true, tmpdir);
   file_spec = FileSpec(std::string(tmpdir.data(), tmpdir.size()), true);
   return true;
 }
 
 bool HostInfoBase::ComputeGlobalTempFileDirectory(FileSpec &file_spec) {
   file_spec.Clear();
 
   FileSpec temp_file_spec;
   if (!HostInfo::ComputeTempFileBaseDirectory(temp_file_spec))
     return false;
 
   temp_file_spec.AppendPathComponent("lldb");
-  if (!FileSystem::MakeDirectory(temp_file_spec,
-                                 eFilePermissionsDirectoryDefault)
-           .Success())
+  if (llvm::sys::fs::create_directory(temp_file_spec.GetPath()))
     return false;
 
   file_spec.GetDirectory().SetCString(temp_file_spec.GetCString());
   return true;
 }
 
 bool HostInfoBase::ComputeHeaderDirectory(FileSpec &file_spec) {
   // TODO(zturner): Figure out how to compute the header directory for all

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

 using namespace lldb_private;
 
 const char *FileSystem::DEV_NULL = "/dev/null";
 
 FileSpec::PathSyntax FileSystem::GetNativePathSyntax() {
   return FileSpec::ePathSyntaxPosix;
 }
 
-Error FileSystem::MakeDirectory(const FileSpec &file_spec,
-                                uint32_t file_permissions) {
-  if (file_spec) {
-    Error error;
-    if (::mkdir(file_spec.GetCString(), file_permissions) == -1) {
-      error.SetErrorToErrno();
-      errno = 0;
-      switch (error.GetError()) {
-      case ENOENT: {
-        // Parent directory doesn't exist, so lets make it if we can
-        // Make the parent directory and try again
-        FileSpec parent_file_spec{file_spec.GetDirectory().GetCString(), false};
-        error = MakeDirectory(parent_file_spec, file_permissions);
-        if (error.Fail())
-          return error;
-        // Try and make the directory again now that the parent directory was
-        // made successfully
-        if (::mkdir(file_spec.GetCString(), file_permissions) == -1) {
-          error.SetErrorToErrno();
-        }
-        return error;
-      } break;
-      case EEXIST: {
-        if (llvm::sys::fs::is_directory(file_spec.GetPath()))
-          return Error(); // It is a directory and it already exists
-      } break;
-      }
-    }
-    return error;
-  }
-  return Error("empty path");
-}
-
 Error FileSystem::GetFilePermissions(const FileSpec &file_spec,
                                      uint32_t &file_permissions) {
   Error error;
   struct stat file_stats;
   if (::stat(file_spec.GetCString(), &file_stats) == 0) {
     // The bits in "st_mode" currently match the definitions
     // for the file mode bits in unix.
     file_permissions = file_stats.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO);

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

 
 const char *FileSystem::PATH_CONVERSION_ERROR =
     "Error converting path between UTF-8 and native encoding";
 
 FileSpec::PathSyntax FileSystem::GetNativePathSyntax() {
   return FileSpec::ePathSyntaxWindows;
 }
 
-Error FileSystem::MakeDirectory(const FileSpec &file_spec,
-                                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;
-  const auto err_code =
-      llvm::sys::fs::create_directories(file_spec.GetPath(), true);
-  if (err_code) {
-    error.SetErrorString(err_code.message().c_str());
-  }
-
-  return error;
-}
-
 Error FileSystem::GetFilePermissions(const FileSpec &file_spec,
                                      uint32_t &file_permissions) {
   Error error;
   // Beware that Windows's permission model is different from Unix's, and it's
   // not clear if this API is supposed to check ACLs.  To match the caller's
   // expectations as closely as possible, we'll use Microsoft's _stat, which
   // attempts to emulate POSIX stat.  This should be good enough for basic
   // checks like FileSpec::Readable.

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

Error PlatformDarwin::ResolveSymbolFile(Target &target,
   }
   return error;
 }
 
 static lldb_private::Error
 MakeCacheFolderForFile(const FileSpec &module_cache_spec) {
   FileSpec module_cache_folder =
       module_cache_spec.CopyByRemovingLastPathComponent();
-  return FileSystem::MakeDirectory(module_cache_folder,
-                                   eFilePermissionsDirectoryDefault);
+  return llvm::sys::fs::create_directory(module_cache_folder.GetPath());
 }
 
 static lldb_private::Error
 BringInRemoteFile(Platform *platform,
                   const lldb_private::ModuleSpec &module_spec,
                   const FileSpec &module_cache_spec) {
   MakeCacheFolderForFile(module_cache_spec);
   Error err = platform->GetFile(module_spec.GetFileSpec(), module_cache_spec);

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

 // Other libraries and framework includes
 // Project includes
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Host/FileSpec.h"
-#include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/Error.h"
 #include "lldb/Utility/Log.h"
if (local_os_build.compare(remote_os_build) == 0) {
       if (module_cache_spec.Exists()) {
         local_file = module_cache_spec;
         return Error();
       }
       // bring in the remote module file
       FileSpec module_cache_folder =
           module_cache_spec.CopyByRemovingLastPathComponent();
       // try to make the local directory first
-      Error err = FileSystem::MakeDirectory(module_cache_folder,
-                                            eFilePermissionsDirectoryDefault);
+      Error err(llvm::sys::fs::create_directory(module_cache_folder.GetPath()));
       if (err.Fail())
         return err;
       err = GetFile(platform_file, module_cache_spec);
       if (err.Fail())
         return err;
       if (module_cache_spec.Exists()) {
         local_file = module_cache_spec;
         return Error();

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp

 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerCommon::Handle_qPlatform_mkdir(
     StringExtractorGDBRemote &packet) {
   packet.SetFilePos(::strlen("qPlatform_mkdir:"));
   mode_t mode = packet.GetHexMaxU32(false, UINT32_MAX);
   if (packet.GetChar() == ',') {
     std::string path;
     packet.GetHexByteString(path);
-    Error error = FileSystem::MakeDirectory(FileSpec{path, false}, mode);
+    Error error(llvm::sys::fs::create_directory(path, mode));
 
     StreamGDBRemote response;
     response.Printf("F%u", error.GetError());
 
     return SendPacketNoLock(response.GetString());
   }
   return SendErrorResponse(20);
 }

lldb/trunk/source/Target/Platform.cpp

bool Platform::SetWorkingDirectory(const FileSpec &file_spec) {
   } else {
     m_working_dir.Clear();
     return SetRemoteWorkingDirectory(file_spec);
   }
 }
 
 Error Platform::MakeDirectory(const FileSpec &file_spec, uint32_t permissions) {
   if (IsHost())
-    return FileSystem::MakeDirectory(file_spec, permissions);
+    return llvm::sys::fs::create_directory(file_spec.GetPath(), permissions);
   else {
     Error error;
     error.SetErrorStringWithFormat("remote platform %s doesn't support %s",
                                    GetPluginName().GetCString(),
                                    LLVM_PRETTY_FUNCTION);
     return error;
   }
 }

lldb/trunk/tools/lldb-server/lldb-platform.cpp

 #include "llvm/Support/FileUtilities.h"
 
 #include "Acceptor.h"
 #include "LLDBServerUtilities.h"
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h"
 #include "Plugins/Process/gdb-remote/ProcessGDBRemoteLog.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/FileSpec.h"
-#include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostGetOpt.h"
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Host/common/TCPSocket.h"
 #include "lldb/Utility/Error.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::lldb_server;
fprintf(stderr, "Usage:\n  %s %s [--log-file log-file-name] [--log-channels "
                   "--listen port\n",
           progname, subcommand);
   exit(0);
 }
 
 static Error save_socket_id_to_file(const std::string &socket_id,
                                     const FileSpec &file_spec) {
   FileSpec temp_file_spec(file_spec.GetDirectory().AsCString(), false);
-  auto error = FileSystem::MakeDirectory(temp_file_spec,
-                                         eFilePermissionsDirectoryDefault);
+  Error error(llvm::sys::fs::create_directory(temp_file_spec.GetPath()));
   if (error.Fail())
     return Error("Failed to create directory %s: %s",
                  temp_file_spec.GetCString(), error.AsCString());
 
   llvm::SmallString<PATH_MAX> temp_file_path;
   temp_file_spec.AppendPathComponent("port-file.%%%%%%");
   auto err_code = llvm::sys::fs::createUniqueFile(temp_file_spec.GetCString(),
                                                   temp_file_path);