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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 5 inline comments as done.
mib added inline comments.


================
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);
----------------
labath wrote:
> mib wrote:
> > davide wrote:
> > > 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].
> > Indeed, they're very similar and I already tried using templates (and SFINAE) to make it more generic, however I couldn't achieve that.
> > 
> > Since the remote architecture might be different from lldb's, we can't use macros to generate the underlying struct with the right size. So, I decided to template the structure used by CF, and have one of each architecture as a class attribute (look at CFBasicHash.h:114). 
> > 
> > Basically it's a tradeoff I chose voluntarily: I preferred having the CFBasicHash class handle the architecture to only expose one CFBasicHash object in the CFDictionary and CFSet data-formatter, rather than having two CFBasicHash objects templated for each ptr_size and have all the logic duplicated for each different architecture AND each data formatters.
> > 
> > If you can see a better way to do it, please let me know :) 
> ```
> template<typename T> updateFor(std::unique_ptr<__CFBasicHash<T>> &m_ht, ...)
> 
> if (m_ptr_size == 4)
>   updateFor<uint32_t>(m_ht_4, ...);
> else if (m_ptr_size == 8)
>   updateFor<uint64_t>(m_ht_8, ...)
> ```
> ?
> 
> Or the entire class could be a template, inheriting from a common (non-templatized) interface...
I didn't think about that --' ... Thanks for the suggestion ^^


================
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,
----------------
labath wrote:
> mib wrote:
> > davide wrote:
> > > Maybe a reference to the foundation header where these are defined, if public.
> > It is not in a public header, that's why I copied the explanation.
> Are these actually used anywhere?
This is a left-over used in the multi variant implement that I'm working on ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78396