[lldb] Fix SBAddressRange validation checks.

[mbucko]

No description provided.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Miro Bucko (mbucko)

Changes

---

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

2 Files Affected:

• (modified) lldb/source/API/SBAddressRange.cpp (+3-15)
• (modified) lldb/test/API/python_api/address_range/TestAddressRange.py (+1-1)

diff --git a/lldb/source/API/SBAddressRange.cpp b/lldb/source/API/SBAddressRange.cpp
index 9b1affdade439..0fba252979093 100644
--- a/lldb/source/API/SBAddressRange.cpp
+++ b/lldb/source/API/SBAddressRange.cpp
@@ -50,8 +50,6 @@ const SBAddressRange &SBAddressRange::operator=(const SBAddressRange &rhs) {
 bool SBAddressRange::operator==(const SBAddressRange &rhs) {
   LLDB_INSTRUMENT_VA(this, rhs);
 
-  if (!IsValid() || !rhs.IsValid())
-    return false;
   return m_opaque_up->operator==(*(rhs.m_opaque_up));
 }
 
@@ -64,28 +62,24 @@ bool SBAddressRange::operator!=(const SBAddressRange &rhs) {
 void SBAddressRange::Clear() {
   LLDB_INSTRUMENT_VA(this);
 
-  m_opaque_up.reset();
+  m_opaque_up->Clear();
 }
 
 bool SBAddressRange::IsValid() const {
   LLDB_INSTRUMENT_VA(this);
 
-  return m_opaque_up && m_opaque_up->IsValid();
+  return m_opaque_up->IsValid();
 }
 
 lldb::SBAddress SBAddressRange::GetBaseAddress() const {
   LLDB_INSTRUMENT_VA(this);
 
-  if (!IsValid())
-    return lldb::SBAddress();
   return lldb::SBAddress(m_opaque_up->GetBaseAddress());
 }
 
 lldb::addr_t SBAddressRange::GetByteSize() const {
   LLDB_INSTRUMENT_VA(this);
 
-  if (!IsValid())
-    return 0;
   return m_opaque_up->GetByteSize();
 }
 
@@ -93,11 +87,5 @@ bool SBAddressRange::GetDescription(SBStream &description,
                                     const SBTarget target) {
   LLDB_INSTRUMENT_VA(this, description, target);
 
-  Stream &stream = description.ref();
-  if (!IsValid()) {
-    stream << "<invalid>";
-    return true;
-  }
-  m_opaque_up->GetDescription(&stream, target.GetSP().get());
-  return true;
+  return m_opaque_up->GetDescription(&description.ref(), target.GetSP().get());
 }
diff --git a/lldb/test/API/python_api/address_range/TestAddressRange.py b/lldb/test/API/python_api/address_range/TestAddressRange.py
index 86ca4a62155f0..ae4b8c7c90ce4 100644
--- a/lldb/test/API/python_api/address_range/TestAddressRange.py
+++ b/lldb/test/API/python_api/address_range/TestAddressRange.py
@@ -166,7 +166,7 @@ def test_address_range_list_iterator(self):
     def test_address_range_print_invalid(self):
         """Make sure the SBAddressRange can be printed when invalid."""
         range = lldb.SBAddressRange()
-        self.assertEqual(str(range), "<invalid>")
+        self.assertEqual(str(range), "[0xffffffffffffffff-0xffffffffffffffff)")
 
     def test_address_range_print_resolved(self):
         """Make sure the SBAddressRange can be printed when resolved."""

[JDevlieghere]

This class behaves quite differently from other SB API classes. Normally, the opaque pointer can be cleared to release the potentially more resource heavy private counterpart. AddressRange is a pretty simple class, so I understand that it makes things easier if we guarantee the pointer is always valid, but it is somewhat of a surprise.

Personally, I think consistency beats the small advantage of not having to check the pointer. If we want to stick to this approach, I'd like to see an assert that makes it clear that in this class, we have a precondition that the pointer is always valid:

assert(m_opaque_up && "opaque pointer must always be valid");

[mbucko]

This class behaves quite differently from other SB API classes. Normally, the opaque pointer can be cleared to release the potentially more resource heavy private counterpart. AddressRange is a pretty simple class, so I understand that it makes things easier if we guarantee the pointer is always valid, but it is somewhat of a surprise.

Personally, I think consistency beats the small advantage of not having to check the pointer. If we want to stick to this approach, I'd like to see an assert that makes it clear that in this class, we have a precondition that the pointer is always valid:

assert(m_opaque_up && "opaque pointer must always be valid");

So you're ok with keeping the code as is plus adding the asserts?

[clayborg]

This class behaves quite differently from other SB API classes. Normally, the opaque pointer can be cleared to release the potentially more resource heavy private counterpart. AddressRange is a pretty simple class, so I understand that it makes things easier if we guarantee the pointer is always valid, but it is somewhat of a surprise.

Personally, I think consistency beats the small advantage of not having to check the pointer. If we want to stick to this approach, I'd like to see an assert that makes it clear that in this class, we have a precondition that the pointer is always valid:

assert(m_opaque_up && "opaque pointer must always be valid");

For simple classes, there is no need to clear the unique pointer, so I like this approach for small classes.

We can use the assert in a new protected ref() method:

lldb_private::AddressRange & SBAddressRange::ref() {
  assert(m_opaque_up && "opaque pointer must always be valid");
  return *m_opaque_up;
}

And then have everything that accesses m_opaque_up use the ref() function. It is similar to other classes and makes the code nicer when we don't see direct uses of m_opaque_up