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

[Alex Langford via Phabricator via lldb-commits]

bulbazord 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);
----------------
jingham wrote:
> 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.
I feel like this may lead to some confusion. `__lldb_init_module_with_target` makes sense as the thing that runs when you source the python file from dSYM and `__lldb_init_module` makes sense as the thing that runs when you do `command script import`. Implicitly running `__lldb_init_module` when you are imported as a scripting resource in a module would mean there's no way to get different behavior depending on which thing ran. Specifically, `__lldb_init_module` is always guaranteed to run. If a user wanted both to run, I think it might be better as an explicit thing. Like `__lldb_init_module_with_target` can call `__lldb_init_module`... I'm not sure, what do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146152