[Lldb-commits] [PATCH] D90729: [trace][intel-pt] Scaffold the 'thread trace start | stop' commands

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/include/lldb/Interpreter/CommandObjectDelegate.h:30-32
+  virtual lldb::CommandObjectSP DoGetDelegateCommand() = 0;
+
+  CommandObject *GetDelegateCommand();
----------------
So currently you are storing the command object into m_delegate_command_sp in GetDelegateCommand(). This can cause an issue when you have two debuggers (Xcode creates a new debugger for each debug window). If thread A calls this function, and fills in m_delegate_command_sp, then thread B fills in m_delegate_command_sp from another command interpreter, we could end up using the one from thread B. Since the only reason we are storing the value in m_delegate_command_sp is to keep the shared pointer alive, we should just return the shared pointer. So I would suggest removing DoGetDelegateCommand() and just have:

```
virtual llvm::Expected<lldb::CommandObjectSP> GetDelegateCommand() = 0;
```
Don't store it. Each function that calls it will keep it around as long as needed. If we use expected then we don't need to store the m_delegate_error later...


================
Comment at: lldb/source/Interpreter/CommandObjectDelegate.cpp:16-19
+  m_delegate_command_sp = DoGetDelegateCommand();
+  if (m_delegate_command_sp)
+    return m_delegate_command_sp.get();
+  return nullptr;
----------------
This caching isn't really doing anything. If we have two debuggers, each one has its own command interpreter and this can actually cause things to fail. If thread A calls this, then thread B, then thread A continues to use the one populated by thread B. See inline comment in header file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90729