[lldb][NFC] avoid unnecessary roundtrips between different string types

[xujuntwt95329]

Summary

The amount of roundtrips between StringRefs, ConstStrings and std::strings is
getting a bit out of hand, this patch avoid the unnecessary roundtrips.

[wallace]

thanks!

[dblaikie]

lldb/source/API/SBTarget.cpp

1589–1590

Aside (since the old code did this too): Generally code should prefer = initialization over () initialization - because the former can't invoke explicit conversions, so is easier to read because it limits the possible conversion to simpler/safer ones (for instance, with unique ptrs "std::unique_ptr<int> p = x();" tells me x() returns a unique_ptr or equivalent, but std::unique_ptr<int> p(x()); could be that x() returns a raw pointer and then I have to wonder if it meant to transfer ownership or not - but I don't have to wonder that in the first example because the type system checks that for me).

Admittedly StringRef has no explicit ctors - but it's a good general style to use imho (would be happy to include this in the LLVM style guide if this seems contentious in any way & is worth some discussion/formalization - but I think it's just generally good C++ practice & hopefully can be accepted as such)

lldb/source/Target/PathMappingList.cpp

35

out of date comment - maybe this comment isn't pulling its weight? The content seems fairly simple/probably self explanatory?

[xujuntwt95329]

address dblaikie's comments

[xujuntwt95329]

@dblaikie Thanks for your detailed comments, I've uploaded the new patch-set

[wallace]

@xujuntwt95329 do you have push permissions or do you need help to land this diff?

[xujuntwt95329]

In D112863#3098706, @wallace wrote:

@xujuntwt95329 do you have push permissions or do you need help to land this diff?

I don't have push permissions, could you please help me to land this?

Thanks a lot!

[aprantl]

Thank you!

[wallace]

This comment was removed by wallace.

[xujuntwt95329]

rebase

[xujuntwt95329]

@wallace rebased

But I guess you should apply this patch firstly?
https://reviews.llvm.org/D112439

[jansvoboda11]

This breaks our build: https://green.lab.llvm.org/green/job/lldb-cmake/37529/console
Can you take a look? If the fix is not obvious, please revert for now.

[teemperor]

I think this was just fixed by 48677f58b06cfb8715902173c5bc3d1764d7c8c6

[JDevlieghere]

Thanks!

lldb/include/lldb/Target/PathMappingList.h

public:
   PathMappingList(ChangedCallback callback, void *callback_baton);
 
   PathMappingList(const PathMappingList &rhs);
 
   ~PathMappingList();
 
   const PathMappingList &operator=(const PathMappingList &rhs);
 
-  void Append(ConstString path, ConstString replacement,
-              bool notify);
+  void Append(llvm::StringRef path, llvm::StringRef replacement, bool notify);
 
   void Append(const PathMappingList &rhs, bool notify);
 
   void Clear(bool notify);
 
   // By default, dump all pairs.
   void Dump(Stream *s, int pair_index = -1);
 
   bool IsEmpty() const { return m_pairs.empty(); }
 
   size_t GetSize() const { return m_pairs.size(); }
 
   bool GetPathsAtIndex(uint32_t idx, ConstString &path,
                        ConstString &new_path) const;
 
-  void Insert(ConstString path, ConstString replacement,
+  void Insert(llvm::StringRef path, llvm::StringRef replacement,
               uint32_t insert_idx, bool notify);
 
   bool Remove(size_t index, bool notify);
 
   bool Remove(ConstString path, bool notify);
 
-  bool Replace(ConstString path, ConstString replacement,
-               bool notify);
+  bool Replace(llvm::StringRef path, llvm::StringRef replacement, bool notify);
 
-  bool Replace(ConstString path, ConstString replacement,
+  bool Replace(llvm::StringRef path, llvm::StringRef replacement,
                uint32_t index, bool notify);
   bool RemapPath(ConstString path, ConstString &new_path) const;
 
   /// Remaps a source file given \a path into \a new_path.
   ///
   /// Remaps \a path if any source remappings match. This function
   /// does NOT stat the file system so it can be used in tight loops
   /// where debug info is being parsed.
public:
   ///
   /// \param[in] orig_spec
   ///     The original source file path to try and remap.
   ///
   /// \return
   ///     The newly remapped filespec that is guaranteed to exist.
   llvm::Optional<FileSpec> FindFile(const FileSpec &orig_spec) const;
 
-  uint32_t FindIndexForPath(ConstString path) const;
+  uint32_t FindIndexForPath(llvm::StringRef path) const;
 
   uint32_t GetModificationID() const { return m_mod_id; }
 
 protected:
   typedef std::pair<ConstString, ConstString> pair;
   typedef std::vector<pair> collection;
   typedef collection::iterator iterator;
   typedef collection::const_iterator const_iterator;

lldb/source/API/SBTarget.cpp

 SBStructuredData SBTarget::GetStatistics() {
   LLDB_RECORD_METHOD_NO_ARGS(lldb::SBStructuredData, SBTarget, GetStatistics);
 
   SBStructuredData data;
   TargetSP target_sp(GetSP());
   if (!target_sp)
     return LLDB_RECORD_RESULT(data);
   std::string json_str =
-      llvm::formatv("{0:2}", 
-          DebuggerStats::ReportStatistics(target_sp->GetDebugger(), 
+      llvm::formatv("{0:2}",
+          DebuggerStats::ReportStatistics(target_sp->GetDebugger(),
                                           target_sp.get())).str();
   data.m_impl_up->SetObjectSP(StructuredData::ParseJSON(json_str));
   return LLDB_RECORD_RESULT(data);
 }
 
 void SBTarget::SetCollectingStats(bool v) {
   LLDB_RECORD_METHOD(void, SBTarget, SetCollectingStats, (bool), v);
 
void SBTarget::AppendImageSearchPath(const char *from, const char *to,
   LLDB_RECORD_METHOD(void, SBTarget, AppendImageSearchPath,
                      (const char *, const char *, lldb::SBError &), from, to,
                      error);
 
   TargetSP target_sp(GetSP());
   if (!target_sp)
     return error.SetErrorString("invalid target");
 
-  const ConstString csFrom(from), csTo(to);
-  if (!csFrom)
+  llvm::StringRef srFrom = from, srTo = to;
+  if (srFrom.empty())
     return error.SetErrorString("<from> path can't be empty");
-  if (!csTo)
+  if (srTo.empty())
     return error.SetErrorString("<to> path can't be empty");
 
-  target_sp->GetImageSearchPathList().Append(csFrom, csTo, true);
+  target_sp->GetImageSearchPathList().Append(srFrom, srTo, true);
 }
 
 lldb::SBModule SBTarget::AddModule(const char *path, const char *triple,
                                    const char *uuid_cstr) {
   LLDB_RECORD_METHOD(lldb::SBModule, SBTarget, AddModule,
                      (const char *, const char *, const char *), path, triple,
                      uuid_cstr);
 

lldb/source/Commands/CommandObjectTarget.cpp

if (argc & 1) {
           if (log) {
             LLDB_LOGF(log,
                       "target modules search path adding ImageSearchPath "
                       "pair: '%s' -> '%s'",
                       from, to);
           }
           bool last_pair = ((argc - i) == 2);
           target->GetImageSearchPathList().Append(
-              ConstString(from), ConstString(to),
-              last_pair); // Notify if this is the last pair
+              from, to, last_pair); // Notify if this is the last pair
           result.SetStatus(eReturnStatusSuccessFinishNoResult);
         } else {
           if (from[0])
             result.AppendError("<path-prefix> can't be empty\n");
           else
             result.AppendError("<new-path-prefix> can't be empty\n");
         }
       }
if (argc >= 3 && argc & 1) {
       argc = command.GetArgumentCount();
 
       for (uint32_t i = 0; i < argc; i += 2, ++insert_idx) {
         const char *from = command.GetArgumentAtIndex(i);
         const char *to = command.GetArgumentAtIndex(i + 1);
 
         if (from[0] && to[0]) {
           bool last_pair = ((argc - i) == 2);
-          target->GetImageSearchPathList().Insert(
-              ConstString(from), ConstString(to), insert_idx, last_pair);
+          target->GetImageSearchPathList().Insert(from, to, insert_idx,
+                                                  last_pair);
           result.SetStatus(eReturnStatusSuccessFinishNoResult);
         } else {
           if (from[0])
             result.AppendError("<path-prefix> can't be empty\n");
           else
             result.AppendError("<new-path-prefix> can't be empty\n");
           return false;
         }

lldb/source/Core/Module.cpp

llvm::Optional<std::string> Module::RemapSourceFile(llvm::StringRef path) const {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   if (auto remapped = m_source_mappings.RemapPath(path))
     return remapped->GetPath();
   return {};
 }
 
 void Module::RegisterXcodeSDK(llvm::StringRef sdk_name, llvm::StringRef sysroot) {
   XcodeSDK sdk(sdk_name.str());
-  ConstString sdk_path(HostInfo::GetXcodeSDKPath(sdk));
-  if (!sdk_path)
+  llvm::StringRef sdk_path(HostInfo::GetXcodeSDKPath(sdk));
+  if (sdk_path.empty())
     return;
   // If the SDK changed for a previously registered source path, update it.
   // This could happend with -fdebug-prefix-map, otherwise it's unlikely.
-  ConstString sysroot_cs(sysroot);
-  if (!m_source_mappings.Replace(sysroot_cs, sdk_path, true))
+  if (!m_source_mappings.Replace(sysroot, sdk_path, true))
     // In the general case, however, append it to the list.
-    m_source_mappings.Append(sysroot_cs, sdk_path, false);
+    m_source_mappings.Append(sysroot, sdk_path, false);
 }
 
 bool Module::MergeArchitecture(const ArchSpec &arch_spec) {
   if (!arch_spec.IsValid())
     return false;
   LLDB_LOG(GetLogIfAllCategoriesSet(LIBLLDB_LOG_OBJECT | LIBLLDB_LOG_MODULES),
            "module has arch %s, merging/replacing with arch %s",
            m_arch.GetTriple().getTriple().c_str(),

lldb/source/Core/ModuleList.cpp

FileSpecList list = m_collection_sp
                           ->GetCurrentValue();
   llvm::sys::ScopedWriter lock(m_symlink_paths_mutex);
   const bool notify = false;
   m_symlink_paths.Clear(notify);
   for (FileSpec symlink : list) {
     FileSpec resolved;
     Status status = FileSystem::Instance().Readlink(symlink, resolved);
     if (status.Success())
-      m_symlink_paths.Append(ConstString(symlink.GetPath()),
-                             ConstString(resolved.GetPath()), notify);
+      m_symlink_paths.Append(symlink.GetPath(), resolved.GetPath(), notify);
   }
 }
 
 PathMappingList ModuleListProperties::GetSymlinkMappings() const {
   llvm::sys::ScopedReader lock(m_symlink_paths_mutex);
   return m_symlink_paths;
 }
 

lldb/source/Interpreter/OptionValuePathMappings.cpp

if (argc >= 3 && (((argc - 1) & 1) == 0)) {
             "invalid file list index %s, index must be 0 through %u",
             args.GetArgumentAtIndex(0), count);
       } else {
         bool changed = false;
         for (size_t i = 1; i < argc; idx++, i += 2) {
           const char *orginal_path = args.GetArgumentAtIndex(i);
           const char *replace_path = args.GetArgumentAtIndex(i + 1);
           if (VerifyPathExists(replace_path)) {
-            ConstString a(orginal_path);
-            ConstString b(replace_path);
-            if (!m_path_mappings.Replace(a, b, idx, m_notify_changes))
-              m_path_mappings.Append(a, b, m_notify_changes);
+            if (!m_path_mappings.Replace(orginal_path, replace_path, idx,
+                                         m_notify_changes))
+              m_path_mappings.Append(orginal_path, replace_path,
+                                     m_notify_changes);
             changed = true;
           } else {
             std::string previousError =
                 error.Fail() ? std::string(error.AsCString()) + "\n" : "";
             error.SetErrorStringWithFormat(
                 "%sthe replacement path doesn't exist: \"%s\"",
                 previousError.c_str(), replace_path);
           }
if (argc < 2 || (argc & 1)) {
       error.SetErrorString("append operation takes one or more path pairs");
       break;
     } else {
       bool changed = false;
       for (size_t i = 0; i < argc; i += 2) {
         const char *orginal_path = args.GetArgumentAtIndex(i);
         const char *replace_path = args.GetArgumentAtIndex(i + 1);
         if (VerifyPathExists(replace_path)) {
-          ConstString a(orginal_path);
-          ConstString b(replace_path);
-          m_path_mappings.Append(a, b, m_notify_changes);
+          m_path_mappings.Append(orginal_path, replace_path, m_notify_changes);
           m_value_was_set = true;
           changed = true;
         } else {
           std::string previousError =
               error.Fail() ? std::string(error.AsCString()) + "\n" : "";
           error.SetErrorStringWithFormat(
               "%sthe replacement path doesn't exist: \"%s\"",
               previousError.c_str(), replace_path);
if (argc >= 3 && (((argc - 1) & 1) == 0)) {
       } else {
         bool changed = false;
         if (op == eVarSetOperationInsertAfter)
           ++idx;
         for (size_t i = 1; i < argc; i += 2) {
           const char *orginal_path = args.GetArgumentAtIndex(i);
           const char *replace_path = args.GetArgumentAtIndex(i + 1);
           if (VerifyPathExists(replace_path)) {
-            ConstString a(orginal_path);
-            ConstString b(replace_path);
-            m_path_mappings.Insert(a, b, idx, m_notify_changes);
+            m_path_mappings.Insert(orginal_path, replace_path, idx,
+                                   m_notify_changes);
             changed = true;
             idx++;
           } else {
             std::string previousError =
                 error.Fail() ? std::string(error.AsCString()) + "\n" : "";
             error.SetErrorStringWithFormat(
                 "%sthe replacement path doesn't exist: \"%s\"",
                 previousError.c_str(), replace_path);

lldb/source/Target/PathMappingList.cpp

namespace {
   // things won't always work. We found a case where if we did:
   // (lldb) settings set target.source-map . /tmp
   // We would store a path pairs of "." and "/tmp" as raw strings. If the debug
   // info contains "./foo/bar.c", the path will get normalized to "foo/bar.c".
   // When PathMappingList::RemapPath() is called, it expects the path to start
   // with the raw path pair, which doesn't work anymore because the paths have
   // been normalized when the debug info was loaded. So we need to store
   // nomalized path pairs to ensure things match up.
-  ConstString NormalizePath(ConstString path) {
-    // If we use "path" to construct a FileSpec, it will normalize the path for
-    // us. We then grab the string and turn it back into a ConstString.
-    return ConstString(FileSpec(path.GetStringRef()).GetPath());
-  }
+std::string NormalizePath(llvm::StringRef path) {
+  // us. We then grab the string.
+  return FileSpec(path).GetPath();
 }
 // PathMappingList constructor
 PathMappingList::PathMappingList() : m_pairs() {}
 
 PathMappingList::PathMappingList(ChangedCallback callback, void *callback_baton)
     : m_pairs(), m_callback(callback), m_callback_baton(callback_baton),
       m_mod_id(0) {}
 
 PathMappingList::PathMappingList(const PathMappingList &rhs)
     : m_pairs(rhs.m_pairs), m_callback(nullptr), m_callback_baton(nullptr),
       m_mod_id(0) {}
 
 const PathMappingList &PathMappingList::operator=(const PathMappingList &rhs) {
   if (this != &rhs) {
     m_pairs = rhs.m_pairs;
     m_callback = nullptr;
     m_callback_baton = nullptr;
     m_mod_id = rhs.m_mod_id;
   }
   return *this;
 }
 
 PathMappingList::~PathMappingList() = default;
 
-void PathMappingList::Append(ConstString path,
-                             ConstString replacement, bool notify) {
+void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement,
+                             bool notify) {
   ++m_mod_id;
   m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
   if (notify && m_callback)
     m_callback(*this, m_callback_baton);
 }
 
 void PathMappingList::Append(const PathMappingList &rhs, bool notify) {
   ++m_mod_id;
   if (!rhs.m_pairs.empty()) {
     const_iterator pos, end = rhs.m_pairs.end();
     for (pos = rhs.m_pairs.begin(); pos != end; ++pos)
       m_pairs.push_back(*pos);
     if (notify && m_callback)
       m_callback(*this, m_callback_baton);
   }
 }
 
-void PathMappingList::Insert(ConstString path,
-                             ConstString replacement, uint32_t index,
-                             bool notify) {
+void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement,
+                             uint32_t index, bool notify) {
   ++m_mod_id;
   iterator insert_iter;
   if (index >= m_pairs.size())
     insert_iter = m_pairs.end();
   else
     insert_iter = m_pairs.begin() + index;
   m_pairs.emplace(insert_iter, pair(NormalizePath(path),
                                     NormalizePath(replacement)));
   if (notify && m_callback)
     m_callback(*this, m_callback_baton);
 }
 
-bool PathMappingList::Replace(ConstString path,
-                              ConstString replacement, uint32_t index,
-                              bool notify) {
+bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef replacement,
+                              uint32_t index, bool notify) {
   if (index >= m_pairs.size())
     return false;
   ++m_mod_id;
   m_pairs[index] = pair(NormalizePath(path), NormalizePath(replacement));
   if (notify && m_callback)
     m_callback(*this, m_callback_baton);
   return true;
 }
bool PathMappingList::ReverseRemapPath(const FileSpec &file, FileSpec &fixed) const {
   }
   return false;
 }
 
 llvm::Optional<FileSpec> PathMappingList::FindFile(const FileSpec &orig_spec) const {
   // We must normalize the orig_spec again using the host's path style,
   // otherwise there will be mismatch between the host and remote platform
   // if they use different path styles.
-  if (auto remapped = RemapPath(
-          NormalizePath(ConstString(orig_spec.GetCString())).GetStringRef(),
+  if (auto remapped = RemapPath(NormalizePath(orig_spec.GetPath()),
                                 /*only_if_exists=*/true))
     return remapped;
 
   return {};
 }
 
-bool PathMappingList::Replace(ConstString path,
-                              ConstString new_path, bool notify) {
+bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef new_path,
+                              bool notify) {
   uint32_t idx = FindIndexForPath(path);
   if (idx < m_pairs.size()) {
     ++m_mod_id;
-    m_pairs[idx].second = new_path;
+    m_pairs[idx].second = ConstString(new_path);
     if (notify && m_callback)
       m_callback(*this, m_callback_baton);
     return true;
   }
   return false;
 }
 
 bool PathMappingList::Remove(ConstString path, bool notify) {
bool PathMappingList::GetPathsAtIndex(uint32_t idx, ConstString &path,
   if (idx < m_pairs.size()) {
     path = m_pairs[idx].first;
     new_path = m_pairs[idx].second;
     return true;
   }
   return false;
 }
 
-uint32_t PathMappingList::FindIndexForPath(ConstString orig_path) const {
-  const ConstString path = NormalizePath(orig_path);
+uint32_t PathMappingList::FindIndexForPath(llvm::StringRef orig_path) const {
+  const ConstString path = ConstString(NormalizePath(orig_path));
   const_iterator pos;
   const_iterator begin = m_pairs.begin();
   const_iterator end = m_pairs.end();
 
   for (pos = begin; pos != end; ++pos) {
     if (pos->first == path)
       return std::distance(begin, pos);
   }
   return UINT32_MAX;
 }

lldb/unittests/Target/FindFileTest.cpp

TEST_F(FindFileTest, FindFileTests) {
 
   sys::path::append(FileName, Twine(DirName), Twine("test"));
   ASSERT_NO_ERROR(openFile(FileName, fd, CD_CreateAlways, FA_Read, OF_None));
 
   llvm::FileRemover dir_remover(DirName);
   llvm::FileRemover file_remover(FileName);
   PathMappingList map;
 
-  map.Append(ConstString("/old"), ConstString(DirName.str()), false);
-  map.Append(ConstString(R"(C:\foo)"), ConstString(DirName.str()), false);
+  map.Append("/old", DirName.str(), false);
+  map.Append(R"(C:\foo)", DirName.str(), false);
 
   Matches matches[] = {
       {"/old", llvm::sys::path::Style::posix, DirName.c_str()},
       {"/old/test", llvm::sys::path::Style::posix, FileName.c_str()},
       {R"(C:\foo)", llvm::sys::path::Style::windows, DirName.c_str()},
       {R"(C:\foo\test)", llvm::sys::path::Style::windows, FileName.c_str()}};
 
   ArrayRef<FileSpec> fails{

lldb/unittests/Target/PathMappingListTest.cpp

#ifdef _WIN32
       ConstString("C:\\"),
       ConstString("C:\\a"),
 #else
       ConstString("/a"),
       ConstString("/"),
 #endif
   };
   PathMappingList map;
-  map.Append(ConstString("."), ConstString("/tmp"), false);
+  map.Append(".", "/tmp", false);
   TestPathMappings(map, matches, fails);
   PathMappingList map2;
-  map2.Append(ConstString(""), ConstString("/tmp"), false);
+  map2.Append("", "/tmp", false);
   TestPathMappings(map, matches, fails);
 }
 
 TEST(PathMappingListTest, AbsoluteTests) {
   PathMappingList map;
-  map.Append(ConstString("/old"), ConstString("/new"), false);
+  map.Append("/old", "/new", false);
   Matches matches[] = {
     {"/old", "/new"},
     {"/old/", "/new"},
     {"/old/foo/.", "/new/foo"},
     {"/old/foo.c", "/new/foo.c"},
     {"/old/foo.c/.", "/new/foo.c"},
     {"/old/./foo.c", "/new/foo.c"},
   };
   ConstString fails[] = {
     ConstString("/foo"),
     ConstString("/"),
     ConstString("foo.c"),
     ConstString("./foo.c"),
     ConstString("../foo.c"),
     ConstString("../bar/foo.c"),
   };
   TestPathMappings(map, matches, fails);
 }
 
 TEST(PathMappingListTest, RemapRoot) {
   PathMappingList map;
-  map.Append(ConstString("/"), ConstString("/new"), false);
+  map.Append("/", "/new", false);
   Matches matches[] = {
     {"/old", "/new/old"},
     {"/old/", "/new/old"},
     {"/old/foo/.", "/new/old/foo"},
     {"/old/foo.c", "/new/old/foo.c"},
     {"/old/foo.c/.", "/new/old/foo.c"},
     {"/old/./foo.c", "/new/old/foo.c"},
   };
   ConstString fails[] = {
     ConstString("foo.c"),
     ConstString("./foo.c"),
     ConstString("../foo.c"),
     ConstString("../bar/foo.c"),
   };
   TestPathMappings(map, matches, fails);
 }
 
 #ifndef _WIN32
 TEST(PathMappingListTest, CrossPlatformTests) {
   PathMappingList map;
-  map.Append(ConstString(R"(C:\old)"), ConstString("/new"), false);
+  map.Append(R"(C:\old)", "/new", false);
   Matches matches[] = {
     {R"(C:\old)", llvm::sys::path::Style::windows, "/new"},
     {R"(C:\old\)", llvm::sys::path::Style::windows, "/new"},
     {R"(C:\old\foo\.)", llvm::sys::path::Style::windows, "/new/foo"},
     {R"(C:\old\foo.c)", llvm::sys::path::Style::windows, "/new/foo.c"},
     {R"(C:\old\foo.c\.)", llvm::sys::path::Style::windows, "/new/foo.c"},
     {R"(C:\old\.\foo.c)", llvm::sys::path::Style::windows, "/new/foo.c"},
   };