Skip to content

Commit 7f6f55f

Browse files
royitaqiroyitaqi
committed
Addressed all comments, EXCEPT the lack of a unit test containing both realpath and source-map
1 parent 6011b7e commit 7f6f55f

File tree

12 files changed

+129
-98
lines changed

12 files changed

+129
-98
lines changed

‎lldb/include/lldb/Symbol/CompileUnit.h

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -396,11 +396,10 @@ class CompileUnit : public std::enable_shared_from_this<CompileUnit>,
396396
/// realpath'ed to resolve any symlinks.
397397
///
398398
/// \see enum SymbolContext::Scope
399-
void
400-
ResolveSymbolContext(const SourceLocationSpec &src_location_spec,
401-
lldb::SymbolContextItem resolve_scope,
402-
SymbolContextList &sc_list,
403-
const RealpathPrefixes *realpath_prefixes = nullptr);
399+
void ResolveSymbolContext(const SourceLocationSpec &src_location_spec,
400+
lldb::SymbolContextItem resolve_scope,
401+
SymbolContextList &sc_list,
402+
RealpathPrefixes *realpath_prefixes = nullptr);
404403

405404
/// Get whether compiler optimizations were enabled for this compile unit
406405
///

‎lldb/include/lldb/Target/Statistics.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ struct StatisticsOptions {
176176
};
177177

178178
/// A class that represents statistics for a since lldb_private::Target.
179-
class TargetStats : public RealpathPrefixesStats {
179+
class TargetStats {
180180
public:
181181
llvm::json::Value ToJSON(Target &target,
182182
const lldb_private::StatisticsOptions &options);
@@ -185,8 +185,8 @@ class TargetStats : public RealpathPrefixesStats {
185185
void SetFirstPrivateStopTime();
186186
void SetFirstPublicStopTime();
187187
void IncreaseSourceMapDeduceCount();
188-
virtual void IncreaseSourceRealpathAttemptCount() override;
189-
virtual void IncreaseSourceRealpathCompatibleCount() override;
188+
void IncreaseSourceRealpathAttemptCount(uint32_t count);
189+
void IncreaseSourceRealpathCompatibleCount(uint32_t count);
190190

191191
StatsDuration &GetCreateTime() { return m_create_time; }
192192
StatsSuccessFail &GetExpressionStats() { return m_expr_eval; }

‎lldb/include/lldb/Utility/FileSpecList.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,9 @@ class SupportFileList {
7171
/// \return
7272
/// The index of the file that matches \a file if it is found,
7373
/// else UINT32_MAX is returned.
74-
size_t FindCompatibleIndex(
75-
size_t idx, const FileSpec &file,
76-
const RealpathPrefixes *realpath_prefixes = nullptr) const;
74+
size_t
75+
FindCompatibleIndex(size_t idx, const FileSpec &file,
76+
RealpathPrefixes *realpath_prefixes = nullptr) const;
7777

7878
template <class... Args> void EmplaceBack(Args &&...args) {
7979
m_files.push_back(

‎lldb/include/lldb/Utility/RealpathPrefixes.h

Lines changed: 31 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -21,21 +21,36 @@ namespace lldb_private {
2121

2222
class RealpathPrefixes {
2323
public:
24-
// Prefixes are obtained from FileSpecList, through FileSpec::GetPath(), which
25-
// ensures that the paths are normalized. For example:
26-
// "./foo/.." -> ""
27-
// "./foo/../bar" -> "bar"
28-
explicit RealpathPrefixes(const FileSpecList &file_spec_list);
24+
/// \param[in] file_spec_list
25+
/// Prefixes are obtained from FileSpecList, through FileSpec::GetPath(),
26+
/// which ensures that the paths are normalized. For example:
27+
/// "./foo/.." -> ""
28+
/// "./foo/../bar" -> "bar"
29+
///
30+
/// \param[in] fs
31+
/// An optional filesystem to use for realpath'ing. If not set, the real
32+
/// filesystem will be used.
33+
explicit RealpathPrefixes(const FileSpecList &file_spec_list,
34+
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs =
35+
llvm::vfs::getRealFileSystem());
2936

30-
// Sets an optional filesystem to use for realpath'ing. If not set, the real
31-
// filesystem will be used.
32-
void SetFileSystem(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs);
37+
std::optional<FileSpec> ResolveSymlinks(const FileSpec &file_spec);
3338

34-
// Sets an optional Target instance to gather statistics.
35-
void SetTarget(const lldb::TargetSP& target) { m_target = target; }
36-
lldb::TargetSP GetTarget() const { return m_target.lock(); }
37-
38-
std::optional<FileSpec> ResolveSymlinks(const FileSpec &file_spec) const;
39+
// If/when Statistics.h/cpp is moved into Utility, we can remove these
40+
// methods, hold a (weak) pointer to `TargetStats` and directly increment
41+
// on that object.
42+
void IncreaseSourceRealpathAttemptCount() {
43+
++m_source_realpath_attempt_count;
44+
}
45+
uint32_t GetSourceRealpathAttemptCount() const {
46+
return m_source_realpath_attempt_count;
47+
};
48+
void IncreaseSourceRealpathCompatibleCount() {
49+
++m_source_realpath_compatible_count;
50+
}
51+
uint32_t GetSourceRealpathCompatibleCount() const {
52+
return m_source_realpath_compatible_count;
53+
}
3954

4055
private:
4156
// Paths that start with one of the prefixes in this list will be realpath'ed
@@ -51,14 +66,10 @@ class RealpathPrefixes {
5166

5267
// The optional Target instance to gather statistics.
5368
lldb::TargetWP m_target;
54-
};
55-
56-
class RealpathPrefixesStats {
57-
public:
58-
virtual ~RealpathPrefixesStats() = default;
5969

60-
virtual void IncreaseSourceRealpathAttemptCount() = 0;
61-
virtual void IncreaseSourceRealpathCompatibleCount() = 0;
70+
// Statistics that we temprarily hold here, to be gathered into TargetStats
71+
uint32_t m_source_realpath_attempt_count = 0;
72+
uint32_t m_source_realpath_compatible_count = 0;
6273
};
6374

6475
} // namespace lldb_private

‎lldb/source/Breakpoint/BreakpointResolverFileLine.cpp

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -291,8 +291,8 @@ Searcher::CallbackReturn BreakpointResolverFileLine::SearchCallback(
291291
const uint32_t line = m_location_spec.GetLine().value_or(0);
292292
const std::optional<uint16_t> column = m_location_spec.GetColumn();
293293

294-
RealpathPrefixes realpath_prefixes =
295-
GetBreakpoint()->GetTarget().GetSourceRealpathPrefixes();
294+
Target &target = GetBreakpoint()->GetTarget();
295+
RealpathPrefixes realpath_prefixes = target.GetSourceRealpathPrefixes();
296296

297297
const size_t num_comp_units = context.module_sp->GetNumCompileUnits();
298298
for (size_t i = 0; i < num_comp_units; i++) {
@@ -304,6 +304,12 @@ Searcher::CallbackReturn BreakpointResolverFileLine::SearchCallback(
304304
}
305305
}
306306

307+
// Gather stats into the Target
308+
target.GetStatistics().IncreaseSourceRealpathAttemptCount(
309+
realpath_prefixes.GetSourceRealpathAttemptCount());
310+
target.GetStatistics().IncreaseSourceRealpathCompatibleCount(
311+
realpath_prefixes.GetSourceRealpathCompatibleCount());
312+
307313
FilterContexts(sc_list);
308314

309315
DeduceSourceMapping(sc_list);

‎lldb/source/Symbol/CompileUnit.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ VariableListSP CompileUnit::GetVariableList(bool can_create) {
215215

216216
std::vector<uint32_t>
217217
FindFileIndexes(const SupportFileList &files, const FileSpec &file,
218-
const RealpathPrefixes *realpath_prefixes = nullptr) {
218+
RealpathPrefixes *realpath_prefixes = nullptr) {
219219
std::vector<uint32_t> result;
220220
uint32_t idx = -1;
221221
while ((idx = files.FindCompatibleIndex(idx + 1, file, realpath_prefixes)) !=
@@ -249,7 +249,7 @@ uint32_t CompileUnit::FindLineEntry(uint32_t start_idx, uint32_t line,
249249
void CompileUnit::ResolveSymbolContext(
250250
const SourceLocationSpec &src_location_spec,
251251
SymbolContextItem resolve_scope, SymbolContextList &sc_list,
252-
const RealpathPrefixes *realpath_prefixes) {
252+
RealpathPrefixes *realpath_prefixes) {
253253
const FileSpec file_spec = src_location_spec.GetFileSpec();
254254
const uint32_t line = src_location_spec.GetLine().value_or(0);
255255
const bool check_inlines = src_location_spec.GetCheckInlines();

‎lldb/source/Target/Statistics.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -224,12 +224,12 @@ void TargetStats::IncreaseSourceMapDeduceCount() {
224224
++m_source_map_deduce_count;
225225
}
226226

227-
void TargetStats::IncreaseSourceRealpathAttemptCount() {
228-
++m_source_realpath_attempt_count;
227+
void TargetStats::IncreaseSourceRealpathAttemptCount(uint32_t count) {
228+
m_source_realpath_attempt_count += count;
229229
}
230230

231-
void TargetStats::IncreaseSourceRealpathCompatibleCount() {
232-
++m_source_realpath_compatible_count;
231+
void TargetStats::IncreaseSourceRealpathCompatibleCount(uint32_t count) {
232+
m_source_realpath_compatible_count += count;
233233
}
234234

235235
bool DebuggerStats::g_collecting_stats = false;

‎lldb/source/Target/Target.cpp

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4359,9 +4359,7 @@ InlineStrategy TargetProperties::GetInlineStrategy() const {
43594359
// this because we want the FileSpecList to normalize the file paths for us.
43604360
RealpathPrefixes TargetProperties::GetSourceRealpathPrefixes() const {
43614361
const uint32_t idx = ePropertySourceRealpathPrefixes;
4362-
auto prefixes = RealpathPrefixes(GetPropertyAtIndexAs<FileSpecList>(idx, {}));
4363-
prefixes.SetTarget(this->m_target->shared_from_this());
4364-
return prefixes;
4362+
return RealpathPrefixes(GetPropertyAtIndexAs<FileSpecList>(idx, {}));
43654363
}
43664364

43674365
llvm::StringRef TargetProperties::GetArg0() const {

‎lldb/source/Utility/FileSpecList.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ IsCompatibleResult IsCompatible(const FileSpec &curr_file,
165165

166166
size_t SupportFileList::FindCompatibleIndex(
167167
size_t start_idx, const FileSpec &file_spec,
168-
const RealpathPrefixes *realpath_prefixes) const {
168+
RealpathPrefixes *realpath_prefixes) const {
169169
const size_t num_files = m_files.size();
170170
if (start_idx >= num_files)
171171
return UINT32_MAX;
@@ -180,10 +180,10 @@ size_t SupportFileList::FindCompatibleIndex(
180180
if (realpath_prefixes && result == IsCompatibleResult::kOnlyFileMatch) {
181181
if (std::optional<FileSpec> resolved_curr_file =
182182
realpath_prefixes->ResolveSymlinks(curr_file)) {
183-
if (IsCompatible(*resolved_curr_file, file_spec)) {
183+
if (IsCompatible(*resolved_curr_file, file_spec) ==
184+
IsCompatibleResult::kBothDirectoryAndFileMatch) {
184185
// Stats and logging.
185-
if (lldb::TargetSP target = realpath_prefixes->GetTarget())
186-
target->GetStatistics().IncreaseSourceRealpathCompatibleCount();
186+
realpath_prefixes->IncreaseSourceRealpathCompatibleCount();
187187
Log *log = GetLog(LLDBLog::Source);
188188
LLDB_LOGF(log,
189189
"Realpath'ed support file %s is compatible to input file",

‎lldb/source/Utility/RealpathPrefixes.cpp

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -8,43 +8,47 @@
88

99
#include "lldb/Utility/RealpathPrefixes.h"
1010

11-
#include "lldb/Target/Statistics.h"
12-
#include "lldb/Target/Target.h"
1311
#include "lldb/Utility/FileSpec.h"
1412
#include "lldb/Utility/FileSpecList.h"
1513
#include "lldb/Utility/LLDBLog.h"
1614
#include "lldb/Utility/Log.h"
1715

1816
using namespace lldb_private;
1917

20-
RealpathPrefixes::RealpathPrefixes(const FileSpecList &file_spec_list)
21-
: m_fs(llvm::vfs::getRealFileSystem()) {
18+
RealpathPrefixes::RealpathPrefixes(
19+
const FileSpecList &file_spec_list,
20+
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs)
21+
: m_fs(fs) {
2222
m_prefixes.reserve(file_spec_list.GetSize());
2323
for (const FileSpec &file_spec : file_spec_list) {
2424
m_prefixes.emplace_back(file_spec.GetPath());
2525
}
2626
}
2727

28-
void RealpathPrefixes::SetFileSystem(
29-
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs) {
30-
m_fs = fs;
31-
}
32-
3328
std::optional<FileSpec>
34-
RealpathPrefixes::ResolveSymlinks(const FileSpec &file_spec) const {
29+
RealpathPrefixes::ResolveSymlinks(const FileSpec &file_spec) {
3530
if (m_prefixes.empty())
3631
return std::nullopt;
3732

38-
auto is_prefix = [](llvm::StringRef a, llvm::StringRef b,
39-
bool case_sensitive) -> bool {
40-
return case_sensitive ? a.consume_front(b) : a.consume_front_insensitive(b);
33+
// Test if `b` is a *path* prefix of `a` (not just *string* prefix).
34+
// E.g. "/foo/bar" is a path prefix of "/foo/bar/baz" but not "/foo/barbaz".
35+
auto is_path_prefix = [](llvm::StringRef a, llvm::StringRef b,
36+
bool case_sensitive,
37+
llvm::sys::path::Style style) -> bool {
38+
if (case_sensitive ? a.consume_front(b) : a.consume_front_insensitive(b))
39+
// If `b` isn't "/", then it won't end with "/" because it comes from
40+
// `FileSpec`. After `a` consumes `b`, `a` should either be empty (i.e.
41+
// `a` == `b`) or end with "/" (the remainder of `a` is a subdirectory).
42+
return b == "/" || a.empty() ||
43+
llvm::sys::path::is_separator(a[0], style);
44+
return false;
4145
};
4246
std::string file_spec_path = file_spec.GetPath();
4347
for (const std::string &prefix : m_prefixes) {
44-
if (is_prefix(file_spec_path, prefix, file_spec.IsCaseSensitive())) {
48+
if (is_path_prefix(file_spec_path, prefix, file_spec.IsCaseSensitive(),
49+
file_spec.GetPathStyle())) {
4550
// Stats and logging.
46-
if (lldb::TargetSP target = m_target.lock())
47-
target->GetStatistics().IncreaseSourceRealpathAttemptCount();
51+
IncreaseSourceRealpathAttemptCount();
4852
Log *log = GetLog(LLDBLog::Source);
4953
LLDB_LOGF(log, "Realpath'ing support file %s", file_spec_path.c_str());
5054

‎lldb/unittests/Utility/FileSpecListTest.cpp

Lines changed: 42 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -136,12 +136,11 @@ TEST(SupportFileListTest, SymlinkedAbsolutePaths) {
136136
FileSpec("/symlink_dir/foo.h"), FileSpec("/real_dir/foo.h")));
137137

138138
// Prepare RealpathPrefixes
139-
FileSpecList temp;
140-
temp.EmplaceBack("/symlink_dir");
141-
RealpathPrefixes prefixes(std::move(temp));
142-
prefixes.SetFileSystem(fs);
139+
FileSpecList file_spec_list;
140+
file_spec_list.EmplaceBack("/symlink_dir");
141+
RealpathPrefixes prefixes(file_spec_list, fs);
143142

144-
// Prepare FileSpecList
143+
// Prepare support file list
145144
SupportFileList support_file_list;
146145
support_file_list.EmplaceBack(FileSpec("/symlink_dir/foo.h"));
147146

@@ -161,12 +160,11 @@ TEST(SupportFileListTest, SymlinkedRelativePaths) {
161160
FileSpec("symlink_dir/foo.h"), FileSpec("real_dir/foo.h")));
162161

163162
// Prepare RealpathPrefixes
164-
FileSpecList temp;
165-
temp.EmplaceBack("symlink_dir");
166-
RealpathPrefixes prefixes(std::move(temp));
167-
prefixes.SetFileSystem(fs);
163+
FileSpecList file_spec_list;
164+
file_spec_list.EmplaceBack("symlink_dir");
165+
RealpathPrefixes prefixes(file_spec_list, fs);
168166

169-
// Prepare FileSpecList
167+
// Prepare support file list
170168
SupportFileList support_file_list;
171169
support_file_list.EmplaceBack(FileSpec("symlink_dir/foo.h"));
172170

@@ -176,6 +174,30 @@ TEST(SupportFileListTest, SymlinkedRelativePaths) {
176174
EXPECT_EQ(ret, (size_t)0);
177175
}
178176

177+
// Support file is a symlink to the breakpoint file.
178+
// A matching prefix is set.
179+
// Input file only match basename and not directory.
180+
// Should find the two incompatible.
181+
TEST(SupportFileListTest, RealpathOnlyMatchFileName) {
182+
// Prepare FS
183+
llvm::IntrusiveRefCntPtr<MockSymlinkFileSystem> fs(new MockSymlinkFileSystem(
184+
FileSpec("symlink_dir/foo.h"), FileSpec("real_dir/foo.h")));
185+
186+
// Prepare RealpathPrefixes
187+
FileSpecList file_spec_list;
188+
file_spec_list.EmplaceBack("symlink_dir");
189+
RealpathPrefixes prefixes(file_spec_list, fs);
190+
191+
// Prepare support file list
192+
SupportFileList support_file_list;
193+
support_file_list.EmplaceBack(FileSpec("symlink_dir/foo.h"));
194+
195+
// Test
196+
size_t ret = support_file_list.FindCompatibleIndex(
197+
0, FileSpec("some_other_dir/foo.h"), &prefixes);
198+
EXPECT_EQ(ret, UINT32_MAX);
199+
}
200+
179201
// Support file is a symlink to the breakpoint file.
180202
// A matching prefix is set.
181203
// However, the breakpoint is set with a partial path.
@@ -186,12 +208,11 @@ TEST(SupportFileListTest, PartialBreakpointPath) {
186208
FileSpec("symlink_dir/foo.h"), FileSpec("/real_dir/foo.h")));
187209

188210
// Prepare RealpathPrefixes
189-
FileSpecList temp;
190-
temp.EmplaceBack("symlink_dir");
191-
RealpathPrefixes prefixes(std::move(temp));
192-
prefixes.SetFileSystem(fs);
211+
FileSpecList file_spec_list;
212+
file_spec_list.EmplaceBack("symlink_dir");
213+
RealpathPrefixes prefixes(file_spec_list, fs);
193214

194-
// Prepare FileSpecList
215+
// Prepare support file list
195216
SupportFileList support_file_list;
196217
support_file_list.EmplaceBack(FileSpec("symlink_dir/foo.h"));
197218

@@ -211,12 +232,11 @@ TEST(SupportFileListTest, DifferentBasename) {
211232
FileSpec("/symlink_dir/foo.h"), FileSpec("/real_dir/bar.h")));
212233

213234
// Prepare RealpathPrefixes
214-
FileSpecList temp;
215-
temp.EmplaceBack("/symlink_dir");
216-
RealpathPrefixes prefixes(std::move(temp));
217-
prefixes.SetFileSystem(fs);
235+
FileSpecList file_spec_list;
236+
file_spec_list.EmplaceBack("/symlink_dir");
237+
RealpathPrefixes prefixes(file_spec_list, fs);
218238

219-
// Prepare FileSpecList
239+
// Prepare support file list
220240
SupportFileList support_file_list;
221241
support_file_list.EmplaceBack(FileSpec("/symlink_dir/foo.h"));
222242

@@ -230,7 +250,7 @@ TEST(SupportFileListTest, DifferentBasename) {
230250
// The support file and the breakpoint file are different.
231251
// Should find the two incompatible.
232252
TEST(SupportFileListTest, NoPrefixes) {
233-
// Prepare FileSpecList
253+
// Prepare support file list
234254
SupportFileList support_file_list;
235255
support_file_list.EmplaceBack(FileSpec("/real_dir/bar.h"));
236256

@@ -244,7 +264,7 @@ TEST(SupportFileListTest, NoPrefixes) {
244264
// The support file and the breakpoint file are the same.
245265
// Should find the two compatible.
246266
TEST(SupportFileListTest, SameFile) {
247-
// Prepare FileSpecList
267+
// Prepare support file list
248268
SupportFileList support_file_list;
249269
support_file_list.EmplaceBack(FileSpec("/real_dir/foo.h"));
250270

0 commit comments

Comments
 (0)