Add __lldb_init_module_with_target for use when sourcing modules for a target

[jingham]

Summary

We load python modules in two main ways: through command script import and automatically when we read in a dSYM and find a scripting resource in the dSYM. The latter case has a slight flaw currently which is that when the scripting resource from the dSYM is loaded, the target it's being loaded for is in the process of being created, so the module being imported has no way of knowing what that target is. Even if the debugger had selected this new target before loading the scripting resource, it would still be relying on that being the "currently selected target" which is racy. It's better to do this explicitly.

This patch adds a detector for an optional:

__lldb_init_module_with_target

and if that is present it will run that routine - passing in the target - before running __lldb_init_module.

I considered making an overload of __lldb_init_module that also passed in the target, but that trick seems to end up being confusing. And this will allow more convenient "dual use" modules, where if you are being sourced for a target you can prime the state for the regular init, and if not, then run the regular init.

[JDevlieghere]

Thanks for adding this, Jim. This is one of those things that comes up one in a while when it confuses our users. It's great we'll be able to help them adopt this.

lldb/bindings/python/python-wrapper.swig

1031–1048

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?

[bulbazord]

lldb/bindings/python/python-wrapper.swig

1031–1048

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.

1056

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...

[jingham]

lldb/bindings/python/python-wrapper.swig

1031–1048

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.

1031–1048

I will add docs once we've decided the right way to do this.

1056

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.

[jingham]

Polite review ping...

[bulbazord]

lldb/bindings/python/python-wrapper.swig

1031–1048

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?

lldb/bindings/python/python-wrapper.swig

bool lldb_private::LLDBSWIGPythonRunScriptKeywordValue(
 
   output = result.Str().GetString().str();
 
   return true;
 }
 
 bool lldb_private::LLDBSwigPythonCallModuleInit(
     const char *python_module_name, const char *session_dictionary_name,
-    lldb::DebuggerSP debugger) {
+    lldb::DebuggerSP debugger, lldb::TargetSP target) {
+
   std::string python_function_name_string = python_module_name;
-  python_function_name_string += ".__lldb_init_module";
-  const char *python_function_name = python_function_name_string.c_str();
+  const char *python_function_name = nullptr;
 
   PyErr_Cleaner py_err_cleaner(true);
-
   auto dict = PythonModule::MainModule().ResolveName<PythonDictionary>(
       session_dictionary_name);
+      
+  // 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);
+
+    if (pfunc.IsAllocated())
+      pfunc(ToSWIGWrapper(std::move(target)), dict);
+  }
+
+  // Now call the general method.  also This method is optional and need
+  // not exist.  So if we don't find it,  it's actually a success, not a
+  // failure.
+  python_function_name_string = python_module_name;
+  python_function_name_string += ".__lldb_init_module";
+  python_function_name = python_function_name_string.c_str();
+  
   auto pfunc = PythonObject::ResolveNameWithDictionary<PythonCallable>(
       python_function_name, dict);
 
-  // This method is optional and need not exist.  So if we don't find it,
-  // it's actually a success, not a failure.
   if (!pfunc.IsAllocated())
     return true;
 
   pfunc(ToSWIGWrapper(std::move(debugger)), dict);
 
   return true;
 }
 
 lldb::ValueObjectSP lldb_private::LLDBSWIGPython_GetValueObjectSPFromSBValue(
     void *data) {
   lldb::ValueObjectSP valobj_sp;
   if (data) {

lldb/include/lldb/Interpreter/ScriptInterpreter.h

LoadScriptOptions &SetInitSession(bool b) {
     m_init_session = b;
     return *this;
   }
 
   LoadScriptOptions &SetSilent(bool b) {
     m_silent = b;
     return *this;
   }
   
+  void SetTarget(lldb::TargetSP target_sp) {
+    m_target_sp = target_sp;
+  }
+  
+  lldb::TargetSP GetTarget() const {
+    return m_target_sp;
+  }
+
 private:
   bool m_init_session = false;
   bool m_silent = false;
+  lldb::TargetSP m_target_sp;
 };
 
 class ScriptInterpreterIORedirect {
 public:
   /// Create an IO redirect. If IO is enabled, this will redirects the output
   /// to the command return object if set or to the debugger otherwise. If IO
   /// is disabled, it will redirect all IO to /dev/null.
   static llvm::Expected<std::unique_ptr<ScriptInterpreterIORedirect>>

lldb/source/Core/Module.cpp

if (num_specs) {
                     "true\n",
                     GetFileSpec().GetFileNameStrippingExtension().GetCString(),
                     scripting_fspec.GetPath().c_str());
               return false;
             }
             StreamString scripting_stream;
             scripting_fspec.Dump(scripting_stream.AsRawOstream());
             LoadScriptOptions options;
+            options.SetTarget(target->shared_from_this());
             bool did_load = script_interpreter->LoadScriptingModule(
                 scripting_stream.GetData(), options, error);
             if (!did_load)
               return false;
           }
         }
       } else {
         error.SetErrorString("invalid ScriptInterpreter");

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

 
 bool LLDBSwigPythonCallCommandObject(
     PyObject *implementor, lldb::DebuggerSP debugger, const char *args,
     lldb_private::CommandReturnObject &cmd_retobj,
     lldb::ExecutionContextRefSP exe_ctx_ref_sp);
 
 bool LLDBSwigPythonCallModuleInit(const char *python_module_name,
                                   const char *session_dictionary_name,
-                                  lldb::DebuggerSP debugger);
+                                  lldb::DebuggerSP debugger,
+                                  lldb::TargetSP target);
 
 python::PythonObject
 LLDBSWIGPythonCreateOSPlugin(const char *python_class_name,
                              const char *session_dictionary_name,
                              const lldb::ProcessSP &process_sp);
 
 python::PythonObject
 LLDBSWIGPython_CreateFrameRecognizer(const char *python_class_name,

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

bool ScriptInterpreterPythonImpl::LoadScriptingModule(
   error = ExecuteMultipleLines(command_stream.GetData(), exc_options);
   if (error.Fail())
     return false;
 
   // if we are here, everything worked
   // call __lldb_init_module(debugger,dict)
   if (!LLDBSwigPythonCallModuleInit(module_name.c_str(),
                                     m_dictionary_name.c_str(),
-                                    m_debugger.shared_from_this())) {
+                                    m_debugger.shared_from_this(),
+                                    options.GetTarget())) {
     error.SetErrorString("calling __lldb_init_module failed");
     return false;
   }
 
   if (module_sp) {
     // everything went just great, now set the module object
     command_stream.Clear();
     command_stream.Printf("%s", module_name.c_str());

lldb/test/API/commands/add-dsym/uuid/TestAddDsymCommand.py

def test_add_dsym_with_dSYM_bundle(self):
 
         # Call the program generator to produce main.cpp, version 1.
         self.generate_main_cpp(version=1)
         self.build(debug_info="dsym")
 
         self.exe_name = 'a.out'
         self.do_add_dsym_with_dSYM_bundle(self.exe_name)
 
+    @no_debug_info_test
+    def test_add_dsym_checking_initializer(self):
+        """Test that the 'add-dsym' correctly imports python code."""
+
+        # Call the program generator to produce main.cpp, version 1.
+        self.generate_main_cpp(version=1)
+        self.build(debug_info="dsym")
+
+        self.exe_name = 'a.out'
+        self.do_add_dsym_and_check_initializers(self.exe_name)
+
     def generate_main_cpp(self, version=0):
         """Generate main.cpp from main.cpp.template."""
         temp = os.path.join(self.getSourceDir(), self.template)
         with open(temp, 'r') as f:
             content = f.read()
 
         new_content = content.replace(
             '%ADD_EXTRA_CODE%',
def do_add_dsym_with_success(self, exe_name):
         self.expect("add-dsym " + right_path,
                     substrs=['symbol file', 'has been added to'])
 
     def do_add_dsym_with_dSYM_bundle(self, exe_name):
         """Test that the 'add-dsym' command informs the user about success when loading files in bundles."""
         exe_path = self.getBuildArtifact(exe_name)
         self.runCmd("file " + exe_path, CURRENT_EXECUTABLE_SET)
 
+
+        # This time, the UUID should be found inside the bundle
+        right_path = "%s.dSYM" % exe_path
+        self.expect("add-dsym " + right_path,
+                    substrs=['symbol file', 'has been added to'])
+
+    # Also test that when we added the dSYM, we also ran any python that was available,
+    # and ran both module initializers in the right order:
+    def do_add_dsym_and_check_initializers(self, exe_name):
+        import os, shutil
+        exe_path = self.getBuildArtifact(exe_name)
+        target = self.dbg.CreateTarget(exe_path)
+        self.dbg.SetSelectedTarget(target)
+        
+        self.assertTrue(target.IsValid(), "Made a valid target")
+
         # This time, the UUID should be found inside the bundle
         right_path = "%s.dSYM" % exe_path
+        # Add our scripting resource:
+        self.assertTrue(os.path.exists(right_path), "dSYM exists")
+        python_path = os.path.join(right_path, "Contents")
+        self.assertTrue(os.path.exists(python_path), "Contents exists")
+        python_path = os.path.join(python_path, "Resources")
+        self.assertTrue(os.path.exists(python_path), "Resources exists")
+        python_path = os.path.join(python_path, "Python")
+        os.mkdir(python_path)
+        source_name = "a_out.py"
+        shutil.copy(os.path.join(self.getSourceDir(), source_name),
+                    os.path.join(python_path, source_name))
+        
+        # Turn on reading in python in dsym files:
+        self.runCmd("settings set target.load-script-from-symbol-file true")
         self.expect("add-dsym " + right_path,
                     substrs=['symbol file', 'has been added to'])
+
+        
+        self.expect("script a_out.called_module_init", substrs=["True"],
+                    msg = "Called module initialization")
+        self.expect("script a_out.called_target_init", substrs=["True"],
+                    msg = "Called target initializer")
+        self.expect(f"script a_out.init_target", substrs = [target.executable.fullpath],
+                    msg = "Got the expected target")

lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp

bool lldb_private::LLDBSwigPythonCallCommandObject(
     PyObject *implementor, lldb::DebuggerSP debugger, const char *args,
     lldb_private::CommandReturnObject &cmd_retobj,
     lldb::ExecutionContextRefSP exe_ctx_ref_sp) {
   return false;
 }
 
 bool lldb_private::LLDBSwigPythonCallModuleInit(
     const char *python_module_name, const char *session_dictionary_name,
-    lldb::DebuggerSP debugger) {
+    lldb::DebuggerSP debugger, lldb::TargetSP target) {
   return false;
 }
 
 python::PythonObject
 lldb_private::LLDBSWIGPythonCreateOSPlugin(const char *python_class_name,
                                            const char *session_dictionary_name,
                                            const lldb::ProcessSP &process_sp) {
   return python::PythonObject();