[Lldb-commits] [PATCH] D146152: Add __lldb_init_module_with_target for use when sourcing modules for a target

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.


================
Comment at: lldb/bindings/python/python-wrapper.swig:1031-1048
+  // First call the target initializer:
+  if (target) {
+    python_function_name_string += ".__lldb_init_module_with_target";
+    python_function_name = python_function_name_string.c_str();
+
+    auto pfunc = PythonObject::ResolveNameWithDictionary<PythonCallable>(
+      python_function_name, dict);
----------------
bulbazord wrote:
> JDevlieghere wrote:
> > I'm surprised we might call both. The way I expected this to work is that if `__lldb_init_module_with_target` is defined, that's what we use if wee have a target and otherwise fall back to `__lldb_init_module` assuming it's defined.
> > 
> > What's the benefit of calling both? Do you expect the implementation to be different? Or do you think it's more likely that the implementations will be similar, just one having access to the target? 
> I can see the advantage to calling both: The use of `__lldb_init_module_with_target` can be used to set up things specific to your target and `__lldb_init_module` can be used for more general things. That being said, you should be able to do everything with `__lldb_init_module_with_target` since if you have a target, you should have a debugger too.
> 
> Whatever we choose to go with, the behavior should be explicitly documented here: https://lldb.llvm.org/use/python-reference.html (`llvm-project/lldb/docs/use/python-reference.rst`). We already document one, we should be documenting this one and the expected behavior when both are present.
How you would use this depends on how you expect your python module to be used.  

1) If your python module is only going to be sourced from a Scripting Resource in a Module, then you could just implement `__lldb_init_module_with_target`.  

2) If your python module is imported through `command script import`, then it will have to implement `__lldb_init_module` since it's really never initialized in the presence of a specific target, so I wouldn't have a valid target to pass in.  I don't want to pass in something like the "currently selected target" in this case just so there's something you can get a debugger from, that kinda lying...

3) It's only if you want this python module to be used in either mode, or if you need to support lldb's that have and don't have this new API, that you need to have both API's.  In that case, it seemed more useful to let the user separate the "I'm being initialized as a particular target's scripting resource" part of the initialization from the more general part.


================
Comment at: lldb/bindings/python/python-wrapper.swig:1031-1048
+  // First call the target initializer:
+  if (target) {
+    python_function_name_string += ".__lldb_init_module_with_target";
+    python_function_name = python_function_name_string.c_str();
+
+    auto pfunc = PythonObject::ResolveNameWithDictionary<PythonCallable>(
+      python_function_name, dict);
----------------
jingham wrote:
> bulbazord wrote:
> > JDevlieghere wrote:
> > > I'm surprised we might call both. The way I expected this to work is that if `__lldb_init_module_with_target` is defined, that's what we use if wee have a target and otherwise fall back to `__lldb_init_module` assuming it's defined.
> > > 
> > > What's the benefit of calling both? Do you expect the implementation to be different? Or do you think it's more likely that the implementations will be similar, just one having access to the target? 
> > I can see the advantage to calling both: The use of `__lldb_init_module_with_target` can be used to set up things specific to your target and `__lldb_init_module` can be used for more general things. That being said, you should be able to do everything with `__lldb_init_module_with_target` since if you have a target, you should have a debugger too.
> > 
> > Whatever we choose to go with, the behavior should be explicitly documented here: https://lldb.llvm.org/use/python-reference.html (`llvm-project/lldb/docs/use/python-reference.rst`). We already document one, we should be documenting this one and the expected behavior when both are present.
> How you would use this depends on how you expect your python module to be used.  
> 
> 1) If your python module is only going to be sourced from a Scripting Resource in a Module, then you could just implement `__lldb_init_module_with_target`.  
> 
> 2) If your python module is imported through `command script import`, then it will have to implement `__lldb_init_module` since it's really never initialized in the presence of a specific target, so I wouldn't have a valid target to pass in.  I don't want to pass in something like the "currently selected target" in this case just so there's something you can get a debugger from, that kinda lying...
> 
> 3) It's only if you want this python module to be used in either mode, or if you need to support lldb's that have and don't have this new API, that you need to have both API's.  In that case, it seemed more useful to let the user separate the "I'm being initialized as a particular target's scripting resource" part of the initialization from the more general part.
I will add docs once we've decided the right way to do this.


================
Comment at: lldb/bindings/python/python-wrapper.swig:1056
 
   pfunc(ToSWIGWrapper(std::move(debugger)), dict);
 
----------------
bulbazord wrote:
> I know you didn't touch this but we're assuming debugger isn't nullptr here right? Should we add an assert? I know we'd have to be in a bad state to get to this point but an assertion may help catch that kind of thing if we do hit this point...
That doesn't seem to be the practice in this file for any of the shared pointer parameters that we call ToSWIGWrapper on.  If we're going to start being more careful about this, we should do so consistently, which seems outside the scope of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146152