[Lldb-commits] [PATCH] D78396: [lldb/Dataformatter] Add support for CoreFoundation Dictionaries and Sets

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

Thanks for taking care of this. It's a lot of work. First round of comments.



================
Comment at: lldb/source/Plugins/Language/ObjC/CFBasicHash.cpp:12
+
+CFBasicHash::~CFBasicHash() { m_address = LLDB_INVALID_ADDRESS; }
+
----------------
Why do you need this? Can't you just use the default destructor?


================
Comment at: lldb/source/Plugins/Language/ObjC/CFBasicHash.cpp:18-19
+      return m_ht_32;
+  return m_ht_64;
+  return false;
+}
----------------
I'm under the impression the second return is never hit.


================
Comment at: lldb/source/Plugins/Language/ObjC/CFBasicHash.cpp:23-24
+bool CFBasicHash::Update(addr_t addr, ExecutionContextRef exe_ctx_rf) {
+  if (addr == LLDB_INVALID_ADDRESS)
+    return false;
+
----------------
Can this ever happen? What if `addr` is `0` instead?


================
Comment at: lldb/source/Plugins/Language/ObjC/CFBasicHash.cpp:28-29
+  m_exe_ctx_ref = exe_ctx_rf;
+  m_ptr_size =
+      m_exe_ctx_ref.GetTargetSP()->GetArchitecture().GetAddressByteSize();
+  m_byte_order = m_exe_ctx_ref.GetTargetSP()->GetArchitecture().GetByteOrder();
----------------
Thanks for doing this, as it will work on remote devices. We really need to check the test passes on arm64 before committing.


================
Comment at: lldb/source/Plugins/Language/ObjC/CFBasicHash.cpp:35-71
+
+    size = sizeof(__CFBasicHash<uint32_t>::RuntimeBase);
+    size += sizeof(__CFBasicHash<uint32_t>::Bits);
+
+    DataBufferHeap buffer(size, 0);
+    m_exe_ctx_ref.GetProcessSP()->ReadMemory(addr, buffer.GetBytes(), size,
+                                             error);
----------------
These two pieces of code, `m_ptr_size == 4` and `m_ptr_size == 8` are surprisingly similar. I'm really worried we might have a bug in one of them and miss the other. Is there anything we can do to share the code? [e.g. templatize].


================
Comment at: lldb/source/Plugins/Language/ObjC/CFBasicHash.cpp:139-140
+
+  // Unsupported architecure
+  return false;
+}
----------------
This could be an `lldb_assert` or `unreachable`. We really shouldn't be here if ptrsize != 8 or ptrsize != 4, unless there's an egregious bug.


================
Comment at: lldb/source/Plugins/Language/ObjC/NSDictionary.cpp:101
 namespace formatters {
+#pragma mark NSDictionaryI
 class NSDictionaryISyntheticFrontEnd : public SyntheticChildrenFrontEnd {
----------------
Why?


================
Comment at: lldb/source/Plugins/Language/ObjC/NSDictionary.cpp:145
 
+#pragma mark NSCFDictionary
+class NSCFDictionarySyntheticFrontEnd : public SyntheticChildrenFrontEnd {
----------------
Again, why?


================
Comment at: lldb/source/Plugins/Language/ObjC/NSDictionary.cpp:178-188
+private:
+  // Prime numbers. Values above 100 have been adjusted up so that the
+  // malloced block size will be just below a multiple of 512; values
+  // above 1200 have been adjusted up to just below a multiple of 4096.
+  constexpr static const uintptr_t NSDictionaryCapacities[] = {
+      0,        3,        6,         11,        19,        32,       52,
+      85,       118,      155,       237,       390,       672,      1065,
----------------
Maybe a reference to the foundation header where these are defined, if public.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78396/new/

https://reviews.llvm.org/D78396