[lldb] Copy ScriptedInterface object instance to a new StructuredData object

[mib]

Summary

This patch adds the ability to pass a pointer of a script object instance
to initialize a ScriptedInterface instead of having the call the
script object initializer in the ScriptedInterface constructor.

rdar://87425859

Signed-off-by: Med Ismail Bennani <[e-mail]>

[labath]

I'd like to remind everyone of a not-very-widely-known, but incredibly nifty feature of std::shared_ptr, called "aliasing". It allows one to create a shared_ptr which points to one object, but deletes a completely different object when it goes out of scope. It works like so:

std::shared_ptr<A> a = std::make_shared<A>(...); // or whatever
std::shared_ptr<B> b(a, getB(a)); // will point to a B, but call `delete A` when it goes out of scope

The model use case is when the first object is a subobject (field) of the second one (so we'd have struct A { B b; ... }; and getB(a) would be &a->b), but that is not a a requirement. B can be a completely arbitrary object -- the only requirement is that B remains live for as long as A is around.

Now, I don't understand this code well enough to say whether that could/should be used here, but the (few) parts which I did understand led me to think that there are some subobjects (in the broadest sense of the word) being passed around, and so I have a feeling we should at least consider this option -- it seems like it would be better to pass around objects with explicit ownership semantics instead of raw pointers.

[mib]

In D117139#3240436, @labath wrote:

I'd like to remind everyone of a not-very-widely-known, but incredibly nifty feature of std::shared_ptr, called "aliasing". It allows one to create a shared_ptr which points to one object, but deletes a completely different object when it goes out of scope. It works like so:

std::shared_ptr<A> a = std::make_shared<A>(...); // or whatever
std::shared_ptr<B> b(a, getB(a)); // will point to a B, but call `delete A` when it goes out of scope

The model use case is when the first object is a subobject (field) of the second one (so we'd have struct A { B b; ... }; and getB(a) would be &a->b), but that is not a a requirement. B can be a completely arbitrary object -- the only requirement is that B remains live for as long as A is around.

Very neat indeed, but I don't think it will be of any use here unfortunately: IIUC in your example, b is the first to go out-of-scope which calls a destructor. In my case, a when out of scope but it still referenced by b.

Now, I don't understand this code well enough to say whether that could/should be used here, but the (few) parts which I did understand led me to think that there are some subobjects (in the broadest sense of the word) being passed around, and so I have a feeling we should at least consider this option -- it seems like it would be better to pass around objects with explicit ownership semantics instead of raw pointers.

I agree with you on theory but here, the ScriptedThreadInterface tries to be language-agnostic that's why I pass StructuredData::GenericSP around.

[mib]

Following @JDevlieghere's request, the patch will be merged with D117071 since it makes more sense to have both changes together.

[labath]

In D117139#3240564, @mib wrote:

In D117139#3240436, @labath wrote:

I'd like to remind everyone of a not-very-widely-known, but incredibly nifty feature of std::shared_ptr, called "aliasing". It allows one to create a shared_ptr which points to one object, but deletes a completely different object when it goes out of scope. It works like so:

std::shared_ptr<A> a = std::make_shared<A>(...); // or whatever
std::shared_ptr<B> b(a, getB(a)); // will point to a B, but call `delete A` when it goes out of scope

The model use case is when the first object is a subobject (field) of the second one (so we'd have struct A { B b; ... }; and getB(a) would be &a->b), but that is not a a requirement. B can be a completely arbitrary object -- the only requirement is that B remains live for as long as A is around.

Very neat indeed, but I don't think it will be of any use here unfortunately: IIUC in your example, b is the first to go out-of-scope which calls a destructor. In my case, a when out of scope but it still referenced by b.

That is perfectly fine. All of the shared pointers created in this way share their reference counts. It doesn't matter which one goes out of scope first, but when the last one (whichever it is) does, it will call delete on the original object (a). (It is then expected that the deletion of a will somehow ensure that b gets deleted as well -- that is trivially true if b is a member, but may need additional logic in other cases.)
So, if the following holds:

• a is necessary for the proper usage of b (e.g. because b holds a pointer back to a)
• deletion of a will (should) result in deletion of b

then you can still use this pattern.

Now, I don't understand this code well enough to say whether that could/should be used here, but the (few) parts which I did understand led me to think that there are some subobjects (in the broadest sense of the word) being passed around, and so I have a feeling we should at least consider this option -- it seems like it would be better to pass around objects with explicit ownership semantics instead of raw pointers.

I agree with you on theory but here, the ScriptedThreadInterface tries to be language-agnostic that's why I pass StructuredData::GenericSP around.

GenericSP is fine(ish), but in this patch i see a lot of void*'s floating around (which (for an uninitiated reader) is even worse than a raw pointer, since he doesn't even know what kind of an object is that referring to, let alone the ownership semantics). btw, std::shared_ptr<void> is also a thing.

[mib]

In D117139#3240631, @labath wrote:

In D117139#3240564, @mib wrote:

In D117139#3240436, @labath wrote:

I'd like to remind everyone of a not-very-widely-known, but incredibly nifty feature of std::shared_ptr, called "aliasing". It allows one to create a shared_ptr which points to one object, but deletes a completely different object when it goes out of scope. It works like so:

std::shared_ptr<A> a = std::make_shared<A>(...); // or whatever
std::shared_ptr<B> b(a, getB(a)); // will point to a B, but call `delete A` when it goes out of scope

The model use case is when the first object is a subobject (field) of the second one (so we'd have struct A { B b; ... }; and getB(a) would be &a->b), but that is not a a requirement. B can be a completely arbitrary object -- the only requirement is that B remains live for as long as A is around.

Very neat indeed, but I don't think it will be of any use here unfortunately: IIUC in your example, b is the first to go out-of-scope which calls a destructor. In my case, a when out of scope but it still referenced by b.

That is perfectly fine. All of the shared pointers created in this way share their reference counts. It doesn't matter which one goes out of scope first, but when the last one (whichever it is) does, it will call delete on the original object (a). (It is then expected that the deletion of a will somehow ensure that b gets deleted as well -- that is trivially true if b is a member, but may need additional logic in other cases.)
So, if the following holds:

• a is necessary for the proper usage of b (e.g. because b holds a pointer back to a)
• deletion of a will (should) result in deletion of b

then you can still use this pattern.

Very cool stuff! I'll see if/how I can use this on my patch. May be I can make the StructuredData::GenericSP "alias" the underlying smart pointer, so we don't leak memory when deleting it.

Now, I don't understand this code well enough to say whether that could/should be used here, but the (few) parts which I did understand led me to think that there are some subobjects (in the broadest sense of the word) being passed around, and so I have a feeling we should at least consider this option -- it seems like it would be better to pass around objects with explicit ownership semantics instead of raw pointers.

I agree with you on theory but here, the ScriptedThreadInterface tries to be language-agnostic that's why I pass StructuredData::GenericSP around.

GenericSP is fine(ish), but in this patch i see a lot of void*'s floating around (which (for an uninitiated reader) is even worse than a raw pointer, since he doesn't even know what kind of an object is that referring to, let alone the ownership semantics). btw, std::shared_ptr<void> is also a thing.

I've never considered that 🤯 I'll update D117071 to turn the void* into std::shared_ptr<void>.

Thanks!

[mib]

I've never considered that 🤯 I'll update D117071 to turn the void* into std::shared_ptr<void>.

I tried doing that (see attached patch below) but it doesn't work because even though the StructuredData::Generic is created with a PyObject*, it's stored as a void* and is retrieved in the ScriptedThread constructor using void * StructuredData::Generic::GetValue() const.

Making a std::shared_ptr<void> out of a void * doesn't work because it's an incomplete type.

@labath If you have any other suggestion, please let me know :-)

share_ptr_void.diff7 KBDownload

[labath]

Yes, you can't "turn" a void* into a shared_ptr<void> because the shared pointer would not know how to delete the pointed object. However, you can create a pointer which is /aliased/ to a void* and use that for something.

That said, I am (still) not sure if that is what you really want to do. And the more I look at this patch, the more it seems to me that there is still something wrong with the ownerships. Maybe we could start by clarifying what the ownership of various object really is, then seeing if we have some tools that would make that explicit. I'll continue this on the merged patch.

lldb/include/lldb/Interpreter/ScriptedInterface.h

 namespace lldb_private {
 class ScriptedInterface {
 public:
   ScriptedInterface() = default;
   virtual ~ScriptedInterface() = default;
 
   virtual StructuredData::GenericSP
   CreatePluginObject(llvm::StringRef class_name, ExecutionContext &exe_ctx,
-                     StructuredData::DictionarySP args_sp) = 0;
+                     StructuredData::DictionarySP args_sp,
+                     void *instance_obj = nullptr) = 0;
 
   template <typename Ret>
   Ret ErrorWithMessage(llvm::StringRef caller_name, llvm::StringRef error_msg,
                        Status &error,
                        uint32_t log_caterogy = LIBLLDB_LOG_PROCESS) {
     LLDB_LOGF(GetLogIfAllCategoriesSet(log_caterogy), "%s ERROR = %s",
               caller_name.data(), error_msg.data());
     error.SetErrorString(llvm::Twine(caller_name + llvm::Twine(" ERROR = ") +

lldb/include/lldb/Interpreter/ScriptedProcessInterface.h

 
 #include <string>
 
 namespace lldb_private {
 class ScriptedProcessInterface : virtual public ScriptedInterface {
 public:
   StructuredData::GenericSP
   CreatePluginObject(llvm::StringRef class_name, ExecutionContext &exe_ctx,
-                     StructuredData::DictionarySP args_sp) override {
+                     StructuredData::DictionarySP args_sp,
+                     void *instance_obj = nullptr) override {
     return nullptr;
   }
 
   virtual Status Launch() { return Status("ScriptedProcess did not launch"); }
 
   virtual Status Resume() { return Status("ScriptedProcess did not resume"); }
 
   virtual bool ShouldStop() { return true; }
virtual lldb::ScriptedThreadInterfaceSP CreateScriptedThreadInterface() {
     return nullptr;
   }
 };
 
 class ScriptedThreadInterface : virtual public ScriptedInterface {
 public:
   StructuredData::GenericSP
   CreatePluginObject(llvm::StringRef class_name, ExecutionContext &exe_ctx,
-                     StructuredData::DictionarySP args_sp) override {
+                     StructuredData::DictionarySP args_sp,
+                     void *instance_obj = nullptr) override {
     return nullptr;
   }
 
   virtual lldb::tid_t GetThreadID() { return LLDB_INVALID_THREAD_ID; }
 
   virtual llvm::Optional<std::string> GetName() { return llvm::None; }
 
   virtual lldb::StateType GetState() { return lldb::eStateInvalid; }

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h

 class ScriptedProcessPythonInterface : public ScriptedProcessInterface,
                                        public ScriptedPythonInterface {
 public:
   ScriptedProcessPythonInterface(ScriptInterpreterPythonImpl &interpreter);
 
   StructuredData::GenericSP
   CreatePluginObject(const llvm::StringRef class_name,
                      ExecutionContext &exe_ctx,
-                     StructuredData::DictionarySP args_sp) override;
+                     StructuredData::DictionarySP args_sp,
+                     void *instance_obj = nullptr) override;
 
   Status Launch() override;
 
   Status Resume() override;
 
   bool ShouldStop() override;
 
   Status Stop() override;

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp

 using Locker = ScriptInterpreterPythonImpl::Locker;
 
 ScriptedProcessPythonInterface::ScriptedProcessPythonInterface(
     ScriptInterpreterPythonImpl &interpreter)
     : ScriptedProcessInterface(), ScriptedPythonInterface(interpreter) {}
 
 StructuredData::GenericSP ScriptedProcessPythonInterface::CreatePluginObject(
     llvm::StringRef class_name, ExecutionContext &exe_ctx,
-    StructuredData::DictionarySP args_sp) {
+    StructuredData::DictionarySP args_sp, void *instance_obj) {
   if (class_name.empty())
     return {};
 
   TargetSP target_sp = exe_ctx.GetTargetSP();
   StructuredDataImpl args_impl(args_sp);
   std::string error_string;
 
   Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
                  Locker::FreeLock);
 
-  void *ret_val = LLDBSwigPythonCreateScriptedProcess(
+  if (!instance_obj) {
+    instance_obj = LLDBSwigPythonCreateScriptedProcess(
         class_name.str().c_str(), m_interpreter.GetDictionaryName(), target_sp,
         args_impl, error_string);
 
-  if (!ret_val)
+    if (!instance_obj)
       return {};
+  }
 
   m_object_instance_sp =
-      StructuredData::GenericSP(new StructuredPythonObject(ret_val));
+      StructuredData::GenericSP(new StructuredPythonObject(instance_obj));
 
   return m_object_instance_sp;
 }
 
 Status ScriptedProcessPythonInterface::Launch() {
   return GetStatusFromMethod("launch");
 }
 

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedThreadPythonInterface.h

 namespace lldb_private {
 class ScriptedThreadPythonInterface : public ScriptedThreadInterface,
                                       public ScriptedPythonInterface {
 public:
   ScriptedThreadPythonInterface(ScriptInterpreterPythonImpl &interpreter);
 
   StructuredData::GenericSP
   CreatePluginObject(llvm::StringRef class_name, ExecutionContext &exe_ctx,
-                     StructuredData::DictionarySP args_sp) override;
+                     StructuredData::DictionarySP args_sp,
+                     void *instance_obj = nullptr) override;
 
   lldb::tid_t GetThreadID() override;
 
   llvm::Optional<std::string> GetName() override;
 
   lldb::StateType GetState() override;
 
   llvm::Optional<std::string> GetQueue() override;

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedThreadPythonInterface.cpp

 using Locker = ScriptInterpreterPythonImpl::Locker;
 
 ScriptedThreadPythonInterface::ScriptedThreadPythonInterface(
     ScriptInterpreterPythonImpl &interpreter)
     : ScriptedThreadInterface(), ScriptedPythonInterface(interpreter) {}
 
 StructuredData::GenericSP ScriptedThreadPythonInterface::CreatePluginObject(
     const llvm::StringRef class_name, ExecutionContext &exe_ctx,
-    StructuredData::DictionarySP args_sp) {
+    StructuredData::DictionarySP args_sp, void *instance_obj) {
 
   if (class_name.empty())
     return {};
 
   ProcessSP process_sp = exe_ctx.GetProcessSP();
   StructuredDataImpl args_impl(args_sp);
   std::string error_string;
 
   Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
                  Locker::FreeLock);
 
-  void *ret_val = LLDBSwigPythonCreateScriptedThread(
+  if (!instance_obj) {
+    instance_obj = LLDBSwigPythonCreateScriptedThread(
         class_name.str().c_str(), m_interpreter.GetDictionaryName(), process_sp,
         args_impl, error_string);
 
-  if (!ret_val)
+    if (!instance_obj)
       return {};
+  }
 
   m_object_instance_sp =
-      StructuredData::GenericSP(new StructuredPythonObject(ret_val));
+      StructuredData::GenericSP(new StructuredPythonObject(instance_obj));
 
   return m_object_instance_sp;
 }
 
 lldb::tid_t ScriptedThreadPythonInterface::GetThreadID() {
   Status error;
   StructuredData::ObjectSP obj = Dispatch("get_thread_id", error);