zturner created this revision.
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.
https://reviews.llvm.org/D31086
Files:
lldb/include/lldb/Host/FileSystem.h
lldb/source/Host/common/Editline.cpp
lldb/source/Host/common/HostInfoBase.cpp
lldb/source/Host/posix/FileSystem.cpp
lldb/source/Host/windows/FileSystem.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
lldb/source/Target/Platform.cpp
lldb/tools/lldb-server/lldb-platform.cpp
Index: lldb/tools/lldb-server/lldb-platform.cpp
===================================================================
--- lldb/tools/lldb-server/lldb-platform.cpp
+++ lldb/tools/lldb-server/lldb-platform.cpp
@@ -32,7 +32,6 @@
#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"
@@ -102,8 +101,7 @@
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());
Index: lldb/source/Target/Platform.cpp
===================================================================
--- lldb/source/Target/Platform.cpp
+++ lldb/source/Target/Platform.cpp
@@ -761,7 +761,7 @@
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",
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
@@ -794,7 +794,7 @@
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());
Index: lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
===================================================================
--- lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
@@ -22,7 +22,6 @@
#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"
@@ -279,8 +278,7 @@
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);
Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
===================================================================
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
@@ -225,8 +225,7 @@
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
Index: lldb/source/Host/windows/FileSystem.cpp
===================================================================
--- lldb/source/Host/windows/FileSystem.cpp
+++ lldb/source/Host/windows/FileSystem.cpp
@@ -30,21 +30,6 @@
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;
Index: lldb/source/Host/posix/FileSystem.cpp
===================================================================
--- lldb/source/Host/posix/FileSystem.cpp
+++ lldb/source/Host/posix/FileSystem.cpp
@@ -40,39 +40,6 @@
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;
Index: lldb/source/Host/common/HostInfoBase.cpp
===================================================================
--- lldb/source/Host/common/HostInfoBase.cpp
+++ lldb/source/Host/common/HostInfoBase.cpp
@@ -314,9 +314,7 @@
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());
@@ -338,9 +336,7 @@
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());
Index: lldb/source/Host/common/Editline.cpp
===================================================================
--- lldb/source/Host/common/Editline.cpp
+++ lldb/source/Host/common/Editline.cpp
@@ -15,13 +15,13 @@
#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;
@@ -178,9 +178,7 @@
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 {
Index: lldb/include/lldb/Host/FileSystem.h
===================================================================
--- lldb/include/lldb/Host/FileSystem.h
+++ lldb/include/lldb/Host/FileSystem.h
@@ -28,8 +28,6 @@
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,
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
