[Lldb-commits] [PATCH] D35607: Extend 'target symbols add' to set symbol file for a given module

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

> Greg, are you with me checking this in?

Sure thing


https://reviews.llvm.org/D35607



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D35607: Extend 'target symbols add' to set symbol file for a given module

[Eugene Zemtsov via Phabricator via lldb-commits]

eugene updated this revision to Diff 107725.
eugene added a comment.

Added error message when more than one symbol file is provided.


https://reviews.llvm.org/D35607

Files:
  packages/Python/lldbsuite/test/linux/add-symbols/Makefile
  
packages/Python/lldbsuite/test/linux/add-symbols/TestTargetSymbolsAddCommand.py
  packages/Python/lldbsuite/test/linux/add-symbols/main.c
  source/Commands/CommandObjectTarget.cpp
  source/Interpreter/CommandObject.cpp

Index: source/Interpreter/CommandObject.cpp
===
--- source/Interpreter/CommandObject.cpp
+++ source/Interpreter/CommandObject.cpp
@@ -57,7 +57,7 @@
 llvm::StringRef CommandObject::GetHelpLong() { return m_cmd_help_long; }
 
 llvm::StringRef CommandObject::GetSyntax() {
-  if (m_cmd_syntax.empty())
+  if (!m_cmd_syntax.empty())
 return m_cmd_syntax;
 
   StreamString syntax_str;
Index: source/Commands/CommandObjectTarget.cpp
===
--- source/Commands/CommandObjectTarget.cpp
+++ source/Commands/CommandObjectTarget.cpp
@@ -3969,7 +3969,8 @@
 "Add a debug symbol file to one of the target's current modules by "
 "specifying a path to a debug symbols file, or using the options "
 "to specify a module to download symbols for.",
-"target symbols add []", eCommandRequiresTarget),
+"target symbols add  []",
+eCommandRequiresTarget),
 m_option_group(),
 m_file_option(
 LLDB_OPT_SET_1, false, "shlib", 's',
@@ -4289,18 +4290,22 @@
   if (uuid_option_set) {
 result.AppendError("specify either one or more paths to symbol files "
"or use the --uuid option without arguments");
-  } else if (file_option_set) {
-result.AppendError("specify either one or more paths to symbol files "
-   "or use the --file option without arguments");
   } else if (frame_option_set) {
 result.AppendError("specify either one or more paths to symbol files "
"or use the --frame option without arguments");
+  } else if (file_option_set && argc > 1) {
+result.AppendError("specify at most one symbol file path when "
+   "--shlib option is set");
   } else {
 PlatformSP platform_sp(target->GetPlatform());
 
 for (auto  : args.entries()) {
   if (!entry.ref.empty()) {
 module_spec.GetSymbolFileSpec().SetFile(entry.ref, true);
+if (file_option_set) {
+  module_spec.GetFileSpec() =
+  m_file_option.GetOptionValue().GetCurrentValue();
+}
 if (platform_sp) {
   FileSpec symfile_spec;
   if (platform_sp
Index: packages/Python/lldbsuite/test/linux/add-symbols/main.c
===
--- /dev/null
+++ packages/Python/lldbsuite/test/linux/add-symbols/main.c
@@ -0,0 +1,6 @@
+#include 
+static int var = 5;
+int main() {
+  printf("%p is %d\n", , var);
+  return ++var;
+}
Index: packages/Python/lldbsuite/test/linux/add-symbols/TestTargetSymbolsAddCommand.py
===
--- /dev/null
+++ packages/Python/lldbsuite/test/linux/add-symbols/TestTargetSymbolsAddCommand.py
@@ -0,0 +1,52 @@
+""" Testing explicit symbol loading via target symbols add. """
+import os
+import time
+import lldb
+import sys
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TargetSymbolsAddCommand(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def setUp(self):
+TestBase.setUp(self)
+self.source = 'main.c'
+
+@no_debug_info_test  # Prevent the genaration of the dwarf version of this test
+@skipUnlessPlatform(['linux'])
+def test_target_symbols_add(self):
+"""Test that 'target symbols add' can load the symbols
+even if gnu.build-id and gnu_debuglink are not present in the module.
+Similar to test_add_dsym_mid_execution test for macos."""
+self.build(clean=True)
+exe = os.path.join(os.getcwd(), "stripped.out")
+
+self.target = self.dbg.CreateTarget(exe)
+self.assertTrue(self.target, VALID_TARGET)
+
+main_bp = self.target.BreakpointCreateByName("main", "stripped.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.assertTrue(self.process.GetState() == lldb.eStateStopped,
+STOPPED_DUE_TO_BREAKPOINT)
+
+exe_module = self.target.GetModuleAtIndex(0)
+
+# Check that 

[Lldb-commits] [PATCH] D35607: Extend 'target symbols add' to set symbol file for a given module

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Looks fine, just add an error that checks that when the --file options is used, 
that only one argument (symbol file) is specified.




Comment at: source/Commands/CommandObjectTarget.cpp:4302-4305
+if (file_option_set) {
+  module_spec.GetFileSpec() =
+  m_file_option.GetOptionValue().GetCurrentValue();
+}

We should verify that if the file option is set, then there is only one symbol 
file argument as it wouldn't make sense to do:

```
(lldb) target symbols add -- file a.out a.out.symbols b.out.symbols
```


https://reviews.llvm.org/D35607



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D35607: Extend 'target symbols add' to set symbol file for a given module

[Eugene Zemtsov via Phabricator via lldb-commits]

eugene updated this revision to Diff 107628.
eugene retitled this revision from "Extend 'target symbols add' to load symbols 
from a given file by UUID." to "Extend 'target symbols add' to set symbol file 
for a given module".
eugene edited the summary of this revision.
eugene added a comment.

Found a way to implement what I need much easily and switched to using file 
names instead of UUID.

Please take another look.


https://reviews.llvm.org/D35607

Files:
  packages/Python/lldbsuite/test/linux/add-symbols/Makefile
  
packages/Python/lldbsuite/test/linux/add-symbols/TestTargetSymbolsAddCommand.py
  packages/Python/lldbsuite/test/linux/add-symbols/main.c
  source/Commands/CommandObjectTarget.cpp
  source/Interpreter/CommandObject.cpp

Index: source/Interpreter/CommandObject.cpp
===
--- source/Interpreter/CommandObject.cpp
+++ source/Interpreter/CommandObject.cpp
@@ -57,7 +57,7 @@
 llvm::StringRef CommandObject::GetHelpLong() { return m_cmd_help_long; }
 
 llvm::StringRef CommandObject::GetSyntax() {
-  if (m_cmd_syntax.empty())
+  if (!m_cmd_syntax.empty())
 return m_cmd_syntax;
 
   StreamString syntax_str;
Index: source/Commands/CommandObjectTarget.cpp
===
--- source/Commands/CommandObjectTarget.cpp
+++ source/Commands/CommandObjectTarget.cpp
@@ -3969,7 +3969,8 @@
 "Add a debug symbol file to one of the target's current modules by "
 "specifying a path to a debug symbols file, or using the options "
 "to specify a module to download symbols for.",
-"target symbols add []", eCommandRequiresTarget),
+"target symbols add  []",
+eCommandRequiresTarget),
 m_option_group(),
 m_file_option(
 LLDB_OPT_SET_1, false, "shlib", 's',
@@ -4289,9 +4290,6 @@
   if (uuid_option_set) {
 result.AppendError("specify either one or more paths to symbol files "
"or use the --uuid option without arguments");
-  } else if (file_option_set) {
-result.AppendError("specify either one or more paths to symbol files "
-   "or use the --file option without arguments");
   } else if (frame_option_set) {
 result.AppendError("specify either one or more paths to symbol files "
"or use the --frame option without arguments");
@@ -4301,6 +4299,10 @@
 for (auto  : args.entries()) {
   if (!entry.ref.empty()) {
 module_spec.GetSymbolFileSpec().SetFile(entry.ref, true);
+if (file_option_set) {
+  module_spec.GetFileSpec() =
+  m_file_option.GetOptionValue().GetCurrentValue();
+}
 if (platform_sp) {
   FileSpec symfile_spec;
   if (platform_sp
Index: packages/Python/lldbsuite/test/linux/add-symbols/main.c
===
--- /dev/null
+++ packages/Python/lldbsuite/test/linux/add-symbols/main.c
@@ -0,0 +1,6 @@
+#include 
+static int var = 5;
+int main() {
+  printf("%p is %d\n", , var);
+  return ++var;
+}
Index: packages/Python/lldbsuite/test/linux/add-symbols/TestTargetSymbolsAddCommand.py
===
--- /dev/null
+++ packages/Python/lldbsuite/test/linux/add-symbols/TestTargetSymbolsAddCommand.py
@@ -0,0 +1,52 @@
+""" Testing explicit symbol loading via target symbols add. """
+import os
+import time
+import lldb
+import sys
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TargetSymbolsAddCommand(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def setUp(self):
+TestBase.setUp(self)
+self.source = 'main.c'
+
+@no_debug_info_test  # Prevent the genaration of the dwarf version of this test
+@skipUnlessPlatform(['linux'])
+def test_target_symbols_add(self):
+"""Test that 'target symbols add' can load the symbols
+even if gnu.build-id and gnu_debuglink are not present in the module.
+Similar to test_add_dsym_mid_execution test for macos."""
+self.build(clean=True)
+exe = os.path.join(os.getcwd(), "stripped.out")
+
+self.target = self.dbg.CreateTarget(exe)
+self.assertTrue(self.target, VALID_TARGET)
+
+main_bp = self.target.BreakpointCreateByName("main", "stripped.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.assertTrue(self.process.GetState() == lldb.eStateStopped,
+STOPPED_DUE_TO_BREAKPOINT)
+
+exe_module =