[Lldb-commits] [PATCH] D122859: [trace] Show ideas for the main interfaces for new HTR

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:81
+    // and not to a child.
+    lldb::user_id_t GetLastInstructionId() { return m_last_insn_id; }
+
----------------
This seems like it should be stored as an optional value and if the optional value has no valid, it would need to be  computed lazily?


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:83
+
+    size_t GetChildrenCount() { return m_children; }
+
----------------
Should this function be able to lazily compute itself? Right now you would need to parse the entire stream of instructions for a HTRInMemoryBlock contents to be valid? Is that what we want?


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:88
+    // most 2^32 elements, which seems reasonable.
+    size_t &GetChildAtIndex(size_t index) { return m_children_indices[index]; }
+
----------------
If you go with the GetChildrenCount() + GetChildAtIndex(...) workflow, then you can't lazily compute children. Do we want to go with an iterator kind of thing where you ask for the iterator for the first child and then ask for next until some ending?


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:90
+
+    llvm::Optional<size_t> GetParent() { return m_parent_index; }
+
----------------
I would avoid size_t as it is uint32_t on 32 bit systems and uint64_t on 64 bit systems. Probably just pick uint64_t?


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:113
+  // We need this accessor because each block doesn't know its own index
+  HTRInMemoryBlock &GetBlockAtIndex(size_t index) { return m_blocks[index]; }
+
----------------
Bounds check "index"?


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:187-188
+  union {
+    TraceHTRInMemoryStorage *m_in_memory_storage;
+    TraceHTROnDiskStore *m_on_disk_storage;
+  };
----------------
Should we just make a TraceHTR base class that has all of the needed methods and store a single pointer here? Then TraceHTRInMemoryStorage and TraceHTROnDiskStore would subclass it?


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:191-192
+  union {
+    HTRInMemoryBlock *m_in_memory_block;
+    HTRInMemoryBlock *m_on_disk_block;
+  };
----------------
Should we just make a HTRBlock base class that has all of the needed methods and store a single pointer here? Then HTRInMemoryBlock and HTROnDiskBlock would subclass it?


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:192
+    HTRInMemoryBlock *m_in_memory_block;
+    HTRInMemoryBlock *m_on_disk_block;
+  };
----------------



================
Comment at: lldb/include/lldb/Target/TraceHTR.h:308
+private:
+  std::map<uint32_t, std::vector<size_t>> m_mapping; // symbol id -> block index
+};
----------------

is the idea to somehow represent a lldb_private::Symbol from this index? What does a "symbol id" mean?


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:352-355
+  // Only of them active at a given time. An abstract class might also come
+  // handy.
+  std::unique_ptr<TraceHTRInMemoryStorage> m_in_memory_storage;
+  std::unique_ptr<TraceHTROnDiskStorage> m_in_disk_storage;
----------------
Yeah, I would make a base class here and just store one unique pointer here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122859