Don't push null ExecutionContext on CommandInterpreter exectx stack

[jasonmolenda]

Summary

Part of Tatyana's patch in https://reviews.llvm.org/D92164 added a stack of ExecutionContexts to the CommandInterpreter (pushed with OverrideExecutionContext, popped with RestoreExecutionContext) and when we need to retrieve the currently selected ExecutionContext via CommandInterpeter:: GetExecutionContext(), if the stack has an entry, we return the exe_ctx from the top of the stack, else we fall back to the Debugger::GetSelectedExecutionContext() which gives you an exe_ctx with the currently selected Target.

This patch is fixing the fact that the lldb command driver during startup, pushes the current exe_ctx on the stack here,

-> 2874	    OverrideExecutionContext(m_debugger.GetSelectedExecutionContext());
   2875	  auto finalize = llvm::make_scope_exit([this]() {
   2876	    RestoreExecutionContext();
0 0x00010d9fc80d    LLDB`lldb_private::CommandInterpreter::IOHandlerInputComplete + 285  CommandInterpreter.cpp:2874
1 0x00010d81573d    LLDB`lldb_private::IOHandlerEditline::Run + 349  IOHandler.cpp:582
2 0x00010d7ccd71    LLDB`lldb_private::Debugger::RunIOHandlers + 81  Debugger.cpp:879
3 0x00010d9fe0d4    LLDB`lldb_private::CommandInterpreter::RunCommandInterpreter + 212  CommandInterpreter.cpp:3126
4 0x00010d268370    LLDB`lldb::SBDebugger::RunCommandInterpreter + 544  SBDebugger.cpp:1238
5 0x00010854b8a2    lldb`Driver::MainLoop + 3298  Driver.cpp:678

There is no Target at this point, so we have an ExecutionContext with a nullptr for a target (and all other fields).

This Target won't get popped off the stack, it remains there as the first entry. When loading a corefile, which loads a binary and dSYM, and the dSYM has Python code in it, and the SB API tries to set some settings via SBCommandInterpreter::HandleCommand, if those commands need the current Target to set something, they will silently fail to do anything. Target settings that are set in the template target will work, but those that only take effect in the current target are ignored. It can be a little tricky to spot.

Complicating the test case writing, API tests don't have this same "push a nullptr target on to the stack" issue, so they won't reproduce the problem. I started writing an API test and I kept it in this patch because it might find regressions in the future. Jonas helped rewrite the test into a shell test that uses the driver program to show the issue and confirm the patch fixes it.

I can imagine we might iterate on exactly how I avoid pushing a nullptr ExecutionContext, but I think the basic safeguard is pretty straightforward - it's a pretty unusual situation to have no selected Targets, this only happens during early startup I suspect. Passing an arg to the make_scope_exit to avoid popping the stack seems especially unpretty. I wasn't worried about popping too many elements off the stack, the code will ignore that case, but I was worried that it might be possible to have a real ExecutionContext on the stack, then a nullptr ExecutionContext is pushed, and I didn't want to pop the real exectx later on.

[jasonmolenda]

Another possible approach would be to allow an ExecutionContext with a nullptr Target to push on to the stack, but then in CommandInterpeter::GetExecutionContext skip over those until we find one with a Target, or fall back to the Debugger::GetSelectedExecutionContext like we do with an empty stack.

[tatyana-krasnukha]

Added inline comment

lldb/source/Interpreter/CommandInterpreter.cpp

2842

The intention of overriding the context here with the currently selected one was to have the same context during HandleCommand and GetProcessOutput. However, this logic looks wrong to me now. HandleCommand may change the context and GetProcessOutput should use the new one. I think that you can safely remove this piece of code (untill IOHandlerInputComplete doesn't allow overriding execution context).

[ted]

@jasonmolenda I discovered the same issue in another way - create a python command and load it with "command script import". Here is test.py:

def __lldb_init_module(debugger, dict):
    debugger.HandleCommand(
        'command script add -f test.test test')
    print("test command loaded")

def test(debugger, register, result, dict):
    result.Clear()
    res = lldb.SBCommandReturnObject()
    command = 'target create /bin/ls'
    debugger.GetCommandInterpreter().HandleCommand(command, res)
    command = 'target modules search-paths add . ' + '/tmp'
    debugger.GetCommandInterpreter().HandleCommand(command, res)
    output = res.GetOutput()
    print(output)

    print('test done')

The 2nd HandleCommand uses the execution context of the original command, which doesn't have a target, so the search-paths add fails with no target.

(lldb) command script import test
test command loaded
(lldb) test
Current executable set to '/bin/ls' (x86_64).

test command called
error: error: invalid target, create a target using the 'target create' command

That should be fairly easy to turn into a Shell test, with "REQUIRES: python", because the error happens before the search-path is parsed, so it doesn't have to be valid.

[jingham]

I think we should make simple cases like this work for sure. But OTOH, if you are writing a script that you might use anywhere non-trivial, SBCommandInterpreter.HandleCommand(char *, SBError) is NOT a safe thing to call. You could be in an lldb session with more than one Target, and while your command is running, the other target could have hit a breakpoint that it intended to stop at, and so it becomes the currently selected target.

I'm not sure there's a good way to enforce this, but if you have an execution context in mind when running a command with HandleCommand, you really need to use the version of HandleCommand that takes an execution context, rather than just hoping that when the command is run your execution context is still the currently selected one.

[jingham]

This needs some more design, I think. The obvious solution to Ted's problem is that target create should make the new target the currently selected one. Then the second HandleCommand would have a selected target, and the second command would work. But that doesn't seem right to me when you consider that there might be more than one process at a time in a target, which we definitely want to support so that you can do things like coordinate debugging from a server process into a client, etc...

For instance, suppose I have two targets A & B, and I'm stopped at a breakpoint in target B, issuing commands in the command interpreter. Meanwhile, target A hits a breakpoint with a callback that creates a new target (C) and attaches that Target C to some other process, then continues without stopping. If we do have the HandleCommand for "target create" select the current target, then without anything that I can discern happening, Target C becomes the selected target, and all my commands start going to it. That can't be right.

For the Driver, this is not so complicated. If some one issues a target create command directly in the Driver, then it makes sense to select that target. That's clearly the user's intent.

But in scripting contexts, I don't think we should be switching around the selected target. Instead, commands run in the script interpreter should be explicit about what execution context they are working on. If I could deprecate one API it would be the HandleCommand with no exe_ctx...

[jingham]

So probably the only safe thing to do is to ensure if the command doesn't specify a target in the execution context, we pick the currently selected one. IIUC, that's the proposal here, so I think this is all good. But we want to be careful not to allow implicit behaviors like this to change the currently selected target/thread/frame.

[jasonmolenda]

Hi all, getting back to this one after six months, hah. Reading through Jim's thoughts on this, I think we're OK to add this refinement. Tatyana also thought that removing this altogether would be an option, but this has been sitting unfixed for a while and I'd like to land the fix to it. Jim wishes for a world where HandleCommand requires an SBExecutionContext, or one in which we had an established way of deprecating old SB API. Given that we can't address that at the moment, I'm going to land this patch finally so we're not carrying the problem around for the foreseeable future.

lldb/source/Interpreter/CommandInterpreter.cpp

     // command output and no command...
     if (EchoCommandNonInteractive(line, io_handler.GetFlags()))
       io_handler.GetOutputStreamFileSP()->Printf(
           "%s%s\n", io_handler.GetPrompt(), line.c_str());
   }
 
   StartHandlingCommand();
 
-  OverrideExecutionContext(m_debugger.GetSelectedExecutionContext());
-  auto finalize = llvm::make_scope_exit([this]() {
-    RestoreExecutionContext();
+  ExecutionContext exe_ctx = m_debugger.GetSelectedExecutionContext();
+  bool pushed_exe_ctx = false;
+  if (exe_ctx.HasTargetScope()) {
+    OverrideExecutionContext(exe_ctx);
+    pushed_exe_ctx = true;
+  }
+  auto finalize = llvm::make_scope_exit([this, pushed_exe_ctx]() {
+    if (pushed_exe_ctx)
+      RestoreExecutionContext();
   });
 
   lldb_private::CommandReturnObject result(m_debugger.GetUseColor());
   HandleCommand(line.c_str(), eLazyBoolCalculate, result);
 
   // Now emit the command output text from the command we just executed
   if ((result.Succeeded() &&
        io_handler.GetFlags().Test(eHandleCommandFlagPrintResult)) ||

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

     offset = cmd_offset + lc.cmdsize;
   }
 
   return image_infos;
 }
 
 bool ObjectFileMachO::LoadCoreFileImages(lldb_private::Process &process) {
   MachOCorefileAllImageInfos image_infos = GetCorefileAllImageInfos();
-  bool added_images = false;
+  ModuleList added_modules;
   if (image_infos.IsValid()) {
     for (const MachOCorefileImageEntry &image : image_infos.all_image_infos) {
       ModuleSpec module_spec;
       module_spec.GetUUID() = image.uuid;
       module_spec.GetFileSpec() = FileSpec(image.filename.c_str());
       if (image.currently_executing) {
         Symbols::DownloadObjectAndSymbolFile(module_spec, true);
         if (FileSystem::Instance().Exists(module_spec.GetFileSpec())) {
           process.GetTarget().GetOrCreateModule(module_spec, false);
         }
       }
       Status error;
       ModuleSP module_sp =
           process.GetTarget().GetOrCreateModule(module_spec, false, &error);
       if (!module_sp.get() || !module_sp->GetObjectFile()) {
         if (image.load_address != LLDB_INVALID_ADDRESS) {
           module_sp = process.ReadModuleFromMemory(module_spec.GetFileSpec(),
                                                    image.load_address);
         }
       }
       if (module_sp.get()) {
-        added_images = true;
+        added_modules.Append(module_sp);
         for (auto name_vmaddr_tuple : image.segment_load_addresses) {
           SectionList *sectlist = module_sp->GetObjectFile()->GetSectionList();
           if (sectlist) {
             SectionSP sect_sp =
                 sectlist->FindSectionByName(std::get<0>(name_vmaddr_tuple));
             if (sect_sp) {
               process.GetTarget().SetSectionLoadAddress(
                   sect_sp, std::get<1>(name_vmaddr_tuple));
             }
           }
         }
       }
     }
   }
-  return added_images;
+  process.GetTarget().ModulesDidLoad(added_modules);
+  return added_modules.GetSize() > 0;
 }

lldb/test/API/macosx/dsym-script-settings-corefile/Makefile

+C_SOURCES := main.c
+
+include Makefile.rules

lldb/test/API/macosx/dsym-script-settings-corefile/TestdSYMScriptSettingsCorefile.py

+"""Test that settings in dSYM Python scripts are registered when read from corefile."""
+
+import os
+import re
+import subprocess
+import shutil
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestdSYMScriptSettingsCorefile(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @no_debug_info_test
+    @skipUnlessDarwin
+    @skipIfOutOfTreeDebugserver  # newer debugserver required for these qMemoryRegionInfo types
+    def test(self):
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+        dsym = self.getBuildArtifact("a.out.dSYM")
+        python_resources_dir = os.path.join(dsym, "Contents", "Resources", "Python")
+        python_file = os.path.join(self.getSourceDir(), "a_out.py")
+        osplugin_python_file = os.path.join(self.getSourceDir(), "operating_system.py")
+        lldbutil.mkdir_p(python_resources_dir)
+        shutil.copyfile(python_file, os.path.join(python_resources_dir, "a_out.py"))
+        shutil.copyfile(osplugin_python_file, os.path.join(python_resources_dir, "operating_system.py"))
+
+        corefile = self.getBuildArtifact("process.core")
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+                self, "// break here", lldb.SBFileSpec("main.c"))
+
+        self.runCmd("process save-core -s stack " + corefile)
+
+        ci = self.dbg.GetCommandInterpreter()
+
+        result = lldb.SBCommandReturnObject()
+        ci.HandleCommand("settings show target.process.memory-cache-line-size", result)
+        init_value_memory_cache_line = re.search(" = ([0-9]+)\s*$", result.GetOutput()).group(1)
+        self.assertEqual(init_value_memory_cache_line, "512")
+
+        result = lldb.SBCommandReturnObject()
+        ci.HandleCommand("settings show target.default-arch", result)
+        self.assertEqual(re.search(" = (.+)\s*$", result.GetOutput()), None)
+
+        result = lldb.SBCommandReturnObject()
+        ci.HandleCommand("settings show target.prefer-dynamic-value", result)
+        init_value_prefer_dynamic_value = re.search(" = (.+)\s*$", result.GetOutput()).group(1)
+        # command driver lldb will have no-run-target;
+        # SB API test will have no-dynamic-values.
+        # the custom python sets it to run-target, so just check that 
+        # it's not equal to that.
+        self.assertNotEqual(init_value_prefer_dynamic_value, "run-target")
+
+        process.Kill()
+        self.dbg.DeleteTarget(target)
+
+
+        self.runCmd("settings set target.load-script-from-symbol-file true", check=False)
+        def cleanup():
+            self.runCmd("settings set target.load-script-from-symbol-file false", check=False)
+            self.runCmd("settings set target.process.memory-cache-line-size " + init_value_memory_cache_line)
+            self.runCmd("settings clear target.default-arch")
+            self.runCmd("settings set target.prefer-dynamic-value " + init_value_prefer_dynamic_value)
+            self.runCmd("settings clear target.process.python-os-plugin-path")
+        self.addTearDownHook(cleanup)
+
+        if self.TraceOn():
+          self.runCmd("settings show target.process.memory-cache-line-size target.default-arch target.prefer-dynamic-value target.process.python-os-plugin-path")
+
+        # Now load the corefile
+        target = self.dbg.CreateTarget('')
+        process = target.LoadCore(corefile)
+
+        result = lldb.SBCommandReturnObject()
+        ci.HandleCommand("settings show target.process.memory-cache-line-size", result)
+        new_init_value_memory_cache_line = re.search(" = ([0-9]+)\s*$", result.GetOutput()).group(1)
+        self.assertEqual(new_init_value_memory_cache_line, "16")
+
+        result = lldb.SBCommandReturnObject()
+        ci.HandleCommand("settings show target.default-arch", result)
+        new_default_arch = re.search(" = (.+)\s*$", result.GetOutput()).group(1)
+        self.assertEqual(new_default_arch, "x86_64")
+
+        result = lldb.SBCommandReturnObject()
+        ci.HandleCommand("settings show target.prefer-dynamic-value", result)
+        new_prefer_dynamic_value = re.search(" = (.+)\s*$", result.GetOutput()).group(1)
+        self.assertEqual(new_prefer_dynamic_value, "run-target")
+
+        if self.TraceOn():
+          self.runCmd("settings show target.process.memory-cache-line-size target.default-arch target.prefer-dynamic-value target.process.python-os-plugin-path")
+          self.runCmd("thread list")
+

lldb/test/API/macosx/dsym-script-settings-corefile/a_out.py

+def __lldb_init_module(debugger, internal_dict):
+    print("")
+    print("")
+    # Process settings will be set in the template
+    set1 = "settings set target.process.memory-cache-line-size 16"
+    print("Process setting:")
+    print(set1)
+    debugger.HandleCommand(set1)
+
+    # Global target settings will be set in the template
+    set2 = "sett set target.default-arch x86_64"
+    print("")
+    print("Global target setting:")
+    print(set2)
+    debugger.HandleCommand(set2)
+
+    # Target specific settings need a Target to set in
+    set3 = "settings set target.prefer-dynamic-value run-target"
+    print("")
+    print("Target specific setting:")
+    print(set3)
+    debugger.HandleCommand(set3)
+    
+    self_path = "{}".format(__file__)
+    base_dir_name = self_path[:self_path.rfind("/")]
+    os_plugin = base_dir_name + "/operating_system.py"
+    set4 = "settings set target.process.python-os-plugin-path \"%s\"" % os_plugin
+    print("")
+    print("OS Plugin setting:")
+    print(set4)
+    debugger.HandleCommand(set4)
+
+    print("")
+    print("")

lldb/test/API/macosx/dsym-script-settings-corefile/main.c

+#include <stdio.h>
+int main() {
+  puts ("HI corefile"); // break here
+}

lldb/test/API/macosx/dsym-script-settings-corefile/operating_system.py

+import lldb
+
+
+class OperatingSystemPlugIn(object):
+    """Class that provides data for an instance of a LLDB 'OperatingSystemPython' plug-in class"""
+
+    def __init__(self, process):
+        '''Initialization needs a valid.SBProcess object.
+
+        This plug-in will get created after a live process is valid and has stopped for the first time.
+        '''
+        self.process = None
+        self.registers = None
+        self.threads = None
+        if isinstance(process, lldb.SBProcess) and process.IsValid():
+            self.process = process
+            self.threads = None  # Will be an dictionary containing info for each thread
+
+    def get_target(self):
+        return self.process.target
+
+    def get_thread_info(self):
+        if not self.threads:
+            # FIXME: LLDB is not actually parsing thread stop reasons.
+            self.threads = [{
+                'tid': 0x111111111,
+                'name': 'one',
+                'queue': 'queue1',
+                'state': 'stopped',
+                'stop_reason': 'not parsed'
+            }, {
+                'tid': 0x222222222,
+                'name': 'two',
+                'queue': 'queue2',
+                'state': 'stopped',
+                'stop_reason': 'not parsed'
+            }, {
+                'tid': 0x333333333,
+                'name': 'three',
+                'queue': 'queue3',
+                'state': 'stopped',
+                'stop_reason': 'not parsed - should be "sigstop" though',
+                'core': 0
+            }]
+        return self.threads

lldb/test/Shell/ScriptInterpreter/Python/OS/Inputs/a_out.py

+def __lldb_init_module(debugger, internal_dict):
+    print("")
+    print("")
+    # Process settings will be set in the template
+    set1 = "settings set target.process.memory-cache-line-size 16"
+    print("Process setting:")
+    print(set1)
+    debugger.HandleCommand(set1)
+
+    # Global target settings will be set in the template
+    set2 = "sett set target.default-arch x86_64"
+    print("")
+    print("Global target setting:")
+    print(set2)
+    debugger.HandleCommand(set2)
+
+    # Target specific settings need a Target to set in
+    set3 = "settings set target.prefer-dynamic-value run-target"
+    print("")
+    print("Target specific setting:")
+    print(set3)
+    debugger.HandleCommand(set3)
+    
+    self_path = "{}".format(__file__)
+    base_dir_name = self_path[:self_path.rfind("/")]
+    os_plugin = base_dir_name + "/operating_system.py"
+    set4 = "settings set target.process.python-os-plugin-path \"%s\"" % os_plugin
+    print("")
+    print("OS Plugin setting:")
+    print(set4)
+    debugger.HandleCommand(set4)
+
+    print("")
+    print("")

lldb/test/Shell/ScriptInterpreter/Python/OS/Inputs/main.c

+#include <stdio.h>
+int main() {
+  puts ("Hello, corefile"); // break here
+}

lldb/test/Shell/ScriptInterpreter/Python/OS/Inputs/operating_system.py

+import lldb
+
+
+class OperatingSystemPlugIn(object):
+    """Class that provides data for an instance of a LLDB 'OperatingSystemPython' plug-in class"""
+
+    def __init__(self, process):
+        '''Initialization needs a valid.SBProcess object.
+
+        This plug-in will get created after a live process is valid and has stopped for the first time.
+        '''
+        self.process = None
+        self.registers = None
+        self.threads = None
+        if isinstance(process, lldb.SBProcess) and process.IsValid():
+            self.process = process
+            self.threads = None  # Will be an dictionary containing info for each thread
+
+    def get_target(self):
+        return self.process.target
+
+    def get_thread_info(self):
+        if not self.threads:
+            # FIXME: LLDB is not actually parsing thread stop reasons.
+            self.threads = [{
+                'tid': 0x111111111,
+                'name': 'one',
+                'queue': 'queue1',
+                'state': 'stopped',
+                'stop_reason': 'not parsed'
+            }, {
+                'tid': 0x222222222,
+                'name': 'two',
+                'queue': 'queue2',
+                'state': 'stopped',
+                'stop_reason': 'not parsed'
+            }, {
+                'tid': 0x333333333,
+                'name': 'three',
+                'queue': 'queue3',
+                'state': 'stopped',
+                'stop_reason': 'not parsed - should be "sigstop" though',
+                'core': 0
+            }]
+        return self.threads

lldb/test/Shell/ScriptInterpreter/Python/OS/os.test

+# REQUIRES: system-darwin
+
+# RUN: %clang_host %S/Inputs/main.c -g -o %t.out
+# RUN: mkdir -p %t.out.dSYM/Contents/Resources/Python
+# RUN: cp %S/Inputs/a_out.py %t.out.dSYM/Contents/Resources/Python/os_test_tmp_out.py
+# RUN: cp %S/Inputs/operating_system.py  %t.out.dSYM/Contents/Resources/Python/
+
+# RUN: %lldb -x %t.out \
+# RUN:    -o 'br s -p break' \
+# RUN:    -o 'r' \
+# RUN:    -o 'pro save-core -s stack %t.corefile' \
+# RUN:    -o 'pro kill' \
+# RUN:    -o 'tar del' \
+# RUN:    -o 'settings set target.load-script-from-symbol-file true' \
+# RUN:    -o 'tar cr -c %t.corefile' \
+# RUN:    -o 'sett show target.process.memory-cache-line-size target.default-arch target.prefer-dynamic-value target.process.python-os-plugin-path' \
+# RUN:    -o 'thread list' | FileCheck %s
+
+# CHECK: target.process.memory-cache-line-size (unsigned) = 16
+# CHECK: target.default-arch (arch) = x86_64
+# CHECK: target.prefer-dynamic-value (enum) = run-target
+# CHECK: name = 'one', queue = 'queue1'
+# CHECK: name = 'two', queue = 'queue2'