[Lldb-commits] [PATCH] D117071: [lldb/Plugins] Add support of multiple ScriptedThreads in a ScriptedProcess

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Unifying the two paths by making turning the other reference into an owned one is a step in the right direction, but it is not enough, as StructuredPythonObject expects a *borrowed* reference. So, instead of fixing things, you've made both code paths leak. :)

I've created D117462 <https://reviews.llvm.org/D117462>, which should make reasoning about ownership much easier. StructuredPythonObject and all of the LLDBSwigPython functions now tracks ownership automatically, and one only needs to be careful when converting to/from a void *. Ideally those would be removed too, but that would require a bigger refactor. If you rebase this patch on top of that, you should be able to transfer ownership correctly.

While working on the patch I also realized that pretty much all of our StructuredPythonObject interfaces were leaking. So, in a way, you were very unfortunate to hit a use after free here (like, it it must have taken multiple decrefs on borrowed references (one for each thread, I guess) to undo all of those leaks). OTOH, we were also fortunate that you've found this, as we can now improve the interfaces and fix those leaks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117071