clayborg updated this revision to Diff 147587.
clayborg added a comment.
- 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
https://reviews.llvm.org/D47021
Files:
include/lldb/Target/PathMappingList.h
lldb.xcodeproj/project.pbxproj
source/Target/PathMappingList.cpp
source/Target/Target.cpp
source/Utility/FileSpec.cpp
unittests/Target/CMakeLists.txt
unittests/Target/PathMappingListTest.cpp
unittests/Utility/FileSpecTest.cpp
Index: unittests/Utility/FileSpecTest.cpp
===================================================================
--- unittests/Utility/FileSpecTest.cpp
+++ unittests/Utility/FileSpecTest.cpp
@@ -273,3 +273,50 @@
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());
+ }
+}
+
Index: unittests/Target/PathMappingListTest.cpp
===================================================================
--- unittests/Target/PathMappingListTest.cpp
+++ unittests/Target/PathMappingListTest.cpp
@@ -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);
+}
Index: unittests/Target/CMakeLists.txt
===================================================================
--- unittests/Target/CMakeLists.txt
+++ unittests/Target/CMakeLists.txt
@@ -1,6 +1,7 @@
add_lldb_unittest(TargetTests
MemoryRegionInfoTest.cpp
ModuleCacheTest.cpp
+ PathMappingListTest.cpp
LINK_LIBS
lldbCore
Index: source/Utility/FileSpec.cpp
===================================================================
--- source/Utility/FileSpec.cpp
+++ source/Utility/FileSpec.cpp
@@ -67,6 +67,31 @@
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::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(); }
Index: source/Target/Target.cpp
===================================================================
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -328,11 +328,7 @@
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) {
Index: source/Target/PathMappingList.cpp
===================================================================
--- source/Target/PathMappingList.cpp
+++ source/Target/PathMappingList.cpp
@@ -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 @@
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 @@
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 @@
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,58 +152,53 @@
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;
}
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()))
- 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 @@
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();
Index: lldb.xcodeproj/project.pbxproj
===================================================================
--- lldb.xcodeproj/project.pbxproj
+++ lldb.xcodeproj/project.pbxproj
@@ -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 */,
Index: include/lldb/Target/PathMappingList.h
===================================================================
--- include/lldb/Target/PathMappingList.h
+++ include/lldb/Target/PathMappingList.h
@@ -90,7 +90,7 @@
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.
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
