[lldb] Fix the semantics of SupportFile equivalence

[JDevlieghere]

Currently, two SupportFiles with the same FileSpec are considered different if one of them has a Checksum and the other doesn't. However, this is overly strict. It's totally valid to mix LineTables that do and do not contain Checksums. This patch makes it so that the Checksum is only compared if both SupportFiles have a valid Checksum.

[JDevlieghere]

Credit to @jasonmolenda for uncovering this bug.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

Currently, two SupportFiles with the same FileSpec are considered different if one of them has a Checksum and the other doesn't. However, this is overly strict. It's totally valid to mix LineTables that do and do not contain Checksums. This patch makes it so that the Checksum is only compared if both SupportFiles have a valid Checksum.

---

Full diff: https://github.com/llvm/llvm-project/pull/95606.diff

1 Files Affected:

• (modified) lldb/include/lldb/Utility/SupportFile.h (+5-1)

diff --git a/lldb/include/lldb/Utility/SupportFile.h b/lldb/include/lldb/Utility/SupportFile.h
index 7505d7f345c5d..d65156cea768f 100644
--- a/lldb/include/lldb/Utility/SupportFile.h
+++ b/lldb/include/lldb/Utility/SupportFile.h
@@ -30,8 +30,12 @@ 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 {
-    return m_file_spec == other.m_file_spec && m_checksum == other.m_checksum;
+    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;
   }
 
   bool operator!=(const SupportFile &other) const { return !(*this == other); }

[jimingham]

The previous check does seem overly restrictive.

LGTM

[jasonmolenda]

Thanks for coming up with the right fix, this looks good to me.

[JDevlieghere]

Thanks for the overwhelming support :-)

[labath]

Thanks for the overwhelming support :-)

I hate to ruin a party, but I don't think this is a good use of operator==, precisely because it does not define an equivalence relation (it's not transitive). Might I suggest named function instead? IsCompatible ?

[jasonmolenda]

Thanks for the overwhelming support :-)

I hate to ruin a party, but I don't think this is a good use of operator==, precisely because it does not define an equivalence relation (it's not transitive). Might I suggest named function instead? IsCompatible ?

The place I hit a problem with this was in ThreadPlanStepRange::InRange where we're doing

    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.line == new_context.line_entry.line) {
          m_addr_context = new_context;
          const bool include_inlined_functions =
              GetKind() == eKindStepOverRange;
...

and only one of the SupportFile's here had a checksum, so the == test failed (and an inlined stepping bug came up)

If I read m_addr_context.line_entry.original_file_sp->IsCompatible(new_context.line_entry.original_file_sp) I would think "Oh is this considering maybe a remapped filename?" or something, I'm not sure I would see that as "the same file, ignoring checksum if only one has it".

There are probbly some other uses of this operator== and maybe I shouldn't focus on this one, but I'm trying to think of how the intention could be clearly expressed with a method name here.

[jasonmolenda]

I hate to ruin a party, but I don't think this is a good use of operator==, precisely because it does not define an equivalence relation (it's not transitive). Might I suggest named function instead? IsCompatible ?

I'd be fine with SupportFile::IsSameFile(SupportFile&), I think that would make it clear what is being evaluated in caller locations. But is it any different than operator==, which you'd expect to behave the same?

[labath]

I think that operator== should represent the strictest possible way to compare two objects, and ideally mean "the two objects are interchangable". Interchangeability implies transitivity, and that doesn't hold here because an object with no checksum can match two other objects with different checksums, but those two objects won't match each other directly.

This can have implications for this bug as well. I don't know how we can end up in a different line table here, but if that can happen twice, and the middle line table has no checksum, then it can make a difference in whether you compare to the previous line table, or the initial/starting one. (I don't think this is likely to happen, I'm just using it to illustrate the point.)

With that in mind, I think IsSameFile is still slightly better than operator== (because it's harder to use it by accident, so you're more likely to look up the definition and see the comment next to it -- which hopefully explains the subtlety), but it still implies some form of interchangability. And fundamentally, I think it's not correct -- without the checksum (and maybe even with it) we can't definitively say whether the two objects describe the same file. (we may decide we're going to treat them as if they were, but I think that's something different)

Unfortunately, I don't know of a concise way to express this difference, maybe because the concept is not concise to begin with. IsProbablySameFile, MayDescribeSameFile, PotentiallySameFile ?

[jasonmolenda]

Unfortunately, I don't know of a concise way to express this difference, maybe because the concept is not concise to begin with. IsProbablySameFile, MayDescribeSameFile, PotentiallySameFile ?

I was thinking about this too, and the best idea I had was IsProbablySame.

[JDevlieghere]

IsProbablySame works for me