[Lldb-commits] [PATCH] D95713: [lldb/Plugins] Add ScriptedProcess Process Plugin

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/source/Plugins/Process/CMakeLists.txt:16
 endif()
+add_subdirectory(Scripted)
 add_subdirectory(gdb-remote)
----------------
nit: I'd make this lowercase for consistency with the other plugins (and keep Utility the exception as it's not a plugin). 


================
Comment at: lldb/source/Plugins/Process/Scripted/ScriptedProcess.cpp:1
+//===-- ScriptedProcess.cpp -------------------------------------------===//
+//
----------------
This is shorter than the closing ASCII art. Missing `-`s?


================
Comment at: lldb/source/Plugins/Process/Scripted/ScriptedProcess.cpp:41
+                                                bool can_connect) {
+  LaunchInfo launch_info(target_sp->GetProcessLaunchInfo());
+
----------------
Maybe qualify this as `ScriptedProcess::LaunchInfo` to make it clear you're not just making a copy but actually converting it.

This also answers my earlier question about who holds onto the LaunchInfo: nobody once this function returns. I would pass he process launch info to the `ScriptedProcess` and have the constructor instantiate the `ScriptedProcess::LaunchInfo` as its ivar. 


================
Comment at: lldb/source/Plugins/Process/Scripted/ScriptedProcess.cpp:43
+
+  return std::make_shared<ScriptedProcess>(target_sp, listener_sp, launch_info);
+}
----------------
This will always return a valid ScriptedProcess which (IIRC) is the mechanism used by the plugin framework to communicate that the current plugin is the right one and able to handle the process. Shouldn't it only return a  ScriptedProcess when the user has requested one? Does this work when you launch without `-S`?


================
Comment at: lldb/source/Plugins/Process/Scripted/ScriptedProcess.cpp:159
+  Status error;
+  auto size =
+      m_interpreter->ScriptedProcess_GetNumMemoryRegions(m_python_object_sp);
----------------
`size_t`


================
Comment at: lldb/source/Plugins/Process/Scripted/ScriptedProcess.cpp:167
+
+  for (uint64_t i = 0; i < size; i++) {
+    // FIXME: Update interface method to handle extra arg (index)
----------------
`size_t`


================
Comment at: lldb/source/Plugins/Process/Scripted/ScriptedProcess.cpp:174
+    if (!mem_region_sp) {
+      // FIXME: Interpolate index in error string
+      error.SetErrorString(
----------------
Seems like this is still outstanding ;-) 


================
Comment at: lldb/source/Plugins/Process/Scripted/ScriptedProcess.h:1
+//===-- ScriptedProcess.h -------------------------------------------------===//
+//
----------------
Header needs the C++ thingy


================
Comment at: lldb/source/Plugins/Process/Scripted/ScriptedProcess.h:33
+  private:
+    llvm::StringRef m_class_name;
+    StructuredData::DictionarySP m_dictionary_sp;
----------------
I think someone else already pointed that out elsewhere but this should probably be a `std::string`. 


================
Comment at: lldb/source/Plugins/Process/Scripted/ScriptedProcess.h:102
+private:
+  const LaunchInfo &m_launch_info;
+  lldb_private::ScriptInterpreter *m_interpreter;
----------------
This seems suspicious. Who manages the lifetime of the launch info and how do we guarantee it matches the lifetime of the process? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95713