[Lldb-commits] [lldb] r332842 - Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes
Greg Clayton via lldb-commits
lldb-commits at lists.llvm.org
Mon May 21 07:14:36 PDT 2018
Author: gclayton
Date: Mon May 21 07:14:36 2018
New Revision: 332842
URL: http://llvm.org/viewvc/llvm-project?rev=332842&view=rev
Log:
Fix PathMappingList for relative and empty paths after recent FileSpec normalization changes
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.
Added tests for absolute, relative and empty paths.
Differential Revision: https://reviews.llvm.org/D47021
Added:
lldb/trunk/unittests/Target/PathMappingListTest.cpp
Modified:
lldb/trunk/include/lldb/Target/PathMappingList.h
lldb/trunk/lldb.xcodeproj/project.pbxproj
lldb/trunk/source/Target/PathMappingList.cpp
lldb/trunk/source/Target/Target.cpp
lldb/trunk/source/Utility/FileSpec.cpp
lldb/trunk/unittests/Target/CMakeLists.txt
lldb/trunk/unittests/Utility/FileSpecTest.cpp
Modified: lldb/trunk/include/lldb/Target/PathMappingList.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/PathMappingList.h?rev=332842&r1=332841&r2=332842&view=diff
==============================================================================
--- lldb/trunk/include/lldb/Target/PathMappingList.h (original)
+++ lldb/trunk/include/lldb/Target/PathMappingList.h Mon May 21 07:14:36 2018
@@ -90,7 +90,7 @@ public:
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.
Modified: lldb/trunk/lldb.xcodeproj/project.pbxproj
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/lldb.xcodeproj/project.pbxproj?rev=332842&r1=332841&r2=332842&view=diff
==============================================================================
--- lldb/trunk/lldb.xcodeproj/project.pbxproj (original)
+++ lldb/trunk/lldb.xcodeproj/project.pbxproj Mon May 21 07:14:36 2018
@@ -267,6 +267,7 @@
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 */; };
@@ -1755,6 +1756,7 @@
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; };
@@ -6709,6 +6711,7 @@
children = (
9A2057061F3B818600F6C293 /* MemoryRegionInfoTest.cpp */,
AFAFD8091E57E1B90017A14F /* ModuleCacheTest.cpp */,
+ 2668A2ED20AF417D00D94111 /* PathMappingListTest.cpp */,
);
name = Target;
sourceTree = "<group>";
@@ -7351,6 +7354,7 @@
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 */,
Modified: lldb/trunk/source/Target/PathMappingList.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/PathMappingList.cpp?rev=332842&r1=332841&r2=332842&view=diff
==============================================================================
--- lldb/trunk/source/Target/PathMappingList.cpp (original)
+++ lldb/trunk/source/Target/PathMappingList.cpp Mon May 21 07:14:36 2018
@@ -14,6 +14,7 @@
// 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"
@@ -23,6 +24,22 @@
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
//----------------------------------------------------------------------
@@ -52,7 +69,7 @@ PathMappingList::~PathMappingList() = de
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);
}
@@ -77,7 +94,8 @@ void PathMappingList::Insert(const Const
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);
}
@@ -88,7 +106,7 @@ bool PathMappingList::Replace(const Cons
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;
@@ -134,22 +152,10 @@ void PathMappingList::Clear(bool notify)
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 true;
}
return false;
}
@@ -158,34 +164,41 @@ bool PathMappingList::RemapPath(llvm::St
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()))
- continue;
-
- new_path = pos->second.GetStringRef();
- new_path.append(path);
+ 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;
+ // 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 true;
}
return false;
}
@@ -277,7 +290,8 @@ bool PathMappingList::GetPathsAtIndex(ui
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();
Modified: lldb/trunk/source/Target/Target.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/Target.cpp?rev=332842&r1=332841&r2=332842&view=diff
==============================================================================
--- lldb/trunk/source/Target/Target.cpp (original)
+++ lldb/trunk/source/Target/Target.cpp Mon May 21 07:14:36 2018
@@ -328,11 +328,7 @@ BreakpointSP Target::CreateBreakpoint(co
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) {
Modified: lldb/trunk/source/Utility/FileSpec.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/FileSpec.cpp?rev=332842&r1=332841&r2=332842&view=diff
==============================================================================
--- lldb/trunk/source/Utility/FileSpec.cpp (original)
+++ lldb/trunk/source/Utility/FileSpec.cpp Mon May 21 07:14:36 2018
@@ -67,6 +67,31 @@ void Denormalize(llvm::SmallVectorImpl<c
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) {
@@ -814,31 +839,10 @@ bool FileSpec::IsSourceImplementationFil
}
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(); }
Modified: lldb/trunk/unittests/Target/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Target/CMakeLists.txt?rev=332842&r1=332841&r2=332842&view=diff
==============================================================================
--- lldb/trunk/unittests/Target/CMakeLists.txt (original)
+++ lldb/trunk/unittests/Target/CMakeLists.txt Mon May 21 07:14:36 2018
@@ -1,6 +1,7 @@
add_lldb_unittest(TargetTests
MemoryRegionInfoTest.cpp
ModuleCacheTest.cpp
+ PathMappingListTest.cpp
LINK_LIBS
lldbCore
Added: lldb/trunk/unittests/Target/PathMappingListTest.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Target/PathMappingListTest.cpp?rev=332842&view=auto
==============================================================================
--- lldb/trunk/unittests/Target/PathMappingListTest.cpp (added)
+++ lldb/trunk/unittests/Target/PathMappingListTest.cpp Mon May 21 07:14:36 2018
@@ -0,0 +1,109 @@
+//===-- 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);
+}
Modified: lldb/trunk/unittests/Utility/FileSpecTest.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Utility/FileSpecTest.cpp?rev=332842&r1=332841&r2=332842&view=diff
==============================================================================
--- lldb/trunk/unittests/Utility/FileSpecTest.cpp (original)
+++ lldb/trunk/unittests/Utility/FileSpecTest.cpp Mon May 21 07:14:36 2018
@@ -273,3 +273,50 @@ TEST(FileSpecTest, FormatFileSpec) {
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());
+ }
+}
+
More information about the lldb-commits
mailing list