[lldb] Make semantics of SupportFile equivalence explicit #97126
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is an improved attempt to improve the semantics of SupportFile equivalence, taking into account the feedback from llvm#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.
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesThis 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. Full diff: https://github.com/llvm/llvm-project/pull/97126.diff 7 Files Affected:
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 =
|
labath
approved these changes
Jul 1, 2024
lravenclaw
pushed a commit
to lravenclaw/llvm-project
that referenced
this pull request
Jul 3, 2024
This is an improved attempt to improve the semantics of SupportFile equivalence, taking into account the feedback from llvm#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.
kbluck
pushed a commit
to kbluck/llvm-project
that referenced
this pull request
Jul 6, 2024
This is an improved attempt to improve the semantics of SupportFile equivalence, taking into account the feedback from llvm#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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.