clayborg created this revision.
clayborg added reviewers: labath, zturner, davide.
Herald added subscribers: JDevlieghere, aprantl, mgorny.
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.
https://reviews.llvm.org/D47021
Files:
include/lldb/Target/PathMappingList.h
lldb.xcodeproj/project.pbxproj
source/Target/PathMappingList.cpp
source/Target/Target.cpp
unittests/Utility/CMakeLists.txt
unittests/Utility/PathMappingListTest.cpp
Index: unittests/Utility/PathMappingListTest.cpp
===================================================================
--- unittests/Utility/PathMappingListTest.cpp
+++ unittests/Utility/PathMappingListTest.cpp
@@ -0,0 +1,101 @@
+//===-- NameMatchesTest.cpp -------------------------------------*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#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) {}
+ };
+}
+
+TEST(PathMappingListTest, RelativeTests) {
+ PathMappingList map;
+ map.Append(ConstString("."), ConstString("/tmp"), false);
+ Matches tests[] = {
+ {".", "/tmp"},
+ {"./", "/tmp"},
+ {"./////", "/tmp"},
+ {"./foo.c", "/tmp/foo.c"},
+ {"./bar/foo.c", "/tmp/bar/foo.c"}
+ };
+ ConstString actual_remapped;
+ EXPECT_FALSE(map.RemapPath(ConstString("/foo"), actual_remapped));
+ for (const auto &m: tests) {
+ FileSpec orig_spec(m.original.GetCString(), false);
+ std::string orig_normalized = orig_spec.GetPath();
+ EXPECT_TRUE(map.RemapPath(m.original, actual_remapped));
+ EXPECT_TRUE(actual_remapped == m.remapped);
+ FileSpec remapped_spec(m.remapped.GetCString(), false);
+ FileSpec unmapped_spec;
+ EXPECT_TRUE(map.ReverseRemapPath(remapped_spec, unmapped_spec));
+ std::string unmapped_path = unmapped_spec.GetPath();
+ EXPECT_TRUE(unmapped_path == orig_normalized);
+ }
+}
+
+TEST(PathMappingListTest, AbsoluteTests) {
+ PathMappingList map;
+ map.Append(ConstString("/old"), ConstString("/new"), false);
+ Matches tests[] = {
+ {"/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 actual_remapped;
+ // Make sure that the path
+ for (const auto &m: tests) {
+ FileSpec orig_spec(m.original.GetCString(), false);
+ std::string orig_normalized = orig_spec.GetPath();
+ EXPECT_TRUE(map.RemapPath(m.original, actual_remapped));
+ EXPECT_TRUE(actual_remapped == m.remapped);
+ FileSpec remapped_spec(m.remapped.GetCString(), false);
+ FileSpec unmapped_spec;
+ EXPECT_TRUE(map.ReverseRemapPath(remapped_spec, unmapped_spec));
+ std::string unmapped_path = unmapped_spec.GetPath();
+ EXPECT_TRUE(unmapped_path == orig_normalized);
+ }
+}
+
+TEST(PathMappingListTest, RemapEverything) {
+ PathMappingList map;
+ map.Append(ConstString(""), ConstString("/new"), false);
+ Matches tests[] = {
+ {"/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 actual_remapped;
+ // Make sure that the path
+ for (const auto &m: tests) {
+ FileSpec orig_spec(m.original.GetCString(), false);
+ std::string orig_normalized = orig_spec.GetPath();
+ EXPECT_TRUE(map.RemapPath(m.original, actual_remapped));
+ EXPECT_TRUE(actual_remapped == m.remapped);
+ FileSpec remapped_spec(m.remapped.GetCString(), false);
+ FileSpec unmapped_spec;
+ EXPECT_TRUE(map.ReverseRemapPath(remapped_spec, unmapped_spec));
+ std::string unmapped_path = unmapped_spec.GetPath();
+ EXPECT_TRUE(unmapped_path == orig_normalized);
+ }
+}
Index: unittests/Utility/CMakeLists.txt
===================================================================
--- unittests/Utility/CMakeLists.txt
+++ unittests/Utility/CMakeLists.txt
@@ -8,6 +8,7 @@
JSONTest.cpp
LogTest.cpp
NameMatchesTest.cpp
+ PathMappingListTest.cpp
StatusTest.cpp
StringExtractorTest.cpp
StructuredDataTest.cpp
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,26 @@
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) {
+ // Don't change nothing into ".", nothing will remap all files to the
+ // other path
+ if (!path)
+ return 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.GetCString(), false).GetPath());
+ }
+}
//----------------------------------------------------------------------
// PathMappingList constructor
//----------------------------------------------------------------------
@@ -52,7 +73,7 @@
void PathMappingList::Append(const ConstString &path,
const ConstString &replacement, bool notify) {
++m_mod_id;
- m_pairs.push_back(pair(path, replacement));
+ m_pairs.push_back(pair(NormalizePath(path), NormalizePath(replacement)));
if (notify && m_callback)
m_callback(*this, m_callback_baton);
}
@@ -77,7 +98,8 @@
insert_iter = m_pairs.end();
else
insert_iter = m_pairs.begin() + index;
- m_pairs.insert(insert_iter, pair(path, replacement));
+ m_pairs.insert(insert_iter, pair(NormalizePath(path),
+ NormalizePath(replacement)));
if (notify && m_callback)
m_callback(*this, m_callback_baton);
}
@@ -88,7 +110,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,56 +156,48 @@
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.SetCStringWithLength(remapped.c_str(), remapped.size());
+ 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()))
+
+ for (const auto &it : m_pairs) {
+ auto prefix = it.first.GetStringRef();
+ if (!path.consume_front(prefix))
continue;
-
- new_path = pos->second.GetStringRef();
- new_path.append(path);
+ FileSpec remapped(it.second.GetStringRef(), false);
+ remapped.AppendPathComponent(path);
+ new_path = std::move(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.data(), path.size());
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());
+ llvm::StringRef second(it.second.GetCString(), it.second.GetLength());
+ if (path_ref.startswith(second)) {
+ llvm::StringRef first(it.first.GetCString(), it.first.GetLength());
+ llvm::StringRef suffix = path_ref.substr(second.size());
+ if (first.empty()) {
+ // If our original path was "", then the new path is just the suffix.
+ // If we call fixed.SetFile("", false) we will end up with "." as the
+ // path and any path we appended would end up being relative.
+ fixed.SetFile(suffix, false);
+ } else {
+ fixed.SetFile(first, false);
+ fixed.AppendPathComponent(suffix);
+ }
return true;
}
}
@@ -277,7 +291,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
@@ -211,6 +211,7 @@
264A58EE1A7DBCAD00A6B1B0 /* OptionValueFormatEntity.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 264A58ED1A7DBCAD00A6B1B0 /* OptionValueFormatEntity.cpp */; };
264A97BF133918BC0017F0BE /* PlatformRemoteGDBServer.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 264A97BD133918BC0017F0BE /* PlatformRemoteGDBServer.cpp */; };
264D8D5013661BD7003A368F /* UnwindAssembly.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 264D8D4F13661BD7003A368F /* UnwindAssembly.cpp */; };
+ 264FA56720A608F900134FA2 /* PathMappingListTest.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 264FA56620A608F900134FA2 /* PathMappingListTest.cpp */; };
265192C61BA8E905002F08F6 /* CompilerDecl.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 265192C51BA8E905002F08F6 /* CompilerDecl.cpp */; };
265205A813D3E3F700132FE2 /* RegisterContextKDP_arm.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 265205A213D3E3F700132FE2 /* RegisterContextKDP_arm.cpp */; };
265205AA13D3E3F700132FE2 /* RegisterContextKDP_i386.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 265205A413D3E3F700132FE2 /* RegisterContextKDP_i386.cpp */; };
@@ -1722,6 +1723,7 @@
264AD83911095BBD00E0B039 /* CommandObjectLog.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = CommandObjectLog.h; path = source/Commands/CommandObjectLog.h; sourceTree = "<group>"; };
264D8D4E13661BCC003A368F /* UnwindAssembly.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = UnwindAssembly.h; path = include/lldb/Target/UnwindAssembly.h; sourceTree = "<group>"; };
264D8D4F13661BD7003A368F /* UnwindAssembly.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = UnwindAssembly.cpp; path = source/Target/UnwindAssembly.cpp; sourceTree = "<group>"; };
+ 264FA56620A608F900134FA2 /* PathMappingListTest.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = PathMappingListTest.cpp; sourceTree = "<group>"; };
265192C41BA8E8F8002F08F6 /* CompilerDecl.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = CompilerDecl.h; path = include/lldb/Symbol/CompilerDecl.h; sourceTree = "<group>"; };
265192C51BA8E905002F08F6 /* CompilerDecl.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = CompilerDecl.cpp; path = source/Symbol/CompilerDecl.cpp; sourceTree = "<group>"; };
265205A213D3E3F700132FE2 /* RegisterContextKDP_arm.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = RegisterContextKDP_arm.cpp; sourceTree = "<group>"; };
@@ -3473,6 +3475,7 @@
9A3D43C81F3150D200EB767C /* ConstStringTest.cpp */,
23CB14FD1D66CD2400EDDDE1 /* FileSpecTest.cpp */,
9A3D43C71F3150D200EB767C /* LogTest.cpp */,
+ 264FA56620A608F900134FA2 /* PathMappingListTest.cpp */,
9A3D43CB1F3150D200EB767C /* NameMatchesTest.cpp */,
9A3D43C61F3150D200EB767C /* StatusTest.cpp */,
9A3D43CA1F3150D200EB767C /* StructuredDataTest.cpp */,
@@ -7351,6 +7354,7 @@
23CB15341D66DA9300EDDDE1 /* CPlusPlusLanguageTest.cpp in Sources */,
9A2057381F3B8E7E00F6C293 /* FileSystemTest.cpp in Sources */,
9A2057201F3B8D2500F6C293 /* UnixSignalsTest.cpp in Sources */,
+ 264FA56720A608F900134FA2 /* 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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
