[lldb] Add --stack option to `target symbols add` command

[JDevlieghere]

Summary

Currently you can ask the target symbols add command to locate the debug symbols for the current frame. This patch add an options to do that for the whole call stack.

[kastiglione]

lldb/source/Commands/CommandObjectTarget.cpp

4268–4271

Is this missing a return?

4281–4282

we don't have an iterator for this?

[kastiglione]

lldb/source/Commands/CommandObjectTarget.cpp

4301–4302

do you need the separate variable? can it be:

flush |= true;

[JDevlieghere]

lldb/source/Commands/CommandObjectTarget.cpp

4268–4271

Yep!

4281–4282

Not currently, no. I looked at StackFrameList in case it was just a matter of adding a LockingAdaptedIterable, but unsurprisingly, stack frames are computed rather than stored in a list. We'd need to implement a custom iterator that keeps track of the index and calls GetStackFrameAtIndex under the hood, which is definitely something for a separate patch.

4301–4302

I want flush to be true only if at least one frame requires a flush.

[kastiglione]

lldb/source/Commands/CommandObjectTarget.cpp

4301–4302

my suggestion still achieves that.

Instead of:

bool current_frame_flush = false;
if (DownloadObjectAndSymbolFile(module_spec, target, result,
                                current_frame_flush))
  symbols_found = true;

flush |= current_frame_flush;

Inline the current_frame_flush variable:

if (DownloadObjectAndSymbolFile(module_spec, target, result,
                                current_frame_flush))
  flush |= true;

This makes flush only true when one frame needs it.

[kastiglione]

lldb/source/Commands/CommandObjectTarget.cpp

4300

It seems this is call is synchronous? A separate improvement would be to perform the unique downloads in parallel.

[JDevlieghere]

lldb/source/Commands/CommandObjectTarget.cpp

4301–4302

That relies on the assumption that flush is true iff DownloadObjectAndSymbolFilereturn true (and the same for AddModuleSymbols). Looking at the code that does indeed seem to be the case, but then I'd rather see the API refactored to remove the flush output variable instead of relying on an implementation-specific assumption here.

[shafik]

Does it make sense to add a test for this? It looks like we have two basic tests for add-dsym already.

[JDevlieghere]

• Rebase
• Add test

[kastiglione]

lldb/source/Commands/CommandObjectTarget.cpp

4301–4302

thanks, makes sense. I had overlooked that current_frame_flush is also an output parameter.

lldb/source/Commands/CommandObjectTarget.cpp

CommandObjectTargetSymbolsAdd(CommandInterpreter &interpreter)
         m_option_group(),
         m_file_option(
             LLDB_OPT_SET_1, false, "shlib", 's',
             CommandCompletions::eModuleCompletion, eArgTypeShlibName,
             "Locate the debug symbols for the shared library specified by "
             "name."),
         m_current_frame_option(
             LLDB_OPT_SET_2, false, "frame", 'F',
-            "Locate the debug symbols for the currently selected frame.",
+            "Locate the debug symbols for the currently selected frame.", false,
+            true),
+        m_current_stack_option(LLDB_OPT_SET_2, false, "stack", 'S',
+                               "Locate the debug symbols for every frame in "
+                               "the current call stack.",
                                false, true)
 
   {
     m_option_group.Append(&m_uuid_option_group, LLDB_OPT_SET_ALL,
                           LLDB_OPT_SET_1);
     m_option_group.Append(&m_file_option, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1);
     m_option_group.Append(&m_current_frame_option, LLDB_OPT_SET_2,
                           LLDB_OPT_SET_2);
+    m_option_group.Append(&m_current_stack_option, LLDB_OPT_SET_2,
+                          LLDB_OPT_SET_2);
     m_option_group.Finalize();
   }
 
   ~CommandObjectTargetSymbolsAdd() override = default;
 
   void
   HandleArgumentCompletion(CompletionRequest &request,
                            OptionElementVector &opt_element_vector) override {
bool AddSymbolsForFrame(CommandReturnObject &result, bool &flush) {
     if (!DownloadObjectAndSymbolFile(module_spec, result, flush)) {
       result.AppendError("unable to find debug symbols for the current frame");
       return false;
     }
 
     return true;
   }
 
+  bool AddSymbolsForStack(CommandReturnObject &result, bool &flush) {
+    assert(m_current_stack_option.GetOptionValue().OptionWasSet());
+
+    Process *process = m_exe_ctx.GetProcessPtr();
+    if (!process) {
+      result.AppendError(
+          "a process must exist in order to use the --stack option");
+      return false;
+    }
+
+    const StateType process_state = process->GetState();
+    if (!StateIsStoppedState(process_state, true)) {
+      result.AppendErrorWithFormat("process is not stopped: %s",
+                                   StateAsCString(process_state));
+      return false;
+    }
+
+    Thread *thread = m_exe_ctx.GetThreadPtr();
+    if (!thread) {
+      result.AppendError("invalid current thread");
+      return false;
+    }
+
+    bool symbols_found = false;
+    uint32_t frame_count = thread->GetStackFrameCount();
+    for (uint32_t i = 0; i < frame_count; ++i) {
+      lldb::StackFrameSP frame_sp = thread->GetStackFrameAtIndex(i);
+
+      ModuleSP frame_module_sp(
+          frame_sp->GetSymbolContext(eSymbolContextModule).module_sp);
+      if (!frame_module_sp)
+        continue;
+
+      ModuleSpec module_spec;
+      module_spec.GetUUID() = frame_module_sp->GetUUID();
+
+      if (FileSystem::Instance().Exists(
+              frame_module_sp->GetPlatformFileSpec())) {
+        module_spec.GetArchitecture() = frame_module_sp->GetArchitecture();
+        module_spec.GetFileSpec() = frame_module_sp->GetPlatformFileSpec();
+      }
+
+      bool current_frame_flush = false;
+      if (DownloadObjectAndSymbolFile(module_spec, result, current_frame_flush))
+        symbols_found = true;
+      flush |= current_frame_flush;
+    }
+
+    if (!symbols_found) {
+      result.AppendError(
+          "unable to find debug symbols in the current call stack");
+      return false;
+    }
+
+    return true;
+  }
+
   bool DoExecute(Args &args, CommandReturnObject &result) override {
     Target *target = m_exe_ctx.GetTargetPtr();
     result.SetStatus(eReturnStatusFailed);
     bool flush = false;
     ModuleSpec module_spec;
     const bool uuid_option_set =
         m_uuid_option_group.GetOptionValue().OptionWasSet();
     const bool file_option_set = m_file_option.GetOptionValue().OptionWasSet();
     const bool frame_option_set =
         m_current_frame_option.GetOptionValue().OptionWasSet();
+    const bool stack_option_set =
+        m_current_stack_option.GetOptionValue().OptionWasSet();
     const size_t argc = args.GetArgumentCount();
 
     if (argc == 0) {
       if (uuid_option_set)
         AddSymbolsForUUID(result, flush);
       else if (file_option_set)
         AddSymbolsForFile(result, flush);
       else if (frame_option_set)
         AddSymbolsForFrame(result, flush);
+      else if (stack_option_set)
+        AddSymbolsForStack(result, flush);
       else
         result.AppendError("one or more symbol file paths must be specified, "
                            "or options must be specified");
     } else {
       if (uuid_option_set) {
         result.AppendError("specify either one or more paths to symbol files "
                            "or use the --uuid option without arguments");
       } else if (frame_option_set) {
bool DoExecute(Args &args, CommandReturnObject &result) override {
     }
     return result.Succeeded();
   }
 
   OptionGroupOptions m_option_group;
   OptionGroupUUID m_uuid_option_group;
   OptionGroupFile m_file_option;
   OptionGroupBoolean m_current_frame_option;
+  OptionGroupBoolean m_current_stack_option;
 };
 
 #pragma mark CommandObjectTargetSymbols
 
 // CommandObjectTargetSymbols
 
 class CommandObjectTargetSymbols : public CommandObjectMultiword {
 public:

lldb/test/API/macosx/add-dsym/TestAddDsymDownload.py

+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+@skipUnlessDarwin
+class AddDsymDownload(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+    dwarfdump_uuid_regex = re.compile('UUID: ([-0-9a-fA-F]+) \(([^\(]+)\) .*')
+
+    def get_uuid(self):
+        dwarfdump_cmd_output = subprocess.check_output(
+            ('/usr/bin/dwarfdump --uuid "%s"' % self.exe),
+            shell=True).decode("utf-8")
+        for line in dwarfdump_cmd_output.splitlines():
+            match = self.dwarfdump_uuid_regex.search(line)
+            if match:
+                return match.group(1)
+        return None
+
+    def create_dsym_for_uuid(self):
+        shell_cmds = [
+            '#! /bin/sh', '# the last argument is the uuid',
+            'while [ $# -gt 1 ]', 'do', '  shift', 'done', 'ret=0',
+            'echo "<?xml version=\\"1.0\\" encoding=\\"UTF-8\\"?>"',
+            'echo "<!DOCTYPE plist PUBLIC \\"-//Apple//DTD PLIST 1.0//EN\\" \\"http://www.apple.com/DTDs/PropertyList-1.0.dtd\\">"',
+            'echo "<plist version=\\"1.0\\">"', '',
+            'if [ "$1" != "%s" ]' % (self.uuid), 'then',
+            '  echo "<key>DBGError</key><string>not found</string>"',
+            '  echo "</plist>"', '  exit 1', 'fi',
+            '  uuid=%s' % self.uuid,
+            '  bin=%s' % self.exe,
+            '  dsym=%s' % self.dsym, 'echo "<dict><key>$uuid</key><dict>"', '',
+            'echo "<key>DBGDSYMPath</key><string>$dsym</string>"',
+            'echo "<key>DBGSymbolRichExecutable</key><string>$bin</string>"',
+            'echo "</dict></dict></plist>"', 'exit $ret'
+        ]
+
+        with open(self.dsym_for_uuid, "w") as writer:
+            for l in shell_cmds:
+                writer.write(l + '\n')
+
+        os.chmod(self.dsym_for_uuid, 0o755)
+
+    def setUp(self):
+        TestBase.setUp(self)
+        self.source = 'main.c'
+        self.exe = self.getBuildArtifact("a.out")
+        self.dsym = os.path.join(
+            self.getBuildDir(),
+            "hide.app/Contents/a.out.dSYM/Contents/Resources/DWARF/",
+            os.path.basename(self.exe))
+        self.dsym_for_uuid = self.getBuildArtifact("dsym-for-uuid.sh")
+
+        self.buildDefault(dictionary={'MAKE_DSYM': 'YES'})
+        self.assertTrue(os.path.exists(self.exe))
+        self.assertTrue(os.path.exists(self.dsym))
+
+        self.uuid = self.get_uuid()
+        self.assertNotEqual(self.uuid, None, "Could not get uuid for a.out")
+
+        self.create_dsym_for_uuid()
+
+        os.environ['LLDB_APPLE_DSYMFORUUID_EXECUTABLE'] = self.dsym_for_uuid
+        self.addTearDownHook(
+            lambda: os.environ.pop('LLDB_APPLE_DSYMFORUUID_EXECUTABLE', None))
+
+    def do_test(self, command):
+        self.target = self.dbg.CreateTarget(self.exe)
+        self.assertTrue(self.target, VALID_TARGET)
+
+        main_bp = self.target.BreakpointCreateByName("main", "a.out")
+        self.assertTrue(main_bp, VALID_BREAKPOINT)
+
+        self.process = self.target.LaunchSimple(
+            None, None, self.get_process_working_directory())
+        self.assertTrue(self.process, PROCESS_IS_VALID)
+
+        # The stop reason of the thread should be breakpoint.
+        self.assertEquals(self.process.GetState(), lldb.eStateStopped,
+                          STOPPED_DUE_TO_BREAKPOINT)
+
+        self.runCmd(command)
+        self.expect("frame select", substrs=['a.out`main at main.c'])
+
+    @no_debug_info_test
+    def test_frame(self):
+        self.do_test("add-dsym --frame")
+
+    @no_debug_info_test
+    def test_uuid(self):
+        self.do_test("add-dsym --uuid {}".format(self.uuid))
+
+    @no_debug_info_test
+    def test_stack(self):
+        self.do_test("add-dsym --stack")