[Lldb-commits] [PATCH] D107585: [lldb/Plugins] Add support for ScriptedThread in ScriptedProcess

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/examples/python/scripted_process/scripted_process.py:242
+        """
+        pass
+
----------------
Should `eStateStopped` be the default?


================
Comment at: lldb/examples/python/scripted_process/scripted_process.py:244
+
+    # @abstractmethod
+    def get_queue(self):
----------------
Why's this commented out?


================
Comment at: lldb/include/lldb/Interpreter/ScriptedProcessInterface.h:77
+  StructuredData::GenericSP
+  CreatePluginObject(const llvm::StringRef class_name,
+                     ExecutionContext &exe_ctx,
----------------



================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:105
+const char *ScriptedThread::GetName() {
+  if (m_name.empty()) {
+    CheckInterpreterAndScriptObject();
----------------
We're caching the name and the queue, is this an optimization (to avoid calling into Python over and over) or are there assumptions that will break if these are not stable? If so, should that be something we call out on the Python side or alternatively, should we allow this to change and not cache it? 


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:141-144
+  uint32_t concrete_frame_idx = 0;
+
+  if (frame)
+    concrete_frame_idx = frame->GetConcreteFrameIndex();
----------------



================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:152-156
+  auto error_with_message = [](llvm::StringRef message) {
+    LLDB_LOGF(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS),
+              "ScriptedProcess::%s ERROR = %s", __FUNCTION__, message.data());
+    return false;
+  };
----------------
Seems like you have this lambda in a lot of places, I wonder if it's worth the trade-off of making this a static function that takes `__FUNCTION__` as an extra argument. 


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:158-160
+  if (m_cached_stop_info_sp) {
+    SetStopInfo(m_cached_stop_info_sp);
+  } else {
----------------
You call `SetStopInfo(m_cached_stop_info_sp);` on line 211, so we could drop the case for `m_cached_stop_info_sp` being true? 


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedThreadPythonInterface.cpp:128-133
+  if (!obj || !obj->IsValid() || error.Fail()) {
+    return error_with_message(llvm::Twine("Null or invalid object (" +
+                                          llvm::Twine(error.AsCString()) +
+                                          llvm::Twine(")."))
+                                  .str());
+  }
----------------
This code is duplicated over and over for objects and dicts. Seems like a templated function could help here. Then you wouldn't have to worry about being more verbose and differentiating the different errors (null vs invalid vs error). Also, is there always an error when `!obj` or `!obj->IsValid()` or are there cases where this might print something like "Null or invalid object ()"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107585