Author: Jonas Devlieghere Date: 2024-07-01T12:54:35-07:00 New Revision: dd5df27d9c6b47793b72d4c8f2a796e5d8dc343d
URL: https://github.com/llvm/llvm-project/commit/dd5df27d9c6b47793b72d4c8f2a796e5d8dc343d DIFF: https://github.com/llvm/llvm-project/commit/dd5df27d9c6b47793b72d4c8f2a796e5d8dc343d.diff LOG: [lldb] Make semantics of SupportFile equivalence explicit (#97126) 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. Added: lldb/unittests/Utility/SupportFileTest.cpp Modified: lldb/include/lldb/Utility/SupportFile.h lldb/source/Breakpoint/BreakpointResolver.cpp lldb/source/Commands/CommandObjectSource.cpp lldb/source/Symbol/LineEntry.cpp lldb/source/Symbol/LineTable.cpp lldb/source/Target/ThreadPlanStepOverRange.cpp lldb/source/Target/ThreadPlanStepRange.cpp lldb/unittests/Utility/CMakeLists.txt Removed: ################################################################################ diff --git a/lldb/include/lldb/Utility/SupportFile.h b/lldb/include/lldb/Utility/SupportFile.h index 21b986dcaba28..334a0aaac2c27 100644 --- a/lldb/include/lldb/Utility/SupportFile.h +++ b/lldb/include/lldb/Utility/SupportFile.h @@ -30,15 +30,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 = 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 diff erent 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 diff erent 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)); +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
