[Lldb-commits] [lldb] [lldb] Make semantics of SupportFile equivalence explicit (PR #97126)
Jonas Devlieghere via lldb-commits
lldb-commits at lists.llvm.org
Mon Jul 1 10:08:20 PDT 2024
https://github.com/JDevlieghere updated https://github.com/llvm/llvm-project/pull/97126
>From 0c39366879c56ceb0ef6b021d8098bc73e26445b Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Fri, 28 Jun 2024 16:50:05 -0700
Subject: [PATCH 1/2] [lldb] Make semantics of SupportFile equivalence explicit
This is an improved attempt to improve the semantics of SupportFile
equivalence, taking into account the feedback from #95606.
Pavel's comment about the lack of a concise name because the concept
isn't trivial made me realize that I don't want to abstract this concept
away behind a helper function. Instead, I opted for a rather verbose
enum that forces the caller to consider exactly what kind of comparison
is appropriate for every call.
---
lldb/include/lldb/Utility/SupportFile.h | 39 +++++++++++++++----
lldb/source/Breakpoint/BreakpointResolver.cpp | 5 ++-
lldb/source/Commands/CommandObjectSource.cpp | 8 +++-
lldb/source/Symbol/LineEntry.cpp | 3 +-
lldb/source/Symbol/LineTable.cpp | 2 +-
.../source/Target/ThreadPlanStepOverRange.cpp | 15 ++++---
lldb/source/Target/ThreadPlanStepRange.cpp | 5 ++-
7 files changed, 55 insertions(+), 22 deletions(-)
diff --git a/lldb/include/lldb/Utility/SupportFile.h b/lldb/include/lldb/Utility/SupportFile.h
index 21b986dcaba28..4b672684e4b4e 100644
--- a/lldb/include/lldb/Utility/SupportFile.h
+++ b/lldb/include/lldb/Utility/SupportFile.h
@@ -11,6 +11,7 @@
#include "lldb/Utility/Checksum.h"
#include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/Flags.h"
namespace lldb_private {
@@ -30,15 +31,37 @@ class SupportFile {
virtual ~SupportFile() = default;
- /// Return true if both SupportFiles have the same FileSpec and, if both have
- /// a valid Checksum, the Checksum is the same.
- bool operator==(const SupportFile &other) const {
- if (m_checksum && other.m_checksum)
- return m_file_spec == other.m_file_spec && m_checksum == other.m_checksum;
- return m_file_spec == other.m_file_spec;
- }
+ enum SupportFileEquality : uint8_t {
+ eEqualFileSpec = (1u << 1),
+ eEqualChecksum = (1u << 2),
+ eEqualChecksumIfSet = (1u << 3),
+ eEqualFileSpecAndChecksum = eEqualFileSpec | eEqualChecksum,
+ eEqualFileSpecAndChecksumIfSet = eEqualFileSpec | eEqualChecksumIfSet,
+ };
+
+ bool Equal(const SupportFile &other,
+ SupportFileEquality equality = eEqualFileSpecAndChecksum) const {
+ assert(!(equality & eEqualChecksum & eEqualChecksumIfSet) &&
+ "eEqualChecksum and eEqualChecksumIfSet are mutually exclusive");
+
+ if (equality & eEqualFileSpec) {
+ if (m_file_spec != other.m_file_spec)
+ return false;
+ }
- bool operator!=(const SupportFile &other) const { return !(*this == other); }
+ if (equality & eEqualChecksum) {
+ if (m_checksum != other.m_checksum)
+ return false;
+ }
+
+ if (equality & eEqualChecksumIfSet) {
+ if (m_checksum && other.m_checksum)
+ if (m_checksum != other.m_checksum)
+ return false;
+ }
+
+ return true;
+ }
/// Return the file name only. Useful for resolving breakpoints by file name.
const FileSpec &GetSpecOnly() const { return m_file_spec; };
diff --git a/lldb/source/Breakpoint/BreakpointResolver.cpp b/lldb/source/Breakpoint/BreakpointResolver.cpp
index ff4e2a9985197..2d52cbf827ef9 100644
--- a/lldb/source/Breakpoint/BreakpointResolver.cpp
+++ b/lldb/source/Breakpoint/BreakpointResolver.cpp
@@ -222,8 +222,9 @@ void BreakpointResolver::SetSCMatchesByLine(
auto worklist_begin = std::partition(
all_scs.begin(), all_scs.end(), [&](const SymbolContext &sc) {
if (sc.line_entry.GetFile() == match.line_entry.GetFile() ||
- *sc.line_entry.original_file_sp ==
- *match.line_entry.original_file_sp) {
+ sc.line_entry.original_file_sp->Equal(
+ *match.line_entry.original_file_sp,
+ SupportFile::eEqualFileSpecAndChecksumIfSet)) {
// When a match is found, keep track of the smallest line number.
closest_line = std::min(closest_line, sc.line_entry.line);
return false;
diff --git a/lldb/source/Commands/CommandObjectSource.cpp b/lldb/source/Commands/CommandObjectSource.cpp
index 0c1267456a184..f54b712adfc46 100644
--- a/lldb/source/Commands/CommandObjectSource.cpp
+++ b/lldb/source/Commands/CommandObjectSource.cpp
@@ -747,13 +747,17 @@ class CommandObjectSourceList : public CommandObjectParsed {
bool operator==(const SourceInfo &rhs) const {
return function == rhs.function &&
- *line_entry.original_file_sp == *rhs.line_entry.original_file_sp &&
+ line_entry.original_file_sp->Equal(
+ *rhs.line_entry.original_file_sp,
+ SupportFile::eEqualFileSpecAndChecksumIfSet) &&
line_entry.line == rhs.line_entry.line;
}
bool operator!=(const SourceInfo &rhs) const {
return function != rhs.function ||
- *line_entry.original_file_sp != *rhs.line_entry.original_file_sp ||
+ !line_entry.original_file_sp->Equal(
+ *rhs.line_entry.original_file_sp,
+ SupportFile::eEqualFileSpecAndChecksumIfSet) ||
line_entry.line != rhs.line_entry.line;
}
diff --git a/lldb/source/Symbol/LineEntry.cpp b/lldb/source/Symbol/LineEntry.cpp
index 19e9bb561375b..c941a6927cb93 100644
--- a/lldb/source/Symbol/LineEntry.cpp
+++ b/lldb/source/Symbol/LineEntry.cpp
@@ -199,7 +199,8 @@ AddressRange LineEntry::GetSameLineContiguousAddressRange(
next_line_sc.line_entry.range.GetByteSize() == 0)
break;
- if (*original_file_sp == *next_line_sc.line_entry.original_file_sp &&
+ if (original_file_sp->Equal(*next_line_sc.line_entry.original_file_sp,
+ SupportFile::eEqualFileSpecAndChecksumIfSet) &&
(next_line_sc.line_entry.line == 0 ||
line == next_line_sc.line_entry.line)) {
// Include any line 0 entries - they indicate that this is compiler-
diff --git a/lldb/source/Symbol/LineTable.cpp b/lldb/source/Symbol/LineTable.cpp
index 06cf4f698316c..8fb002cc93171 100644
--- a/lldb/source/Symbol/LineTable.cpp
+++ b/lldb/source/Symbol/LineTable.cpp
@@ -360,7 +360,7 @@ void LineTable::Dump(Stream *s, Target *target, Address::DumpStyle style,
SupportFileSP prev_file;
for (size_t idx = 0; idx < count; ++idx) {
ConvertEntryAtIndexToLineEntry(idx, line_entry);
- line_entry.Dump(s, target, *prev_file != *line_entry.original_file_sp,
+ line_entry.Dump(s, target, !prev_file->Equal(*line_entry.original_file_sp),
style, fallback_style, show_line_ranges);
s->EOL();
prev_file = line_entry.original_file_sp;
diff --git a/lldb/source/Target/ThreadPlanStepOverRange.cpp b/lldb/source/Target/ThreadPlanStepOverRange.cpp
index 3fe02e0bf4faf..abe4d34bd32c2 100644
--- a/lldb/source/Target/ThreadPlanStepOverRange.cpp
+++ b/lldb/source/Target/ThreadPlanStepOverRange.cpp
@@ -220,8 +220,9 @@ bool ThreadPlanStepOverRange::ShouldStop(Event *event_ptr) {
StackFrameSP frame_sp = thread.GetStackFrameAtIndex(0);
sc = frame_sp->GetSymbolContext(eSymbolContextEverything);
if (sc.line_entry.IsValid()) {
- if (*sc.line_entry.original_file_sp !=
- *m_addr_context.line_entry.original_file_sp &&
+ if (!sc.line_entry.original_file_sp->Equal(
+ *m_addr_context.line_entry.original_file_sp,
+ SupportFile::eEqualFileSpecAndChecksumIfSet) &&
sc.comp_unit == m_addr_context.comp_unit &&
sc.function == m_addr_context.function) {
// Okay, find the next occurrence of this file in the line table:
@@ -244,8 +245,9 @@ bool ThreadPlanStepOverRange::ShouldStop(Event *event_ptr) {
LineEntry prev_line_entry;
if (line_table->GetLineEntryAtIndex(entry_idx - 1,
prev_line_entry) &&
- *prev_line_entry.original_file_sp ==
- *line_entry.original_file_sp) {
+ prev_line_entry.original_file_sp->Equal(
+ *line_entry.original_file_sp,
+ SupportFile::eEqualFileSpecAndChecksumIfSet)) {
SymbolContext prev_sc;
Address prev_address =
prev_line_entry.range.GetBaseAddress();
@@ -279,8 +281,9 @@ bool ThreadPlanStepOverRange::ShouldStop(Event *event_ptr) {
if (next_line_function != m_addr_context.function)
break;
- if (*next_line_entry.original_file_sp ==
- *m_addr_context.line_entry.original_file_sp) {
+ if (next_line_entry.original_file_sp->Equal(
+ *m_addr_context.line_entry.original_file_sp,
+ SupportFile::eEqualFileSpecAndChecksumIfSet)) {
const bool abort_other_plans = false;
const RunMode stop_other_threads = RunMode::eAllThreads;
lldb::addr_t cur_pc = thread.GetStackFrameAtIndex(0)
diff --git a/lldb/source/Target/ThreadPlanStepRange.cpp b/lldb/source/Target/ThreadPlanStepRange.cpp
index 998e76cb65d13..801856bd5419b 100644
--- a/lldb/source/Target/ThreadPlanStepRange.cpp
+++ b/lldb/source/Target/ThreadPlanStepRange.cpp
@@ -120,8 +120,9 @@ bool ThreadPlanStepRange::InRange() {
frame->GetSymbolContext(eSymbolContextEverything));
if (m_addr_context.line_entry.IsValid() &&
new_context.line_entry.IsValid()) {
- if (*m_addr_context.line_entry.original_file_sp ==
- *new_context.line_entry.original_file_sp) {
+ if (m_addr_context.line_entry.original_file_sp->Equal(
+ *new_context.line_entry.original_file_sp,
+ SupportFile::eEqualFileSpecAndChecksumIfSet)) {
if (m_addr_context.line_entry.line == new_context.line_entry.line) {
m_addr_context = new_context;
const bool include_inlined_functions =
>From 6aae0f0b7f78abb0d00625be6a23690fefb57ed1 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Mon, 1 Jul 2024 10:08:05 -0700
Subject: [PATCH 2/2] Add unittest
---
lldb/unittests/Utility/CMakeLists.txt | 1 +
lldb/unittests/Utility/SupportFileTest.cpp | 91 ++++++++++++++++++++++
2 files changed, 92 insertions(+)
create mode 100644 lldb/unittests/Utility/SupportFileTest.cpp
diff --git a/lldb/unittests/Utility/CMakeLists.txt b/lldb/unittests/Utility/CMakeLists.txt
index 097dae860b159..8e12815d51541 100644
--- a/lldb/unittests/Utility/CMakeLists.txt
+++ b/lldb/unittests/Utility/CMakeLists.txt
@@ -36,6 +36,7 @@ add_lldb_unittest(UtilityTests
StringListTest.cpp
StructuredDataTest.cpp
SubsystemRAIITest.cpp
+ SupportFileTest.cpp
TildeExpressionResolverTest.cpp
TimeoutTest.cpp
TimerTest.cpp
diff --git a/lldb/unittests/Utility/SupportFileTest.cpp b/lldb/unittests/Utility/SupportFileTest.cpp
new file mode 100644
index 0000000000000..44fbfe0f57254
--- /dev/null
+++ b/lldb/unittests/Utility/SupportFileTest.cpp
@@ -0,0 +1,91 @@
+//===-- SupportFileTest.cpp
+//--------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/Checksum.h"
+#include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/SupportFile.h"
+
+using namespace lldb_private;
+
+static llvm::MD5::MD5Result hash1 = {
+ {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}};
+
+static llvm::MD5::MD5Result hash2 = {
+ {8, 9, 10, 11, 12, 13, 14, 15, 0, 1, 2, 3, 4, 5, 6, 7}};
+
+TEST(SupportFileTest, TestDefaultConstructor) {
+ SupportFile support_file;
+
+ EXPECT_EQ(support_file.GetSpecOnly(), FileSpec());
+ EXPECT_EQ(support_file.GetChecksum(), Checksum());
+}
+
+TEST(SupportFileTest, TestConstructor) {
+ FileSpec file_spec("/foo/bar");
+ Checksum checksum(hash1);
+ SupportFile support_file(file_spec, checksum);
+
+ EXPECT_EQ(support_file.GetSpecOnly(), file_spec);
+ EXPECT_EQ(support_file.GetChecksum(), checksum);
+}
+
+TEST(SupportFileTest, TestEqual) {
+ auto EQ = [&](const SupportFile &LHS, const SupportFile &RHS,
+ SupportFile::SupportFileEquality equality) -> bool {
+ EXPECT_EQ(LHS.Equal(RHS, equality), RHS.Equal(LHS, equality));
+ return LHS.Equal(RHS, equality);
+ };
+
+ FileSpec foo_bar("/foo/bar");
+ Checksum checksum_foo_bar(hash1);
+
+ FileSpec bar_baz("/bar/baz");
+ Checksum checksum_bar_baz(hash2);
+
+ // The canonical support file we're comparing against.
+ SupportFile support_file(foo_bar, checksum_foo_bar);
+
+ // Support file A is identical.
+ SupportFile support_file_a(foo_bar, checksum_foo_bar);
+ EXPECT_TRUE(EQ(support_file, support_file_a, SupportFile::eEqualFileSpec));
+ EXPECT_TRUE(EQ(support_file, support_file_a, SupportFile::eEqualChecksum));
+ EXPECT_TRUE(
+ EQ(support_file, support_file_a, SupportFile::eEqualFileSpecAndChecksum));
+ EXPECT_TRUE(EQ(support_file, support_file_a,
+ SupportFile::eEqualFileSpecAndChecksumIfSet));
+
+ // Support file C is has the same path but no checksum.
+ SupportFile support_file_b(foo_bar);
+ EXPECT_TRUE(EQ(support_file, support_file_b, SupportFile::eEqualFileSpec));
+ EXPECT_FALSE(EQ(support_file, support_file_b, SupportFile::eEqualChecksum));
+ EXPECT_FALSE(
+ EQ(support_file, support_file_b, SupportFile::eEqualFileSpecAndChecksum));
+ EXPECT_TRUE(EQ(support_file, support_file_b,
+ SupportFile::eEqualFileSpecAndChecksumIfSet));
+
+ // Support file D has a different path and checksum.
+ SupportFile support_file_c(bar_baz, checksum_bar_baz);
+ EXPECT_FALSE(EQ(support_file, support_file_c, SupportFile::eEqualFileSpec));
+ EXPECT_FALSE(EQ(support_file, support_file_c,
+ SupportFile::eEqualFileSpecAndChecksumIfSet));
+ EXPECT_FALSE(EQ(support_file, support_file_c, SupportFile::eEqualChecksum));
+ EXPECT_FALSE(
+ EQ(support_file, support_file_c, SupportFile::eEqualFileSpecAndChecksum));
+
+ // Support file E has a different path but the same checksum.
+ SupportFile support_file_d(bar_baz, checksum_foo_bar);
+ EXPECT_FALSE(EQ(support_file, support_file_d, SupportFile::eEqualFileSpec));
+ EXPECT_FALSE(EQ(support_file, support_file_d,
+ SupportFile::eEqualFileSpecAndChecksumIfSet));
+ EXPECT_TRUE(EQ(support_file, support_file_d, SupportFile::eEqualChecksum));
+ EXPECT_FALSE(
+ EQ(support_file, support_file_d, SupportFile::eEqualFileSpecAndChecksum));
+}
More information about the lldb-commits
mailing list