[lldb] Make the NSSet formatter faster and less prone to infinite recursion

[teemperor]

Summary

Right now to get the 'NSSet *` pointer value we first derefence it and then take the address of the result.

Beside being inefficient this potentially can cause an infinite recursion if the pointer value we get is a pointer
of a type that the TypeSystem can't derefence. If the pointer is for example some form of void * that the
dynamic type resolution can't resolve to an actual type, then the Derefence call goes back to asking
the formatters how to reference it. If the NSSet formatter then checks if it's an NSSet variation under the
hood then we just end infinitely often recursion.

In practice this seems to happen with some form of Builtin.RawPointer we get from a NSDictionary in Swift.

FWIW, no other formatter is doing the same deref->addressOf as here and there doesn't seem to be any
specific reason to do so in the git history (it's just part of the initial formatter commit)

[JDevlieghere]

Looks reasonable to me. Barring a good explanation for the dereference -> address of roundtrip, this LGTM.

[kastiglione]

lldb/source/Plugins/Language/ObjC/NSSet.cpp

729

why GetValueAsUnsigned(0) here and GetPointerValue() above?

[shafik]

Seems reasonable to me, I wonder why it was done that way originally?

[teemperor]

lldb/source/Plugins/Language/ObjC/NSSet.cpp

729

Huh, wrong diff... GetValueAsUnsigned is apparently (according to the docs next to the function) how we are supposed to get pointer values. Not sure why, but it seems that's just the way it's done (I originally thought PointerValue is the right way but that seems actually quite uncommon. FWIW, I think they do the same thing?)

[teemperor]

• Made both ValueAsUnsigned which is what the other formatters use.

[aprantl]

Functionally this LGTM. Would be curious to do git archeology to understand why the old code used this pattern.

[jingham]

GetPointerValue strips sign bits, so formally it should be used whenever you are getting a pointer that might be signed. OTOH, GetPointerValue is pretty new and since plain data pointers aren't going to be signed, there was no reason to go convert all the uses. We probably should use GetPointerValue. Also, using GetValueAsUnsigned with a 0 fallback value is not a great idea, though it seems to have been pretty common practice. That means if we are unable to read the pointer value for some reason, we'll say it is NULL. I think it would be better to use LLDB_INVALID_ADDRESS in these cases.

lldb/source/Plugins/Language/ObjC/NSSet.cpp

bool lldb_private::formatters::NSSetISyntheticFrontEnd::Update() {
   m_data_64 = nullptr;
   m_ptr_size = 0;
   ValueObjectSP valobj_sp = m_backend.GetSP();
   if (!valobj_sp)
     return false;
   if (!valobj_sp)
     return false;
   m_exe_ctx_ref = valobj_sp->GetExecutionContextRef();
-  Status error;
-  if (valobj_sp->IsPointerType()) {
-    valobj_sp = valobj_sp->Dereference(error);
-    if (error.Fail() || !valobj_sp)
-      return false;
-  }
-  error.Clear();
   lldb::ProcessSP process_sp(valobj_sp->GetProcessSP());
   if (!process_sp)
     return false;
   m_ptr_size = process_sp->GetAddressByteSize();
-  uint64_t data_location = valobj_sp->GetAddressOf() + m_ptr_size;
+  uint64_t data_location = valobj_sp->GetPointerValue() + m_ptr_size;
+  Status error;
   if (m_ptr_size == 4) {
     m_data_32 = new DataDescriptor_32();
     process_sp->ReadMemory(data_location, m_data_32, sizeof(DataDescriptor_32),
                            error);
   } else {
     m_data_64 = new DataDescriptor_64();
     process_sp->ReadMemory(data_location, m_data_64, sizeof(DataDescriptor_64),
                            error);
lldb_private::formatters::
   m_data_32 = nullptr;
   delete m_data_64;
   m_data_64 = nullptr;
   if (!valobj_sp)
     return false;
   if (!valobj_sp)
     return false;
   m_exe_ctx_ref = valobj_sp->GetExecutionContextRef();
-  Status error;
-  if (valobj_sp->IsPointerType()) {
-    valobj_sp = valobj_sp->Dereference(error);
-    if (error.Fail() || !valobj_sp)
-      return false;
-  }
-  error.Clear();
   lldb::ProcessSP process_sp(valobj_sp->GetProcessSP());
   if (!process_sp)
     return false;
   m_ptr_size = process_sp->GetAddressByteSize();
-  uint64_t data_location = valobj_sp->GetAddressOf() + m_ptr_size;
+  uint64_t data_location = valobj_sp->GetValueAsUnsigned(0) + m_ptr_size;
+  Status error;
   if (m_ptr_size == 4) {
     m_data_32 = new D32();
     process_sp->ReadMemory(data_location, m_data_32, sizeof(D32),
                            error);
   } else {
     m_data_64 = new D64();
     process_sp->ReadMemory(data_location, m_data_64, sizeof(D64),
                            error);