[Lldb-commits] FileSpec optimizations
Zachary Turner
zturner at google.com
Mon Aug 4 11:00:04 PDT 2014
Hi Jim,
This patch implements some of the optimizations to FileSpec requested by
Greg. In particular, normalization no longer does its own copy, and does
the operation in place. Changing Normalize this way required some changes
to function signatures, which trickled down a bit and required me to change
the signature of ResolveUsername as well, but the resulting implementation
of ResolveUsername should have the same or better performance
characteristics, and is easier to read due to the use of C++ string
operations.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140804/3e494f66/attachment.html>
-------------- next part --------------
diff --git a/include/lldb/Host/FileSpec.h b/include/lldb/Host/FileSpec.h
index 6dfb9d4..14217a2 100644
--- a/include/lldb/Host/FileSpec.h
+++ b/include/lldb/Host/FileSpec.h
@@ -570,8 +570,8 @@ public:
lldb::DataBufferSP
ReadFileContentsAsCString(Error *error_ptr = NULL);
- static void Normalize(llvm::StringRef path, llvm::SmallVectorImpl<char> &result, PathSyntax syntax);
- static void DeNormalize(llvm::StringRef path, llvm::SmallVectorImpl<char> &result, PathSyntax syntax);
+ static void Normalize(llvm::SmallVectorImpl<char> &path, PathSyntax syntax = ePathSyntaxHostNative);
+ static void DeNormalize(llvm::SmallVectorImpl<char> &path, PathSyntax syntax = ePathSyntaxHostNative);
//------------------------------------------------------------------
/// Change the file specified with a new path.
@@ -636,22 +636,11 @@ public:
/// exist, the contents of \a src_path will be copied to \a dst_path
/// unchanged.
///
- /// @param[in] src_path
- /// Input path to be resolved.
- ///
- /// @param[in] dst_path
- /// Buffer to store the resolved path.
- ///
- /// @param[in] dst_len
- /// Size of the buffer pointed to by dst_path.
- ///
- /// @result
- /// The number of characters required to write the resolved path. If the
- /// resolved path doesn't fit in dst_len, dst_len-1 characters will
- /// be written to \a dst_path, but the actual required length will still be returned.
+ /// @param[in] path
+ /// Input path to be resolved, in the form of a llvm::SmallString or similar.
//------------------------------------------------------------------
- static size_t
- Resolve (const char *src_path, char *dst_path, size_t dst_len);
+ static void
+ Resolve (llvm::SmallVectorImpl<char> &path);
FileSpec
CopyByAppendingPathComponent (const char *new_path) const;
@@ -679,18 +668,9 @@ public:
///
/// @param[in] dst_path
/// Buffer to store the resolved path.
- ///
- /// @param[in] dst_len
- /// Size of the buffer pointed to by dst_path.
- ///
- /// @result
- /// The number of characters required to write the resolved path, or 0 if
- /// the user name could not be found. If the
- /// resolved path doesn't fit in dst_len, dst_len-1 characters will
- /// be written to \a dst_path, but the actual required length will still be returned.
//------------------------------------------------------------------
- static size_t
- ResolveUsername (const char *src_path, char *dst_path, size_t dst_len);
+ static void
+ ResolveUsername (llvm::SmallVectorImpl<char> &path);
static size_t
ResolvePartialUsername (const char *partial_name, StringList &matches);
diff --git a/source/API/SBFileSpec.cpp b/source/API/SBFileSpec.cpp
index 0de1c24..8d63fc5 100644
--- a/source/API/SBFileSpec.cpp
+++ b/source/API/SBFileSpec.cpp
@@ -16,6 +16,8 @@
#include "lldb/Core/Log.h"
#include "lldb/Core/Stream.h"
+#include "llvm/ADT/SmallString.h"
+
using namespace lldb;
using namespace lldb_private;
@@ -89,7 +91,11 @@ SBFileSpec::ResolveExecutableLocation ()
int
SBFileSpec::ResolvePath (const char *src_path, char *dst_path, size_t dst_len)
{
- return lldb_private::FileSpec::Resolve (src_path, dst_path, dst_len);
+ llvm::SmallString<64> result(src_path);
+ lldb_private::FileSpec::Resolve (result);
+ size_t result_length = std::min(dst_len-1, result.size());
+ ::strncpy(dst_path, result.c_str(), result_length + 1);
+ return result_length;
}
const char *
diff --git a/source/Commands/CommandCompletions.cpp b/source/Commands/CommandCompletions.cpp
index 970aa69..f0ad4a8 100644
--- a/source/Commands/CommandCompletions.cpp
+++ b/source/Commands/CommandCompletions.cpp
@@ -30,6 +30,8 @@
#include "lldb/Target/Target.h"
#include "lldb/Utility/CleanUp.h"
+#include "llvm/ADT/SmallString.h"
+
using namespace lldb_private;
CommandCompletions::CommonCompletionElement
@@ -221,7 +223,7 @@ DiskFilesOrDirectories
end_ptr = strrchr(partial_name_copy, '/');
// This will store the resolved form of the containing directory
- char containing_part[PATH_MAX];
+ llvm::SmallString<64> containing_part;
if (end_ptr == NULL)
{
@@ -232,14 +234,11 @@ DiskFilesOrDirectories
// Nothing here but the user name. We could just put a slash on the end,
// but for completeness sake we'll resolve the user name and only put a slash
// on the end if it exists.
- char resolved_username[PATH_MAX];
- size_t resolved_username_len = FileSpec::ResolveUsername (partial_name_copy, resolved_username,
- sizeof (resolved_username));
+ llvm::SmallString<64> resolved_username(partial_name_copy);
+ FileSpec::ResolveUsername (resolved_username);
// Not sure how this would happen, a username longer than PATH_MAX? Still...
- if (resolved_username_len >= sizeof (resolved_username))
- return matches.GetSize();
- else if (resolved_username_len == 0)
+ if (resolved_username.size() == 0)
{
// The user name didn't resolve, let's look in the password database for matches.
// The user name database contains duplicates, and is not in alphabetical order, so
@@ -263,8 +262,7 @@ DiskFilesOrDirectories
else
{
// The containing part is the CWD, and the whole string is the remainder.
- containing_part[0] = '.';
- containing_part[1] = '\0';
+ containing_part = ".";
strcpy(remainder, partial_name_copy);
end_ptr = partial_name_copy;
}
@@ -274,14 +272,11 @@ DiskFilesOrDirectories
if (end_ptr == partial_name_copy)
{
// We're completing a file or directory in the root volume.
- containing_part[0] = '/';
- containing_part[1] = '\0';
+ containing_part = "/";
}
else
{
- size_t len = end_ptr - partial_name_copy;
- memcpy(containing_part, partial_name_copy, len);
- containing_part[len] = '\0';
+ containing_part.append(partial_name_copy, end_ptr);
}
// Push end_ptr past the final "/" and set remainder.
end_ptr++;
@@ -293,11 +288,9 @@ DiskFilesOrDirectories
if (*partial_name_copy == '~')
{
- size_t resolved_username_len = FileSpec::ResolveUsername(containing_part,
- containing_part,
- sizeof (containing_part));
+ FileSpec::ResolveUsername(containing_part);
// User name doesn't exist, we're not getting any further...
- if (resolved_username_len == 0 || resolved_username_len >= sizeof (containing_part))
+ if (containing_part.empty())
return matches.GetSize();
}
@@ -314,7 +307,7 @@ DiskFilesOrDirectories
parameters.end_ptr = end_ptr;
parameters.baselen = baselen;
- FileSpec::EnumerateDirectory(containing_part, true, true, true, DiskFilesOrDirectoriesCallback, ¶meters);
+ FileSpec::EnumerateDirectory(containing_part.c_str(), true, true, true, DiskFilesOrDirectoriesCallback, ¶meters);
return matches.GetSize();
}
diff --git a/source/Host/common/FileSpec.cpp b/source/Host/common/FileSpec.cpp
index 845a318..615772e 100644
--- a/source/Host/common/FileSpec.cpp
+++ b/source/Host/common/FileSpec.cpp
@@ -28,6 +28,7 @@
#endif
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
@@ -77,81 +78,66 @@ GetCachedGlobTildeSlash()
// Resolves the username part of a path of the form ~user/other/directories, and
// writes the result into dst_path.
-// Returns 0 if there WAS a ~ in the path but the username couldn't be resolved.
-// Otherwise returns the number of characters copied into dst_path. If the return
-// is >= dst_len, then the resolved path is too long...
-size_t
-FileSpec::ResolveUsername (const char *src_path, char *dst_path, size_t dst_len)
+void
+FileSpec::ResolveUsername (llvm::SmallVectorImpl<char> &path)
{
- if (src_path == NULL || src_path[0] == '\0')
- return 0;
-
-#ifdef LLDB_CONFIG_TILDE_RESOLVES_TO_USER
-
- char user_home[PATH_MAX];
- const char *user_name;
-
-
- // If there's no ~, then just copy src_path straight to dst_path (they may be the same string...)
- if (src_path[0] != '~')
- {
- size_t len = strlen (src_path);
- if (len >= dst_len)
- {
- ::bcopy (src_path, dst_path, dst_len - 1);
- dst_path[dst_len] = '\0';
- }
- else
- ::bcopy (src_path, dst_path, len + 1);
-
- return len;
- }
-
- const char *first_slash = ::strchr (src_path, '/');
- char remainder[PATH_MAX];
+#if LLDB_CONFIG_TILDE_RESOLVES_TO_USER
+ if (path.empty() || path[0] != '~')
+ return;
- if (first_slash == NULL)
+ llvm::StringRef path_str(path.data());
+ size_t slash_pos = path_str.find_first_of("/", 1);
+ if (slash_pos == 1)
{
- // The whole name is the username (minus the ~):
- user_name = src_path + 1;
- remainder[0] = '\0';
- }
- else
- {
- size_t user_name_len = first_slash - src_path - 1;
- ::memcpy (user_home, src_path + 1, user_name_len);
- user_home[user_name_len] = '\0';
- user_name = user_home;
+ // A path of the form ~/ resolves to the current user's home dir
+ llvm::SmallString<64> home_dir;
+ if (!llvm::sys::path::home_directory(home_dir))
+ return;
- ::strcpy (remainder, first_slash);
+ // Overwrite the ~ with the first character of the homedir, and insert
+ // the rest. This way we only trigger one move, whereas an insert
+ // followed by a delete (or vice versa) would trigger two.
+ path[0] = home_dir[0];
+ path.insert(path.begin() + 1, home_dir.begin() + 1, home_dir.end());
+ return;
}
- if (user_name == NULL)
- return 0;
- // User name of "" means the current user...
-
- struct passwd *user_entry;
- const char *home_dir = NULL;
-
- if (user_name[0] == '\0')
+ auto username_begin = path.begin()+1;
+ auto username_end = (slash_pos == llvm::StringRef::npos)
+ ? path.end()
+ : (path.begin() + slash_pos);
+ size_t replacement_length = std::distance(path.begin(), username_end);
+
+ llvm::SmallString<20> username(username_begin, username_end);
+ struct passwd *user_entry = ::getpwnam(username.c_str());
+ if (user_entry != nullptr)
{
- home_dir = GetCachedGlobTildeSlash();
+ // Copy over the first n characters of the path, where n is the smaller of the length
+ // of the home directory and the slash pos.
+ llvm::StringRef homedir(user_entry->pw_dir);
+ size_t initial_copy_length = std::min(homedir.size(), replacement_length);
+ auto src_begin = homedir.begin();
+ auto src_end = src_begin + initial_copy_length;
+ std::copy(src_begin, src_end, path.begin());
+ if (replacement_length > homedir.size())
+ {
+ // We copied the entire home directory, but the ~username portion of the path was
+ // longer, so there's characters that need to be removed.
+ path.erase(path.begin() + initial_copy_length, username_end);
+ }
+ else if (replacement_length < homedir.size())
+ {
+ // We copied all the way up to the slash in the destination, but there's still more
+ // characters that need to be inserted.
+ path.insert(username_end, src_end, homedir.end());
+ }
}
else
{
- user_entry = ::getpwnam (user_name);
- if (user_entry != NULL)
- home_dir = user_entry->pw_dir;
+ // Unable to resolve username (user doesn't exist?)
+ path.clear();
}
-
- if (home_dir == NULL)
- return 0;
- else
- return ::snprintf (dst_path, dst_len, "%s%s", home_dir, remainder);
-#else
- // Resolving home directories is not supported, just copy the path...
- return ::snprintf (dst_path, dst_len, "%s", src_path);
-#endif // #ifdef LLDB_CONFIG_TILDE_RESOLVES_TO_USER
+#endif
}
size_t
@@ -187,44 +173,18 @@ FileSpec::ResolvePartialUsername (const char *partial_name, StringList &matches)
#endif // #ifdef LLDB_CONFIG_TILDE_RESOLVES_TO_USER
}
-
-
-size_t
-FileSpec::Resolve (const char *src_path, char *dst_path, size_t dst_len)
+void
+FileSpec::Resolve (llvm::SmallVectorImpl<char> &path)
{
- if (src_path == NULL || src_path[0] == '\0')
- return 0;
+ if (path.empty())
+ return;
- // Glob if needed for ~/, otherwise copy in case src_path is same as dst_path...
- char unglobbed_path[PATH_MAX];
#ifdef LLDB_CONFIG_TILDE_RESOLVES_TO_USER
- if (src_path[0] == '~')
- {
- size_t return_count = ResolveUsername(src_path, unglobbed_path, sizeof(unglobbed_path));
-
- // If we couldn't find the user referred to, or the resultant path was too long,
- // then just copy over the src_path.
- if (return_count == 0 || return_count >= sizeof(unglobbed_path))
- ::snprintf (unglobbed_path, sizeof(unglobbed_path), "%s", src_path);
- }
- else
+ if (path[0] == '~')
+ ResolveUsername(path);
#endif // #ifdef LLDB_CONFIG_TILDE_RESOLVES_TO_USER
- {
- ::snprintf(unglobbed_path, sizeof(unglobbed_path), "%s", src_path);
- }
- // Now resolve the path if needed
- char resolved_path[PATH_MAX];
- if (::realpath (unglobbed_path, resolved_path))
- {
- // Success, copy the resolved path
- return ::snprintf(dst_path, dst_len, "%s", resolved_path);
- }
- else
- {
- // Failed, just copy the unglobbed path
- return ::snprintf(dst_path, dst_len, "%s", unglobbed_path);
- }
+ llvm::sys::fs::make_absolute(path);
}
FileSpec::FileSpec() :
@@ -292,24 +252,20 @@ FileSpec::operator= (const FileSpec& rhs)
return *this;
}
-void FileSpec::Normalize(llvm::StringRef path, llvm::SmallVectorImpl<char> &result, PathSyntax syntax)
+void FileSpec::Normalize(llvm::SmallVectorImpl<char> &path, PathSyntax syntax)
{
- result.clear();
- result.append(path.begin(), path.end());
if (syntax == ePathSyntaxPosix || (syntax == ePathSyntaxHostNative && Host::GetHostPathSyntax() == ePathSyntaxPosix))
return;
- std::replace(result.begin(), result.end(), '\\', '/');
+ std::replace(path.begin(), path.end(), '\\', '/');
}
-void FileSpec::DeNormalize(llvm::StringRef path, llvm::SmallVectorImpl<char> &result, PathSyntax syntax)
+void FileSpec::DeNormalize(llvm::SmallVectorImpl<char> &path, PathSyntax syntax)
{
- result.clear();
- result.append(path.begin(), path.end());
if (syntax == ePathSyntaxPosix || (syntax == ePathSyntaxHostNative && Host::GetHostPathSyntax() == ePathSyntaxPosix))
return;
- std::replace(result.begin(), result.end(), '/', '\\');
+ std::replace(path.begin(), path.end(), '/', '\\');
}
//------------------------------------------------------------------
@@ -328,41 +284,26 @@ FileSpec::SetFile (const char *pathname, bool resolve, PathSyntax syntax)
if (pathname == NULL || pathname[0] == '\0')
return;
- llvm::SmallString<64> normalized;
- Normalize(pathname, normalized, syntax);
+ llvm::SmallString<64> normalized(pathname);
+ Normalize(normalized, syntax);
- std::vector<char> resolved_path(PATH_MAX);
- bool path_fit = true;
-
if (resolve)
{
- path_fit = (FileSpec::Resolve (normalized.c_str(), &resolved_path[0], resolved_path.size()) < resolved_path.size() - 1);
- m_is_resolved = path_fit;
- }
- else
- {
- // Copy the path because "basename" and "dirname" want to muck with the
- // path buffer
- if (normalized.size() > resolved_path.size() - 1)
- path_fit = false;
- else
- ::strcpy (&resolved_path[0], normalized.c_str());
+ FileSpec::Resolve (normalized);
+ m_is_resolved = true;
}
- if (path_fit)
+ llvm::StringRef resolve_path_ref(normalized.c_str());
+ llvm::StringRef filename_ref = llvm::sys::path::filename(resolve_path_ref);
+ if (!filename_ref.empty())
{
- llvm::StringRef resolve_path_ref(&resolved_path[0]);
- llvm::StringRef filename_ref = llvm::sys::path::filename(resolve_path_ref);
- if (!filename_ref.empty())
- {
- m_filename.SetString (filename_ref);
- llvm::StringRef directory_ref = llvm::sys::path::parent_path(resolve_path_ref);
- if (!directory_ref.empty())
- m_directory.SetString(directory_ref);
- }
- else
- m_directory.SetCString(&resolved_path[0]);
+ m_filename.SetString (filename_ref);
+ llvm::StringRef directory_ref = llvm::sys::path::parent_path(resolve_path_ref);
+ if (!directory_ref.empty())
+ m_directory.SetString(directory_ref);
}
+ else
+ m_directory.SetCString(normalized.c_str());
}
//----------------------------------------------------------------------
@@ -732,11 +673,7 @@ FileSpec::GetPath (bool denormalize) const
if (m_filename)
llvm::sys::path::append(result, m_filename.GetCString());
if (denormalize && !result.empty())
- {
- llvm::SmallString<64> denormalized;
- DeNormalize(result.c_str(), denormalized, m_syntax);
- result = denormalized;
- }
+ DeNormalize(result, m_syntax);
return std::string(result.begin(), result.end());
}
diff --git a/source/Host/common/Host.cpp b/source/Host/common/Host.cpp
index a196083..c469ead 100644
--- a/source/Host/common/Host.cpp
+++ b/source/Host/common/Host.cpp
@@ -1119,7 +1119,6 @@ Host::GetLLDBPath (PathType path_type, FileSpec &file_spec)
if (GetLLDBPath (ePathTypeLLDBShlibDir, lldb_file_spec))
{
char raw_path[PATH_MAX];
- char resolved_path[PATH_MAX];
lldb_file_spec.GetPath(raw_path, sizeof(raw_path));
#if defined (__APPLE__)
@@ -1159,8 +1158,9 @@ Host::GetLLDBPath (PathType path_type, FileSpec &file_spec)
log->Printf ("Host::%s() failed to find /lib/liblldb within the shared lib path, bailing on bin path construction", __FUNCTION__);
}
#endif // #if defined (__APPLE__)
- FileSpec::Resolve (raw_path, resolved_path, sizeof(resolved_path));
- g_lldb_support_exe_dir.SetCString(resolved_path);
+ llvm::SmallString<64> resolved_path(raw_path);
+ FileSpec::Resolve (resolved_path);
+ g_lldb_support_exe_dir.SetCString(resolved_path.c_str());
}
if (log)
log->Printf("Host::GetLLDBPath(ePathTypeSupportExecutableDir) => '%s'", g_lldb_support_exe_dir.GetCString());
@@ -1217,7 +1217,6 @@ Host::GetLLDBPath (PathType path_type, FileSpec &file_spec)
if (GetLLDBPath (ePathTypeLLDBShlibDir, lldb_file_spec))
{
char raw_path[PATH_MAX];
- char resolved_path[PATH_MAX];
#if defined(_WIN32)
lldb_file_spec.AppendPathComponent("../lib/site-packages");
lldb_file_spec.GetPath(raw_path, sizeof(raw_path));
@@ -1248,8 +1247,9 @@ Host::GetLLDBPath (PathType path_type, FileSpec &file_spec)
#if defined (__APPLE__)
}
#endif
- FileSpec::Resolve (raw_path, resolved_path, sizeof(resolved_path));
- g_lldb_python_dir.SetCString(resolved_path);
+ llvm::SmallString<64> resolved_path(raw_path);
+ FileSpec::Resolve (resolved_path);
+ g_lldb_python_dir.SetCString(resolved_path.c_str());
}
if (log)
diff --git a/source/Target/TargetList.cpp b/source/Target/TargetList.cpp
index 091211d..d177521 100644
--- a/source/Target/TargetList.cpp
+++ b/source/Target/TargetList.cpp
@@ -28,6 +28,8 @@
#include "lldb/Target/Process.h"
#include "lldb/Target/TargetList.h"
+#include "llvm/ADT/SmallString.h"
+
using namespace lldb;
using namespace lldb_private;
@@ -241,15 +243,13 @@ TargetList::CreateTarget (Debugger &debugger,
{
// we want to expand the tilde but we don't want to resolve any symbolic links
// so we can't use the FileSpec constructor's resolve flag
- char unglobbed_path[PATH_MAX];
- unglobbed_path[0] = '\0';
-
- size_t return_count = FileSpec::ResolveUsername(user_exe_path, unglobbed_path, sizeof(unglobbed_path));
-
- if (return_count == 0 || return_count >= sizeof(unglobbed_path))
- ::snprintf (unglobbed_path, sizeof(unglobbed_path), "%s", user_exe_path);
+ llvm::SmallString<64> unglobbed_path(user_exe_path);
+ FileSpec::ResolveUsername(unglobbed_path);
- file = FileSpec(unglobbed_path, false);
+ if (unglobbed_path.empty())
+ file = FileSpec(user_exe_path, false);
+ else
+ file = FileSpec(unglobbed_path.c_str(), false);
}
bool user_exe_path_is_bundle = false;
More information about the lldb-commits
mailing list