Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes

[clayborg]

Summary

PathMappingList was broken for relative and empty paths after normalization changes in FileSpec. There were also no tests for PathMappingList so I added those.

Changes include:

• Change PathMappingList::ReverseRemapPath() to take FileSpec objects instead of ConstString. The only client of this was doing work to convert to and from ConstString objects for no reason.
• Normalize all paths prefix and replacements that are added to the PathMappingList vector so they match the paths that have been already normalized in the debug info
• Unify code in the two forms of PathMappingList::RemapPath() so only one contains the actual functionality. Prior to this, there were two versions of this code.
• Use FileSpec::AppendPathComponent() and remove a long standing TODO so paths are correctly appended to each other.
• Correctly handle the case where someone maps "" to something else. This allows all paths to be prefixed with the replacement.
• Added tests for absolute, relative and empty paths.

[zturner]

source/Target/PathMappingList.cpp

76 ↗

(On Diff #147347)

Slightly more idiomatic to say emplace_back(NormalizePath(path), NormalizePath(replacement));

178 ↗

(On Diff #147347)

I don't think the std::move is necessary here. Since the result of GetPath() is already a temporary (rvalue), the move assignment operator will already be invoked even without std::move.

186 ↗

(On Diff #147347)

llvm::StringRef path_ref(path); should be fine, no need to explicitly specify the pointer and size.

188 ↗

(On Diff #147347)

How about llvm::StringRef second = it.second.GetStringRef();

189 ↗

(On Diff #147347)

Can you invert this conditional and continue; if it's false?

189 ↗

(On Diff #147347)

It looks like after this check, the first second.size() characters of path_ref are ignored. So it sounds like you can write this as:

if (!path_ref.consume_front(second))
  continue;

Then further down you can delete the line that calls substr.

190 ↗

(On Diff #147347)

llvm::StringRef first = it.first.GetStringRef();

[clayborg]

Fix issues found by Zach.

[clayborg]

Marked things as done.

[labath]

Although it may not seem that way from the number of comments, the change looks good to me. The main thing is the moving of the test file, as that will fail in the cmake build. And it also looks like some code can be simplified if my assumption about not converting "" to "." is true.

Thank you for adding the test for this class.

source/Target/PathMappingList.cpp

44 ↗

(On Diff #147354)

s/GetCString/GetStringRef

101 ↗

(On Diff #147354)

s/insert/emplace

161 ↗

(On Diff #147354)

SetString(remapped)

191–194 ↗

(On Diff #147354)

Is this really true? Judging by the test you've had to remove in r332633, we shouldn't end up converting "" to ".".

unittests/Utility/PathMappingListTest.cpp

1 ↗

(On Diff #147354)

This file should go to unittests/Target/PathMappingListTest.cpp to match where the class-under-test lives. (Also, please update the name in the header.)

39 ↗

(On Diff #147354)

s/GetCString/GetStringRef (whole file)

42 ↗

(On Diff #147354)

It's better to use EXPECT_EQ(expected, actual) for equality comparison, as that will print out a more useful error message when things fail.

86 ↗

(On Diff #147354)

How does this work for relative paths? I take it foo.c should be remapped to /new/foo.c ? Can you add a test for that?

[clayborg]

I will make the fixes and also test out mapping "" to "." as suggested.

source/Target/PathMappingList.cpp

194 ↗

(On Diff #147354)

When I thought about it, I chose to not convert to "." for path remapping. I think people would expect if the remap "" to "/prefix" that "/prefix" would be prepended to each path no matter what it is. Of course users could just specify "/" if they wish for a prefix. I could see this going either way. Let me know what you think.

unittests/Utility/PathMappingListTest.cpp

86 ↗

(On Diff #147354)

It doesn't work. If "foo.c" would map to "/new/foo.c", we can unmap it correctly since it would unmap to "/foo.c". Both "foo.c" and "/foo.c" would map to to "/new/foo.c" and then we can only unmap the latter correctly.

This might bode well for saying that "" should map to "." actually. Then we won't run into this situation. I will test things out with "" mapping to "." and see how things go.

[clayborg]

• Fixed Pavel's issues
• If user specifies "" as the first directory in PathMappingList, it will match "."
• User can specify "/" as the first directory to remap all absolute paths
• Fixed FileSpec::IsAbsolute() and added tests for issues I discovered when adding tests for relative paths in PathMappingListTests
• Modified tests in PathMappingListTests to use a help function which simplifies code
• Added more tests to things that shouldn't get remapped

[labath]

looks good. I don't know if anyone else has an opinion on how should we treat "" for mapping purposes, but treating it as "." seems fine to me.

unittests/Utility/FileSpecTest.cpp

300 ↗

(On Diff #147587)

In tests like these, it's good to add something like SCOPED_TRACE(path) or similar. That way, when a test fails you will see which path caused the failure instead of just a generic "spec.IsRelative() is true" message.

lldb/trunk/include/lldb/Target/PathMappingList.h

public:
   ///
   /// @return
   ///     /b true if \a path was successfully located and \a new_path
   ///     is filled in with a new source path, \b false otherwise.
   //------------------------------------------------------------------
   bool RemapPath(llvm::StringRef path, std::string &new_path) const;
   bool RemapPath(const char *, std::string &) const = delete;
 
-  bool ReverseRemapPath(const ConstString &path, ConstString &new_path) const;
+  bool ReverseRemapPath(const FileSpec &file, FileSpec &fixed) const;
 
   //------------------------------------------------------------------
   /// Finds a source file given a file spec using the path remappings.
   ///
   /// Tries to resolve \a orig_spec by checking the path remappings.
   /// It makes sure the file exists by checking with the file system,
   /// so this call can be expensive if the remappings are on a network
   /// or are even on the local file system, so use this function

lldb/trunk/lldb.xcodeproj/project.pbxproj

/* Begin PBXBuildFile section */
     26680330116005E7008E1FE4 /* SBDebugger.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 9A9830FA1125FC5800A56CB0 /* SBDebugger.cpp */; };
     26680331116005E9008E1FE4 /* SBCommunication.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 260223E8115F06E500A601A2 /* SBCommunication.cpp */; };
     26680332116005EA008E1FE4 /* SBCommandReturnObject.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 9A9830F81125FC5800A56CB0 /* SBCommandReturnObject.cpp */; };
     26680333116005EC008E1FE4 /* SBCommandInterpreter.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 9A9830F61125FC5800A56CB0 /* SBCommandInterpreter.cpp */; };
     26680335116005EE008E1FE4 /* SBBroadcaster.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 9A9830F21125FC5800A56CB0 /* SBBroadcaster.cpp */; };
     26680336116005EF008E1FE4 /* SBBreakpointLocation.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 9AF16CC7114086A1007A7B3F /* SBBreakpointLocation.cpp */; };
     26680337116005F1008E1FE4 /* SBBreakpoint.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 9AF16A9C11402D5B007A7B3F /* SBBreakpoint.cpp */; };
     2668035C11601108008E1FE4 /* LLDB.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 26680207115FD0ED008E1FE4 /* LLDB.framework */; };
+    2668A2EE20AF417D00D94111 /* PathMappingListTest.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 2668A2ED20AF417D00D94111 /* PathMappingListTest.cpp */; };
     266942001A6DC2AC0063BE93 /* MICmdArgContext.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 266941601A6DC2AB0063BE93 /* MICmdArgContext.cpp */; };
     266942011A6DC2AC0063BE93 /* MICmdArgSet.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 266941621A6DC2AB0063BE93 /* MICmdArgSet.cpp */; };
     266942021A6DC2AC0063BE93 /* MICmdArgValBase.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 266941641A6DC2AB0063BE93 /* MICmdArgValBase.cpp */; };
     266942031A6DC2AC0063BE93 /* MICmdArgValConsume.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 266941661A6DC2AB0063BE93 /* MICmdArgValConsume.cpp */; };
     266942041A6DC2AC0063BE93 /* MICmdArgValFile.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 266941681A6DC2AB0063BE93 /* MICmdArgValFile.cpp */; };
     266942051A6DC2AC0063BE93 /* MICmdArgValListBase.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 2669416A1A6DC2AC0063BE93 /* MICmdArgValListBase.cpp */; };
     266942061A6DC2AC0063BE93 /* MICmdArgValListOfN.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 2669416C1A6DC2AC0063BE93 /* MICmdArgValListOfN.cpp */; };
     266942071A6DC2AC0063BE93 /* MICmdArgValNumber.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 2669416E1A6DC2AC0063BE93 /* MICmdArgValNumber.cpp */; };
/* Begin PBXFileReference section */
     26651A15133BF9CC005B64B7 /* Opcode.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = Opcode.h; path = include/lldb/Core/Opcode.h; sourceTree = "<group>"; };
     26651A17133BF9DF005B64B7 /* Opcode.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = Opcode.cpp; path = source/Core/Opcode.cpp; sourceTree = "<group>"; };
     2665CD0D15080846002C8FAE /* Makefile */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.make; path = Makefile; sourceTree = "<group>"; };
     2666ADC11B3CB675001FAFD3 /* DynamicLoaderHexagonDYLD.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = DynamicLoaderHexagonDYLD.cpp; sourceTree = "<group>"; };
     2666ADC21B3CB675001FAFD3 /* DynamicLoaderHexagonDYLD.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = DynamicLoaderHexagonDYLD.h; sourceTree = "<group>"; };
     2666ADC31B3CB675001FAFD3 /* HexagonDYLDRendezvous.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = HexagonDYLDRendezvous.cpp; sourceTree = "<group>"; };
     2666ADC41B3CB675001FAFD3 /* HexagonDYLDRendezvous.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = HexagonDYLDRendezvous.h; sourceTree = "<group>"; };
     26680207115FD0ED008E1FE4 /* LLDB.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = LLDB.framework; sourceTree = BUILT_PRODUCTS_DIR; };
+    2668A2ED20AF417D00D94111 /* PathMappingListTest.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = PathMappingListTest.cpp; path = Target/PathMappingListTest.cpp; sourceTree = "<group>"; };
     2669415B1A6DC2AB0063BE93 /* CMakeLists.txt */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; name = CMakeLists.txt; path = "tools/lldb-mi/CMakeLists.txt"; sourceTree = SOURCE_ROOT; };
     2669415E1A6DC2AB0063BE93 /* lldb-Info.plist */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.plist.xml; name = "lldb-Info.plist"; path = "tools/lldb-mi/lldb-Info.plist"; sourceTree = SOURCE_ROOT; };
     2669415F1A6DC2AB0063BE93 /* Makefile */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.make; name = Makefile; path = "tools/lldb-mi/Makefile"; sourceTree = SOURCE_ROOT; };
     266941601A6DC2AB0063BE93 /* MICmdArgContext.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = MICmdArgContext.cpp; path = "tools/lldb-mi/MICmdArgContext.cpp"; sourceTree = SOURCE_ROOT; };
     266941611A6DC2AB0063BE93 /* MICmdArgContext.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = MICmdArgContext.h; path = "tools/lldb-mi/MICmdArgContext.h"; sourceTree = SOURCE_ROOT; };
     266941621A6DC2AB0063BE93 /* MICmdArgSet.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = MICmdArgSet.cpp; path = "tools/lldb-mi/MICmdArgSet.cpp"; sourceTree = SOURCE_ROOT; };
     266941631A6DC2AB0063BE93 /* MICmdArgSet.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = MICmdArgSet.h; path = "tools/lldb-mi/MICmdArgSet.h"; sourceTree = SOURCE_ROOT; };
     266941641A6DC2AB0063BE93 /* MICmdArgValBase.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = MICmdArgValBase.cpp; path = "tools/lldb-mi/MICmdArgValBase.cpp"; sourceTree = SOURCE_ROOT; };
AF77E08C1A033C4B0096C0EA /* SysV-ppc64 */ = {
       name = "SysV-ppc64";
       sourceTree = "<group>";
     };
     AFAFD8081E57E19E0017A14F /* Target */ = {
       isa = PBXGroup;
       children = (
         9A2057061F3B818600F6C293 /* MemoryRegionInfoTest.cpp */,
         AFAFD8091E57E1B90017A14F /* ModuleCacheTest.cpp */,
+        2668A2ED20AF417D00D94111 /* PathMappingListTest.cpp */,
       );
       name = Target;
       sourceTree = "<group>";
     };
     AFDBC36A2046638D00B9C8F2 /* PPC64 */ = {
       isa = PBXGroup;
       children = (
         AFDBC36C204663AF00B9C8F2 /* EmulateInstructionPPC64.cpp */,
23CB15321D66DA9300EDDDE1 /* Sources */ = {
       buildActionMask = 2147483647;
       files = (
         9A3D43D91F3151C400EB767C /* StatusTest.cpp in Sources */,
         23CB15331D66DA9300EDDDE1 /* GoParserTest.cpp in Sources */,
         9A2057281F3B8DDB00F6C293 /* TestELFHeader.cpp in Sources */,
         23CB15341D66DA9300EDDDE1 /* CPlusPlusLanguageTest.cpp in Sources */,
         9A2057381F3B8E7E00F6C293 /* FileSystemTest.cpp in Sources */,
         9A2057201F3B8D2500F6C293 /* UnixSignalsTest.cpp in Sources */,
+        2668A2EE20AF417D00D94111 /* PathMappingListTest.cpp in Sources */,
         AFAFD80A1E57E1B90017A14F /* ModuleCacheTest.cpp in Sources */,
         9A3D43DA1F3151C400EB767C /* StructuredDataTest.cpp in Sources */,
         9A2057121F3B824B00F6C293 /* SymbolFileDWARFTests.cpp in Sources */,
         9A20573A1F3B8E7E00F6C293 /* MainLoopTest.cpp in Sources */,
         23CB15351D66DA9300EDDDE1 /* UriParserTest.cpp in Sources */,
         8C3BD9A01EF5D1FF0016C343 /* JSONTest.cpp in Sources */,
         23CB15361D66DA9300EDDDE1 /* FileSpecTest.cpp in Sources */,
         23E2E5251D90373D006F38BB /* ArchSpecTest.cpp in Sources */,

lldb/trunk/source/Target/PathMappingList.cpp

 //===-- PathMappingList.cpp -------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 // C Includes
 // C++ Includes
 #include <climits>
 #include <cstring>
 
 // Other libraries and framework includes
 // Project includes
+#include "lldb/lldb-private-enumerations.h"
 #include "lldb/Host/PosixApi.h"
 #include "lldb/Target/PathMappingList.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/Stream.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
+  // We must normalize our path pairs that we store because if we don't then
+  // 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(const 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(), false).GetPath());
+  }
+}
 //----------------------------------------------------------------------
 // PathMappingList constructor
 //----------------------------------------------------------------------
 PathMappingList::PathMappingList()
     : m_pairs(), m_callback(nullptr), m_callback_baton(nullptr), m_mod_id(0) {}
 
 PathMappingList::PathMappingList(ChangedCallback callback, void *callback_baton)
     : m_pairs(), m_callback(callback), m_callback_baton(callback_baton),
const PathMappingList &PathMappingList::operator=(const PathMappingList &rhs) {
   return *this;
 }
 
 PathMappingList::~PathMappingList() = default;
 
 void PathMappingList::Append(const ConstString &path,
                              const ConstString &replacement, bool notify) {
   ++m_mod_id;
-  m_pairs.push_back(pair(path, replacement));
+  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(const ConstString &path,
                              const ConstString &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.insert(insert_iter, pair(path, replacement));
+  m_pairs.emplace(insert_iter, pair(NormalizePath(path),
+                                    NormalizePath(replacement)));
   if (notify && m_callback)
     m_callback(*this, m_callback_baton);
 }
 
 bool PathMappingList::Replace(const ConstString &path,
                               const ConstString &replacement, uint32_t index,
                               bool notify) {
   if (index >= m_pairs.size())
     return false;
   ++m_mod_id;
-  m_pairs[index] = pair(path, replacement);
+  m_pairs[index] = pair(NormalizePath(path), NormalizePath(replacement));
   if (notify && m_callback)
     m_callback(*this, m_callback_baton);
   return true;
 }
 
 bool PathMappingList::Remove(size_t index, bool notify) {
   if (index >= m_pairs.size())
     return false;
if (!m_pairs.empty())
     ++m_mod_id;
   m_pairs.clear();
   if (notify && m_callback)
     m_callback(*this, m_callback_baton);
 }
 
 bool PathMappingList::RemapPath(const ConstString &path,
                                 ConstString &new_path) const {
-  const char *path_cstr = path.GetCString();
-  // CLEANUP: Convert this function to use StringRefs internally instead
-  // of raw c-strings.
-  if (!path_cstr)
-    return false;
-
-  const_iterator pos, end = m_pairs.end();
-  for (pos = m_pairs.begin(); pos != end; ++pos) {
-    const size_t prefixLen = pos->first.GetLength();
-
-    if (::strncmp(pos->first.GetCString(), path_cstr, prefixLen) == 0) {
-      std::string new_path_str(pos->second.GetCString());
-      new_path_str.append(path.GetCString() + prefixLen);
-      new_path.SetCString(new_path_str.c_str());
-      return true;
-    }
-  }
+  std::string remapped;
+  if (RemapPath(path.GetStringRef(), remapped)) {
+    new_path.SetString(remapped);
   return false;
 }
 
 bool PathMappingList::RemapPath(llvm::StringRef path,
                                 std::string &new_path) const {
   if (m_pairs.empty() || path.empty())
     return false;
-
-  const_iterator pos, end = m_pairs.end();
-  for (pos = m_pairs.begin(); pos != end; ++pos) {
-    if (!path.consume_front(pos->first.GetStringRef()))
+  LazyBool path_is_relative = eLazyBoolCalculate;
+  for (const auto &it : m_pairs) {
+    auto prefix = it.first.GetStringRef();
+    if (!path.consume_front(prefix)) {
+      // Relative paths won't have a leading "./" in them unless "." is the
+      // only thing in the relative path so we need to work around "."
+      // carefully.
+      if (prefix != ".")
         continue;
-
-    new_path = pos->second.GetStringRef();
-    new_path.append(path);
+      // We need to figure out if the "path" argument is relative. If it is,
+      // then we should remap, else skip this entry.
+      if (path_is_relative == eLazyBoolCalculate) {
+        path_is_relative = FileSpec(path, false).IsRelative() ? eLazyBoolYes :
+        eLazyBoolNo;
+      }
+      if (!path_is_relative)
+        continue;
+    }
+    FileSpec remapped(it.second.GetStringRef(), false);
+    remapped.AppendPathComponent(path);
+    new_path = remapped.GetPath();
     return true;
   }
   return false;
 }
 
-bool PathMappingList::ReverseRemapPath(const ConstString &path,
-                                       ConstString &new_path) const {
-  const char *path_cstr = path.GetCString();
-  if (!path_cstr)
-    return false;
-
+bool PathMappingList::ReverseRemapPath(const FileSpec &file, FileSpec &fixed) const {
+  std::string path = file.GetPath();
+  llvm::StringRef path_ref(path);
   for (const auto &it : m_pairs) {
-    // FIXME: This should be using FileSpec API's to do the path appending.
-    const size_t prefixLen = it.second.GetLength();
-    if (::strncmp(it.second.GetCString(), path_cstr, prefixLen) == 0) {
-      std::string new_path_str(it.first.GetCString());
-      new_path_str.append(path.GetCString() + prefixLen);
-      new_path.SetCString(new_path_str.c_str());
-      return true;
-    }
-  }
+    if (!path_ref.consume_front(it.second.GetStringRef()))
+      continue;
+    fixed.SetFile(it.first.GetStringRef(), false);
+    fixed.AppendPathComponent(path_ref);
   return false;
 }
 
 bool PathMappingList::FindFile(const FileSpec &orig_spec,
                                FileSpec &new_spec) const {
   if (!m_pairs.empty()) {
     char orig_path[PATH_MAX];
     const size_t orig_path_len =
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(const ConstString &path) const {
+uint32_t PathMappingList::FindIndexForPath(const ConstString &orig_path) const {
+  const ConstString path = 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/trunk/source/Target/Target.cpp

 BreakpointSP Target::CreateBreakpoint(const FileSpecList *containingModules,
                                       const FileSpec &file, uint32_t line_no,
                                       lldb::addr_t offset,
                                       LazyBool check_inlines,
                                       LazyBool skip_prologue, bool internal,
                                       bool hardware,
                                       LazyBool move_to_nearest_code) {
   FileSpec remapped_file;
-  ConstString remapped_path;
-  if (GetSourcePathMap().ReverseRemapPath(ConstString(file.GetPath().c_str()),
-                                          remapped_path))
-    remapped_file.SetFile(remapped_path.AsCString(), false);
-  else
+  if (!GetSourcePathMap().ReverseRemapPath(file, remapped_file))
     remapped_file = file;
 
   if (check_inlines == eLazyBoolCalculate) {
     const InlineStrategy inline_strategy = GetInlineStrategy();
     switch (inline_strategy) {
     case eInlineBreakpointsNever:
       check_inlines = eLazyBoolNo;
       break;

lldb/trunk/source/Utility/FileSpec.cpp

 }
 
 void Denormalize(llvm::SmallVectorImpl<char> &path, FileSpec::Style style) {
   if (PathStyleIsPosix(style))
     return;
 
   std::replace(path.begin(), path.end(), '/', '\\');
 }
+  
+bool PathIsRelative(llvm::StringRef path, FileSpec::Style style) {
+  
+  if (path.empty())
+    return false;
+
+  if (PathStyleIsPosix(style)) {
+    // If the path doesn't start with '/' or '~', return true
+    switch (path[0]) {
+      case '/':
+      case '~':
+        return false;
+      default:
+        return true;
+    }
+  } else {
+    if (path.size() >= 2 && path[1] == ':')
+      return false;
+    if (path[0] == '/')
+      return false;
+    return true;
+  }
+  return false;
+}
+
 } // end anonymous namespace
 
 void FileSpec::Resolve(llvm::SmallVectorImpl<char> &path) {
   if (path.empty())
     return;
 
   llvm::SmallString<32> Source(path.begin(), path.end());
   StandardTildeExpressionResolver Resolver;
static RegularExpression g_source_file_regex(llvm::StringRef(
       "^([cC]|[mM]|[mM][mM]|[cC][pP][pP]|[cC]\\+\\+|[cC][xX][xX]|[cC][cC]|["
       "cC][pP]|[sS]|[aA][sS][mM]|[fF]|[fF]77|[fF]90|[fF]95|[fF]03|[fF][oO]["
       "rR]|[fF][tT][nN]|[fF][pP][pP]|[aA][dD][aA]|[aA][dD][bB]|[aA][dD][sS])"
       "$"));
   return g_source_file_regex.Execute(extension.GetStringRef());
 }
 
 bool FileSpec::IsRelative() const {
-  const char *dir = m_directory.GetCString();
-  llvm::StringRef directory(dir ? dir : "");
-
-  if (directory.size() > 0) {
-    if (PathStyleIsPosix(m_style)) {
-      // If the path doesn't start with '/' or '~', return true
-      switch (directory[0]) {
-      case '/':
-      case '~':
-        return false;
-      default:
-        return true;
-      }
-    } else {
-      if (directory.size() >= 2 && directory[1] == ':')
-        return false;
-      if (directory[0] == '/')
-        return false;
-      return true;
-    }
-  } else if (m_filename) {
-    // No directory, just a basename, return true
-    return true;
-  }
-  return false;
+  if (m_directory)
+    return PathIsRelative(m_directory.GetStringRef(), m_style);
+  else
+    return PathIsRelative(m_filename.GetStringRef(), m_style);
 }
 
 bool FileSpec::IsAbsolute() const { return !FileSpec::IsRelative(); }
 
 void llvm::format_provider<FileSpec>::format(const FileSpec &F,
                                              raw_ostream &Stream,
                                              StringRef Style) {
   assert(

lldb/trunk/unittests/Target/CMakeLists.txt

 add_lldb_unittest(TargetTests
   MemoryRegionInfoTest.cpp
   ModuleCacheTest.cpp
+  PathMappingListTest.cpp
 
   LINK_LIBS
       lldbCore
       lldbHost
       lldbSymbol
       lldbUtility
       lldbPluginObjectFileELF
       lldbUtilityHelpers
     LINK_COMPONENTS
       Support
   )
 
 add_unittest_inputs(TargetTests TestModule.so)

lldb/trunk/unittests/Target/PathMappingListTest.cpp

+//===-- PathMappingListTest.cpp ---------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ADT/ArrayRef.h"
+#include "lldb/Target/PathMappingList.h"
+#include "lldb/Utility/FileSpec.h"
+#include "gtest/gtest.h"
+#include <utility>
+
+using namespace lldb_private;
+
+namespace {
+  struct Matches {
+    ConstString original;
+    ConstString remapped;
+    Matches(const char *o, const char *r) : original(o),
+        remapped(r) {}
+  };
+  
+  void TestPathMappings(const PathMappingList &map,
+                        llvm::ArrayRef<Matches> matches,
+                        llvm::ArrayRef<ConstString> fails) {
+    ConstString actual_remapped;
+    for (const auto &fail: fails) {
+      EXPECT_FALSE(map.RemapPath(fail, actual_remapped));
+    }
+    for (const auto &match: matches) {
+      FileSpec orig_spec(match.original.GetStringRef(), false);
+      std::string orig_normalized = orig_spec.GetPath();
+      EXPECT_TRUE(map.RemapPath(match.original, actual_remapped));
+      EXPECT_EQ(actual_remapped, match.remapped);
+      FileSpec remapped_spec(match.remapped.GetStringRef(), false);
+      FileSpec unmapped_spec;
+      EXPECT_TRUE(map.ReverseRemapPath(remapped_spec, unmapped_spec));
+      std::string unmapped_path = unmapped_spec.GetPath();
+      EXPECT_EQ(unmapped_path, orig_normalized);
+    }
+  }
+}
+
+TEST(PathMappingListTest, RelativeTests) {
+  Matches matches[] = {
+    {".", "/tmp"},
+    {"./", "/tmp"},
+    {"./////", "/tmp"},
+    {"./foo.c", "/tmp/foo.c"},
+    {"foo.c", "/tmp/foo.c"},
+    {"./bar/foo.c", "/tmp/bar/foo.c"},
+    {"bar/foo.c", "/tmp/bar/foo.c"},
+  };
+  ConstString fails[] = {
+    ConstString("/a"),
+    ConstString("/"),
+  };
+  PathMappingList map;
+  map.Append(ConstString("."), ConstString("/tmp"), false);
+  TestPathMappings(map, matches, fails);
+  PathMappingList map2;
+  map2.Append(ConstString(""), ConstString("/tmp"), false);
+  TestPathMappings(map, matches, fails);
+}
+
+TEST(PathMappingListTest, AbsoluteTests) {
+  PathMappingList map;
+  map.Append(ConstString("/old"), ConstString("/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);
+  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);
+}

lldb/trunk/unittests/Utility/FileSpecTest.cpp

TEST(FileSpecTest, FormatFileSpec) {
   EXPECT_EQ("bar.txt", llvm::formatv("{0:F}", F).str());
 
   F = FileSpec("foo", false, win);
   EXPECT_EQ("foo", llvm::formatv("{0}", F).str());
   EXPECT_EQ("foo", llvm::formatv("{0:F}", F).str());
   EXPECT_EQ("(empty)", llvm::formatv("{0:D}", F).str());
 }
 
+TEST(FileSpecTest, IsRelative) {
+  llvm::StringRef not_relative[] = {
+    "/",
+    "/a",
+    "/a/",
+    "/a/b",
+    "/a/b/",
+    "//",
+    "//a",
+    "//a/",
+    "//a/b",
+    "//a/b/",
+    "~",
+    "~/",
+    "~/a",
+    "~/a/",
+    "~/a/b"
+    "~/a/b/",
+    "/foo/.",
+    "/foo/..",
+    "/foo/../",
+    "/foo/../.",
+  };
+  for (const auto &path: not_relative) {
+    FileSpec spec(path, false, FileSpec::Style::posix);
+    EXPECT_FALSE(spec.IsRelative());
+  }
+  llvm::StringRef is_relative[] = {
+    ".",
+    "./",
+    ".///",
+    "a",
+    "./a",
+    "./a/",
+    "./a/",
+    "./a/b",
+    "./a/b/",
+    "../foo",
+    "foo/bar.c",
+    "./foo/bar.c"
+  };
+  for (const auto &path: is_relative) {
+    FileSpec spec(path, false, FileSpec::Style::posix);
+    EXPECT_TRUE(spec.IsRelative());
+  }
+}
+