[lldb] Make semantics of SupportFile equivalence explicit (llvm#97126)

JDevlieghere · web-flow · commit dd5df27d9c6b · 2024-07-01T12:54:35.000-07:00
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.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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));
+}
