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