[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I don't see myself using this command personally, but I agree that splitting 
the "disable stats collection" and "dump accumulated stats" into two actions 
seems a better choice (maybe disable could do a final auto-dump though). I also 
think the header/footer is not needed for a primarily interactive tool. If we 
have a command that dumps so much data that the user cannot tell where the 
output starts and ends then we're doing something wrong (PS: I wish we had 
automatic pagination for the backtrace command).

I am also wondering whether statistics should be a top-level command, or should 
we put it under "log". We already have "log timers enable/disable/...", which 
are doing something similar to this, so it seems like a good idea to move these 
two things closer together.




Comment at: lldb/packages/Python/lldbsuite/test/functionalities/stats/main.c:12
+  //%self.expect("stats enable")
+  //%self.expect("frame var", substrs=['27'])
+  //%self.expect("stats disable", substrs=['frame var successes : 1', 'frame 
var failures : 0'])

Maybe test for failure as well (I guess trying to print an non-existing 
variable should do the trick)?



Comment at: lldb/source/Commands/CommandObjectStats.cpp:31-35
+if (!target) {
+  result.AppendError("stats command needs a target");
+  result.SetStatus(eReturnStatusFailed);
+  return false;
+}

I think we have some way (should involve `eCommandRequiresTarget`) to avoid the 
need for checking this.



Comment at: lldb/source/Commands/CommandObjectStats.cpp:43-46
+target->RegisterStats(StatisticKind::ExpressionSuccessful);
+target->RegisterStats(StatisticKind::ExpressionFailure);
+target->RegisterStats(StatisticKind::FrameVarSuccess);
+target->RegisterStats(StatisticKind::FrameVarFailure);

Are you planning to have some kind of decentralized method of registering 
statistics?
If not, then I don't see a need for explicitly registering the statistic kinds 
here, since the single source of truth about the available kinds can be 
StatisticKind enum, and Target can just get the list from there automatically 
when need (when stats are enabled?)

This way we could even simplify the code by avoiding the "statistic is not 
registered but someone still called IncrementStats" state altogether) and the 
stats map could be a simple array with NumStatistics elements.


https://reviews.llvm.org/D45547



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

[Lldb-commits] [lldb] r329889 - [dotest] Use in-tree dsymutil on Darwin

[Jonas Devlieghere via lldb-commits]

Author: jdevlieghere
Date: Thu Apr 12 02:25:32 2018
New Revision: 329889

URL: http://llvm.org/viewvc/llvm-project?rev=329889&view=rev
Log:
[dotest] Use in-tree dsymutil on Darwin

Summary:
With the upstream implementation of dsymutil containing almost all
functionality from the one shipped with Xcode, we want to use the
in-tree version for running the test suite.

This will also allow us to re-enable TestUnicodeSymbols which was
failing because of the discrepancy in how Unicode symbols were hashed in
lldb and older versions of dsymutil.

Reviewers: aprantl, davide, jingham, labath

Subscribers: mgorny, llvm-commits, lldb-commits

Differential Revision: https://reviews.llvm.org/D45518

Modified:
lldb/trunk/packages/Python/lldbsuite/test/dotest.py
lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py
lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules
lldb/trunk/test/CMakeLists.txt

Modified: lldb/trunk/packages/Python/lldbsuite/test/dotest.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/dotest.py?rev=329889&r1=329888&r2=329889&view=diff
==
--- lldb/trunk/packages/Python/lldbsuite/test/dotest.py (original)
+++ lldb/trunk/packages/Python/lldbsuite/test/dotest.py Thu Apr 12 02:25:32 2018
@@ -301,6 +301,12 @@ def parseOptionsAndInitTestdirs():
 configuration.compiler = candidate
 break
 
+if args.dsymutil:
+  os.environ['DSYMUTIL'] = args.dsymutil
+else if platform_system == 'Darwin':
+  os.environ['DSYMUTIL'] = seven.get_command_output(
+  'xcrun -find -toolchain default dsymutil')
+
 if args.channels:
 lldbtest_config.channels = args.channels
 

Modified: lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py?rev=329889&r1=329888&r2=329889&view=diff
==
--- lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py (original)
+++ lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py Thu Apr 12 
02:25:32 2018
@@ -83,6 +83,8 @@ def create_parser():
 help=textwrap.dedent('''Specify the extra flags to be passed to the 
toolchain when building the inferior programs to be debugged
suggestions: do not 
lump the "-A arch1 -A arch2" together such that the -E option applies to only 
one of the architectures'''))
 
+group.add_argument('--dsymutil', metavar='dsymutil', dest='dsymutil', 
help=textwrap.dedent('Specify Which dsymutil to use.'))
+
 # Test filtering options
 group = parser.add_argument_group('Test filtering options')
 group.add_argument(

Modified: lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules?rev=329889&r1=329888&r2=329889&view=diff
==
--- lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules (original)
+++ lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules Thu Apr 12 
02:25:32 2018
@@ -147,7 +147,7 @@ ARCHFLAG ?= -arch
 # Change any build/tool options needed
 #--
 ifeq "$(OS)" "Darwin"
-   DS := $(shell xcrun -find -toolchain default dsymutil)
+   DS := $(DSYMUTIL)
DSFLAGS =
DSYM = $(EXE).dSYM
AR := $(CROSS_COMPILE)libtool
@@ -668,7 +668,7 @@ endif
 
 #--
 # From http://blog.melski.net/tag/debugging-makefiles/
-# 
+#
 # Usage: make print-CC print-CXX print-LD
 #--
 print-%:

Modified: lldb/trunk/test/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/test/CMakeLists.txt?rev=329889&r1=329888&r2=329889&view=diff
==
--- lldb/trunk/test/CMakeLists.txt (original)
+++ lldb/trunk/test/CMakeLists.txt Thu Apr 12 02:25:32 2018
@@ -13,7 +13,7 @@ function(add_python_test_target name tes
 )
 endfunction()
 
-set(LLDB_TEST_DEPS lldb)
+set(LLDB_TEST_DEPS lldb dsymutil)
 
 # darwin-debug is an hard dependency for the testsuite.
 if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
@@ -67,6 +67,7 @@ set(LLDB_TEST_USER_ARGS
 set(LLDB_TEST_COMMON_ARGS
   --arch=${LLDB_TEST_ARCH}
   --executable $
+  --dsymutil $
   -s
   ${CMAKE_BINARY_DIR}/lldb-test-traces
   --build-dir


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

[Lldb-commits] [PATCH] D45518: [dotest] Use in-tree dsymutil on Darwin

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL329889: [dotest] Use in-tree dsymutil on Darwin (authored by 
JDevlieghere, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D45518?vs=142037&id=142137#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D45518

Files:
  lldb/trunk/packages/Python/lldbsuite/test/dotest.py
  lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py
  lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules
  lldb/trunk/test/CMakeLists.txt


Index: lldb/trunk/test/CMakeLists.txt
===
--- lldb/trunk/test/CMakeLists.txt
+++ lldb/trunk/test/CMakeLists.txt
@@ -13,7 +13,7 @@
 )
 endfunction()
 
-set(LLDB_TEST_DEPS lldb)
+set(LLDB_TEST_DEPS lldb dsymutil)
 
 # darwin-debug is an hard dependency for the testsuite.
 if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
@@ -67,6 +67,7 @@
 set(LLDB_TEST_COMMON_ARGS
   --arch=${LLDB_TEST_ARCH}
   --executable $
+  --dsymutil $
   -s
   ${CMAKE_BINARY_DIR}/lldb-test-traces
   --build-dir
Index: lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -147,7 +147,7 @@
 # Change any build/tool options needed
 #--
 ifeq "$(OS)" "Darwin"
-   DS := $(shell xcrun -find -toolchain default dsymutil)
+   DS := $(DSYMUTIL)
DSFLAGS =
DSYM = $(EXE).dSYM
AR := $(CROSS_COMPILE)libtool
@@ -668,7 +668,7 @@
 
 #--
 # From http://blog.melski.net/tag/debugging-makefiles/
-# 
+#
 # Usage: make print-CC print-CXX print-LD
 #--
 print-%:
Index: lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py
+++ lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py
@@ -83,6 +83,8 @@
 help=textwrap.dedent('''Specify the extra flags to be passed to the 
toolchain when building the inferior programs to be debugged
suggestions: do not 
lump the "-A arch1 -A arch2" together such that the -E option applies to only 
one of the architectures'''))
 
+group.add_argument('--dsymutil', metavar='dsymutil', dest='dsymutil', 
help=textwrap.dedent('Specify Which dsymutil to use.'))
+
 # Test filtering options
 group = parser.add_argument_group('Test filtering options')
 group.add_argument(
Index: lldb/trunk/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/dotest.py
+++ lldb/trunk/packages/Python/lldbsuite/test/dotest.py
@@ -301,6 +301,12 @@
 configuration.compiler = candidate
 break
 
+if args.dsymutil:
+  os.environ['DSYMUTIL'] = args.dsymutil
+else if platform_system == 'Darwin':
+  os.environ['DSYMUTIL'] = seven.get_command_output(
+  'xcrun -find -toolchain default dsymutil')
+
 if args.channels:
 lldbtest_config.channels = args.channels
 


Index: lldb/trunk/test/CMakeLists.txt
===
--- lldb/trunk/test/CMakeLists.txt
+++ lldb/trunk/test/CMakeLists.txt
@@ -13,7 +13,7 @@
 )
 endfunction()
 
-set(LLDB_TEST_DEPS lldb)
+set(LLDB_TEST_DEPS lldb dsymutil)
 
 # darwin-debug is an hard dependency for the testsuite.
 if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
@@ -67,6 +67,7 @@
 set(LLDB_TEST_COMMON_ARGS
   --arch=${LLDB_TEST_ARCH}
   --executable $
+  --dsymutil $
   -s
   ${CMAKE_BINARY_DIR}/lldb-test-traces
   --build-dir
Index: lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/trunk/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -147,7 +147,7 @@
 # Change any build/tool options needed
 #--
 ifeq "$(OS)" "Darwin"
-	DS := $(shell xcrun -find -toolchain default dsymutil)
+	DS := $(DSYMUTIL)
 	DSFLAGS =
 	DSYM = $(EXE).dSYM
 	AR := $(CROSS_COMPILE)libtool
@@ -668,7 +668,7 @@
 
 #--
 # From http://blog.melski.net/tag/debugging-makefiles/
-# 
+#
 # Usage: make print-CC print-CXX print-LD
 #--
 print-%:
Index: lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The fix seems simple enough, but Jim needs to say whether this is the right way 
to fix this bug, as I am not sure about what are our assumptions about 
Breakpoint object lifetimes.




Comment at: 
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c:13
 {
+printf("Observable side effect\n");
 return 0; // Set break point at this line.

Why did you need to add this? This seems like something that could easily be 
removed/reshuffled in the future (breaking your test, if it depends on it).



Comment at: source/Breakpoint/BreakpointList.cpp:120-121
+auto bp = *pos;
+if (bp->AllowDelete()) {
+  bp->ClearAllBreakpointSites();
+  pos = m_breakpoints.erase(pos);

Don't we need to do the same thing in the other "remove" functions (`Remove`, 
`RemoveAll`).


https://reviews.llvm.org/D45554



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

[Lldb-commits] [lldb] r329890 - [dotest] Fix syntax error and typo.

[Jonas Devlieghere via lldb-commits]

Author: jdevlieghere
Date: Thu Apr 12 02:35:17 2018
New Revision: 329890

URL: http://llvm.org/viewvc/llvm-project?rev=329890&view=rev
Log:
[dotest] Fix syntax error and typo.

Python uses `elif` rather than `else if`. Fixes r329889.

Modified:
lldb/trunk/packages/Python/lldbsuite/test/dotest.py
lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py

Modified: lldb/trunk/packages/Python/lldbsuite/test/dotest.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/dotest.py?rev=329890&r1=329889&r2=329890&view=diff
==
--- lldb/trunk/packages/Python/lldbsuite/test/dotest.py (original)
+++ lldb/trunk/packages/Python/lldbsuite/test/dotest.py Thu Apr 12 02:35:17 2018
@@ -303,7 +303,7 @@ def parseOptionsAndInitTestdirs():
 
 if args.dsymutil:
   os.environ['DSYMUTIL'] = args.dsymutil
-else if platform_system == 'Darwin':
+elif platform_system == 'Darwin':
   os.environ['DSYMUTIL'] = seven.get_command_output(
   'xcrun -find -toolchain default dsymutil')
 

Modified: lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py?rev=329890&r1=329889&r2=329890&view=diff
==
--- lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py (original)
+++ lldb/trunk/packages/Python/lldbsuite/test/dotest_args.py Thu Apr 12 
02:35:17 2018
@@ -83,7 +83,7 @@ def create_parser():
 help=textwrap.dedent('''Specify the extra flags to be passed to the 
toolchain when building the inferior programs to be debugged
suggestions: do not 
lump the "-A arch1 -A arch2" together such that the -E option applies to only 
one of the architectures'''))
 
-group.add_argument('--dsymutil', metavar='dsymutil', dest='dsymutil', 
help=textwrap.dedent('Specify Which dsymutil to use.'))
+group.add_argument('--dsymutil', metavar='dsymutil', dest='dsymutil', 
help=textwrap.dedent('Specify which dsymutil to use.'))
 
 # Test filtering options
 group = parser.add_argument_group('Test filtering options')


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

[Lldb-commits] [PATCH] D45333: WIP: [LIT] Have lit run the lldb test suite

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

This looks good.

Do you have any plans on how will this work with  the XCode build (which is 
kind of a prerequisite to start removing stuff from dotest)?


https://reviews.llvm.org/D45333



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

[Lldb-commits] [PATCH] D45480: Move Args.cpp from Interpreter to Utility

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D45480#1063268, @zturner wrote:

> > This removes the last (direct) dependency from the Host module to 
> > Interpreter, so I remove the Interpreter module from Host's dependency list.
>
> Big milestone!  Kudos


Thanks. Host is still a member of a lot of cycles with other modules, but I 
think I have a plan on how to solve these as well.

I take it that everyone is ok with checking this in then.


https://reviews.llvm.org/D45480



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

[Lldb-commits] [lldb] r329891 - Don't assume backing thread shares protocol ID.

[Jonas Devlieghere via lldb-commits]

Author: jdevlieghere
Date: Thu Apr 12 02:58:20 2018
New Revision: 329891

URL: http://llvm.org/viewvc/llvm-project?rev=329891&view=rev
Log:
Don't assume backing thread shares protocol ID.

When we're dealing with virtual (memory) threads created by the OS
plugins, there's no guarantee that the real thread and the backing
thread share a protocol ID. Instead, we should iterate over the memory
threads to find the virtual thread that is backed by the current real
thread.

Differential revision: https://reviews.llvm.org/D45497

rdar://36485830

Added:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestThreadSelectionBug.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system.py
Modified:
lldb/trunk/include/lldb/Target/ThreadList.h
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/trunk/source/Target/ThreadList.cpp

Modified: lldb/trunk/include/lldb/Target/ThreadList.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/ThreadList.h?rev=329891&r1=329890&r2=329891&view=diff
==
--- lldb/trunk/include/lldb/Target/ThreadList.h (original)
+++ lldb/trunk/include/lldb/Target/ThreadList.h Thu Apr 12 02:58:20 2018
@@ -102,6 +102,8 @@ public:
 
   lldb::ThreadSP GetThreadSPForThreadPtr(Thread *thread_ptr);
 
+  lldb::ThreadSP GetBackingThread(const lldb::ThreadSP &real_thread);
+
   bool ShouldStop(Event *event_ptr);
 
   Vote ShouldReportStop(Event *event_ptr);

Added: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestThreadSelectionBug.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestThreadSelectionBug.py?rev=329891&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestThreadSelectionBug.py
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestThreadSelectionBug.py
 Thu Apr 12 02:58:20 2018
@@ -0,0 +1,50 @@
+from __future__ import print_function
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestThreadSelectionBug(GDBRemoteTestBase):
+def test(self):
+class MyResponder(MockGDBServerResponder):
+def cont(self):
+# Simulate process stopping due to a raise(SIGINT)
+return "T01reason:signal"
+
+self.server.responder = MyResponder()
+target = self.createTarget("a.yaml")
+process = self.connect(target)
+python_os_plugin_path = os.path.join(self.getSourceDir(),
+ 'operating_system.py')
+command = "settings set target.process.python-os-plugin-path 
'{}'".format(
+python_os_plugin_path)
+self.dbg.HandleCommand(command)
+
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertEqual(process.GetNumThreads(), 3)
+
+# Verify our OS plug-in threads showed up
+thread = process.GetThreadByID(0x1)
+self.assertTrue(
+thread.IsValid(),
+"Make sure there is a thread 0x1 after we load the python OS 
plug-in")
+thread = process.GetThreadByID(0x2)
+self.assertTrue(
+thread.IsValid(),
+"Make sure there is a thread 0x2 after we load the python OS 
plug-in")
+thread = process.GetThreadByID(0x3)
+self.assertTrue(
+thread.IsValid(),
+"Make sure there is a thread 0x3 after we load the python OS 
plug-in")
+
+# Verify that a thread other than 3 is selected.
+thread = process.GetSelectedThread()
+self.assertNotEqual(thread.GetThreadID(), 0x3)
+
+# Verify that we select the thread backed by physical thread 1, rather
+# than virtual thread 1. The mapping comes from the OS plugin, where we
+# specified that thread 3 is backed by real thread 1.
+process.Continue()
+thread = process.GetSelectedThread()
+self.assertEqual(thread.GetThreadID(), 0x3)

Added: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system.py?rev=329891&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system.py
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system.py
 Thu Apr 12 02:58:20 2018
@@ -0,0 +1,45 @@
+import lldb
+import struct
+
+
+class OperatingSystemPlugIn(object):
+"""Class that provides data f

[Lldb-commits] [PATCH] D45497: Don't assume the backing thread shares a Protocol ID

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL329891: Don't assume backing thread shares protocol ID. 
(authored by JDevlieghere, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D45497?vs=141982&id=142139#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D45497

Files:
  lldb/trunk/include/lldb/Target/ThreadList.h
  
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestThreadSelectionBug.py
  
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system.py
  lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/trunk/source/Target/ThreadList.cpp

Index: lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system.py
+++ lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system.py
@@ -0,0 +1,45 @@
+import lldb
+import struct
+
+
+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:
+self.threads = [{
+'tid': 0x1,
+'name': 'one',
+'queue': 'queue1',
+'state': 'stopped',
+'stop_reason': 'none'
+}, {
+'tid': 0x2,
+'name': 'two',
+'queue': 'queue2',
+'state': 'stopped',
+'stop_reason': 'none'
+}, {
+'tid': 0x3,
+'name': 'three',
+'queue': 'queue3',
+'state': 'stopped',
+'stop_reason': 'sigstop',
+'core': 0
+}]
+return self.threads
Index: lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestThreadSelectionBug.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestThreadSelectionBug.py
+++ lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestThreadSelectionBug.py
@@ -0,0 +1,50 @@
+from __future__ import print_function
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestThreadSelectionBug(GDBRemoteTestBase):
+def test(self):
+class MyResponder(MockGDBServerResponder):
+def cont(self):
+# Simulate process stopping due to a raise(SIGINT)
+return "T01reason:signal"
+
+self.server.responder = MyResponder()
+target = self.createTarget("a.yaml")
+process = self.connect(target)
+python_os_plugin_path = os.path.join(self.getSourceDir(),
+ 'operating_system.py')
+command = "settings set target.process.python-os-plugin-path '{}'".format(
+python_os_plugin_path)
+self.dbg.HandleCommand(command)
+
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertEqual(process.GetNumThreads(), 3)
+
+# Verify our OS plug-in threads showed up
+thread = process.GetThreadByID(0x1)
+self.assertTrue(
+thread.IsValid(),
+"Make sure there is a thread 0x1 after we load the python OS plug-in")
+thread = process.GetThreadByID(0x2)
+self.assertTrue(
+thread.IsValid(),
+"Make sure there is a thread 0x2 after we load the python OS plug-in")
+thread = process.GetThreadByID(0x3)
+self.assertTrue(
+thread.IsValid(),
+"Make sure there is a thread 0x3 after we load the python OS plug-in")
+
+# Verify that a thread other than 3 is selected.
+thread = process.GetSelectedThread()
+self.assertNotEqual(thread.GetThreadID(), 0x3)
+
+# Verify that we select the thread backed by physical thread 1, rather
+# than virtual thread 1. The mapping comes from the OS plugin, where we
+# specified that thread 3 is backed by real thread 1.
+process.Continue()
+thread = process.GetSelectedThread

[Lldb-commits] [PATCH] D45333: WIP: [LIT] Have lit run the lldb test suite

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In https://reviews.llvm.org/D45333#1065332, @labath wrote:

> This looks good.
>
> Do you have any plans on how will this work with  the XCode build (which is 
> kind of a prerequisite to start removing stuff from dotest)?


I haven't spend too much time worrying about Xcode yet. Before we can remove 
anything we'll need to make sure that we have feature parity, at least for the 
driver part. I think that'll give us enough time to come up with a solution. :-)


https://reviews.llvm.org/D45333



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

[Lldb-commits] [lldb] r329897 - Revert "Don't assume backing thread shares protocol ID."

[Jonas Devlieghere via lldb-commits]

Author: jdevlieghere
Date: Thu Apr 12 03:51:52 2018
New Revision: 329897

URL: http://llvm.org/viewvc/llvm-project?rev=329897&view=rev
Log:
Revert "Don't assume backing thread shares protocol ID."

This reverts r329891 because the test case is timing out on linux:
http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/21834

Removed:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestThreadSelectionBug.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system.py
Modified:
lldb/trunk/include/lldb/Target/ThreadList.h
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/trunk/source/Target/ThreadList.cpp

Modified: lldb/trunk/include/lldb/Target/ThreadList.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/ThreadList.h?rev=329897&r1=329896&r2=329897&view=diff
==
--- lldb/trunk/include/lldb/Target/ThreadList.h (original)
+++ lldb/trunk/include/lldb/Target/ThreadList.h Thu Apr 12 03:51:52 2018
@@ -102,8 +102,6 @@ public:
 
   lldb::ThreadSP GetThreadSPForThreadPtr(Thread *thread_ptr);
 
-  lldb::ThreadSP GetBackingThread(const lldb::ThreadSP &real_thread);
-
   bool ShouldStop(Event *event_ptr);
 
   Vote ShouldReportStop(Event *event_ptr);

Removed: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestThreadSelectionBug.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestThreadSelectionBug.py?rev=329896&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestThreadSelectionBug.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestThreadSelectionBug.py
 (removed)
@@ -1,50 +0,0 @@
-from __future__ import print_function
-import lldb
-from lldbsuite.test.lldbtest import *
-from lldbsuite.test.decorators import *
-from gdbclientutils import *
-
-
-class TestThreadSelectionBug(GDBRemoteTestBase):
-def test(self):
-class MyResponder(MockGDBServerResponder):
-def cont(self):
-# Simulate process stopping due to a raise(SIGINT)
-return "T01reason:signal"
-
-self.server.responder = MyResponder()
-target = self.createTarget("a.yaml")
-process = self.connect(target)
-python_os_plugin_path = os.path.join(self.getSourceDir(),
- 'operating_system.py')
-command = "settings set target.process.python-os-plugin-path 
'{}'".format(
-python_os_plugin_path)
-self.dbg.HandleCommand(command)
-
-self.assertTrue(process, PROCESS_IS_VALID)
-self.assertEqual(process.GetNumThreads(), 3)
-
-# Verify our OS plug-in threads showed up
-thread = process.GetThreadByID(0x1)
-self.assertTrue(
-thread.IsValid(),
-"Make sure there is a thread 0x1 after we load the python OS 
plug-in")
-thread = process.GetThreadByID(0x2)
-self.assertTrue(
-thread.IsValid(),
-"Make sure there is a thread 0x2 after we load the python OS 
plug-in")
-thread = process.GetThreadByID(0x3)
-self.assertTrue(
-thread.IsValid(),
-"Make sure there is a thread 0x3 after we load the python OS 
plug-in")
-
-# Verify that a thread other than 3 is selected.
-thread = process.GetSelectedThread()
-self.assertNotEqual(thread.GetThreadID(), 0x3)
-
-# Verify that we select the thread backed by physical thread 1, rather
-# than virtual thread 1. The mapping comes from the OS plugin, where we
-# specified that thread 3 is backed by real thread 1.
-process.Continue()
-thread = process.GetSelectedThread()
-self.assertEqual(thread.GetThreadID(), 0x3)

Removed: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system.py?rev=329896&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system.py
 (removed)
@@ -1,45 +0,0 @@
-import lldb
-import struct
-
-
-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 proc

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

I prefer having it as a top level command rather than part of log. If you think 
about it LLVM does the same distinction and it worked quite well in practice.
We plan to collect more metrics to the command so I'd very much like to have 
this living as a separate entity.




Comment at: lldb/packages/Python/lldbsuite/test/functionalities/stats/main.c:12
+  //%self.expect("stats enable")
+  //%self.expect("frame var", substrs=['27'])
+  //%self.expect("stats disable", substrs=['frame var successes : 1', 'frame 
var failures : 0'])

labath wrote:
> Maybe test for failure as well (I guess trying to print an non-existing 
> variable should do the trick)?
Testing for failure here is a little tricky because the expression parser 
reports a failure only when it fails internally (and not when succeeds but 
returns an error because clangAST can't find the variable). Eventually we could 
make it more fine grained but for now I just would like to get the number of 
failures resolving variable due to expression parsing logic failing (and not 
because typos).



Comment at: lldb/source/Commands/CommandObjectStats.cpp:31-35
+if (!target) {
+  result.AppendError("stats command needs a target");
+  result.SetStatus(eReturnStatusFailed);
+  return false;
+}

labath wrote:
> I think we have some way (should involve `eCommandRequiresTarget`) to avoid 
> the need for checking this.
I'll check this out, thanks!



Comment at: lldb/source/Commands/CommandObjectStats.cpp:43-46
+target->RegisterStats(StatisticKind::ExpressionSuccessful);
+target->RegisterStats(StatisticKind::ExpressionFailure);
+target->RegisterStats(StatisticKind::FrameVarSuccess);
+target->RegisterStats(StatisticKind::FrameVarFailure);

labath wrote:
> Are you planning to have some kind of decentralized method of registering 
> statistics?
> If not, then I don't see a need for explicitly registering the statistic 
> kinds here, since the single source of truth about the available kinds can be 
> StatisticKind enum, and Target can just get the list from there automatically 
> when need (when stats are enabled?)
> 
> This way we could even simplify the code by avoiding the "statistic is not 
> registered but someone still called IncrementStats" state altogether) and the 
> stats map could be a simple array with NumStatistics elements.
No. I originally thought to make it that way but I have to say that maybe it's 
easier to have a centralized mechanism. I'm still unsure whether this should be 
a vector or a map, I'll think about it more.


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45480: Move Args.cpp from Interpreter to Utility

[Davide Italiano via Phabricator via lldb-commits]

davide accepted this revision.
davide added a comment.
This revision is now accepted and ready to land.

No objections from me.


https://reviews.llvm.org/D45480



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D45547#1065661, @davide wrote:

> I prefer having it as a top level command rather than part of log. If you 
> think about it LLVM does the same distinction and it worked quite well in 
> practice.
>  We plan to collect more metrics to the command so I'd very much like to have 
> this living as a separate entity.


I agree that "logging" and "statistics" sound like two different entities. 
Though in that case, maybe it's the "log timers" command that is misplaced. 
"performance timers" sound more like "statistics" than "logging" to me. Maybe 
eventually these two things should be merged even? I don't know what's the 
right solution here, and I don't care much about it even, though I would be sad 
if the choice we make now prevents some future unification due to backwards 
compatibility issues.




Comment at: lldb/packages/Python/lldbsuite/test/functionalities/stats/main.c:12
+  //%self.expect("stats enable")
+  //%self.expect("frame var", substrs=['27'])
+  //%self.expect("stats disable", substrs=['frame var successes : 1', 'frame 
var failures : 0'])

davide wrote:
> labath wrote:
> > Maybe test for failure as well (I guess trying to print an non-existing 
> > variable should do the trick)?
> Testing for failure here is a little tricky because the expression parser 
> reports a failure only when it fails internally (and not when succeeds but 
> returns an error because clangAST can't find the variable). Eventually we 
> could make it more fine grained but for now I just would like to get the 
> number of failures resolving variable due to expression parsing logic failing 
> (and not because typos).
Makes sense, thanks for the explanation.



Comment at: lldb/source/Commands/CommandObjectStats.cpp:43-46
+target->RegisterStats(StatisticKind::ExpressionSuccessful);
+target->RegisterStats(StatisticKind::ExpressionFailure);
+target->RegisterStats(StatisticKind::FrameVarSuccess);
+target->RegisterStats(StatisticKind::FrameVarFailure);

davide wrote:
> labath wrote:
> > Are you planning to have some kind of decentralized method of registering 
> > statistics?
> > If not, then I don't see a need for explicitly registering the statistic 
> > kinds here, since the single source of truth about the available kinds can 
> > be StatisticKind enum, and Target can just get the list from there 
> > automatically when need (when stats are enabled?)
> > 
> > This way we could even simplify the code by avoiding the "statistic is not 
> > registered but someone still called IncrementStats" state altogether) and 
> > the stats map could be a simple array with NumStatistics elements.
> No. I originally thought to make it that way but I have to say that maybe 
> it's easier to have a centralized mechanism. I'm still unsure whether this 
> should be a vector or a map, I'll think about it more.
llvm manual recommends 

 vector as a first choice data structure for a map. :P


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45573: Report more precise error message when attach fails

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: jingham, JDevlieghere.

If the remote stub sends a specific error message instead of just a E??
code, we can use this to display a more informative error message
instead of just the generic "unable to attach" message.

I write a test for this using the SB API.
On the console this will show up like:
(lldb) process attach ...
error: attach failed: 

if the stub supports error messages, or:
error: attach failed: "Error ??"

if it doesn't.


https://reviews.llvm.org/D45573

Files:
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp


Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -3808,8 +3808,8 @@
 }
 // E01 code from vAttach means that the attach failed
 if (::strstr(continue_cstr, "vAttach") != NULL &&
-response.GetError() == 0x1) {
-  process->SetExitStatus(-1, "unable to attach");
+response.GetStatus().Fail()) {
+  process->SetExitStatus(-1, response.GetStatus().AsCString());
 } else {
   process->SetExitStatus(-1, "lost connection");
 }
Index: 
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
===
--- 
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
+++ 
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
@@ -126,6 +126,8 @@
 return self.qfThreadInfo()
 if packet == "qC":
 return self.qC()
+if packet == "QEnableErrorStrings":
+return self.QEnableErrorStrings()
 if packet == "?":
 return self.haltReason()
 if packet[0] == "H":
@@ -137,6 +139,9 @@
 if data is not None:
 return self._qXferResponse(data, has_more)
 return ""
+if packet.startswith("vAttach;"):
+pid = packet.partition(';')[2]
+return self.vAttach(int(pid, 16))
 if packet[0] == "Z":
 return self.setBreakpoint(packet)
 return self.other(packet)
@@ -177,6 +182,9 @@
 def qC(self):
 return "QC0"
 
+def QEnableErrorStrings(self):
+return "OK"
+
 def haltReason(self):
 # SIGINT is 2, return type is 2 digit hex string
 return "S02"
@@ -187,6 +195,9 @@
 def _qXferResponse(self, data, has_more):
 return "%s%s" % ("m" if has_more else "l", escape_binary(data))
 
+def vAttach(self, pid):
+raise self.UnexpectedPacketException()
+
 def selectThread(self, op, thread_id):
 return "OK"
 
Index: 
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
===
--- 
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
+++ 
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
@@ -11,3 +11,28 @@
 target = self.createTarget("a.yaml")
 process = self.connect(target)
 self.assertPacketLogContains(["qProcessInfo", "qfThreadInfo"])
+
+def test_attach_fail(self):
+error_msg = "mock-error-msg"
+
+class MyResponder(MockGDBServerResponder):
+# Pretend we don't have any process during the initial queries.
+def qC(self):
+return "E42"
+
+def qfThreadInfo(self):
+return "OK" # No threads.
+
+# Then, when we are asked to attach, error out.
+def vAttach(self, pid):
+return "E42;" + error_msg.encode("hex")
+
+self.server.responder = MyResponder()
+
+target = self.dbg.CreateTarget("")
+process = self.connect(target)
+lldbutil.expect_state_changes(self, self.dbg.GetListener(), process, 
[lldb.eStateConnected])
+
+error = lldb.SBError()
+target.AttachToProcessWithID(lldb.SBListener(), 47, error)
+self.assertEquals(error_msg, error.GetCString())


Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -3808,8 +3808,8 @@
 }
 // E01 code from vAttach means that the attach failed
 if (::strstr(continue_cstr, "vAttach") != NULL &&
-response.GetError() == 

[Lldb-commits] [PATCH] D45573: Report more precise error message when attach fails

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

LGTM module comment




Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:3809
 }
 // E01 code from vAttach means that the attach failed
 if (::strstr(continue_cstr, "vAttach") != NULL &&

Outdated comment


https://reviews.llvm.org/D45573



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

[Lldb-commits] [PATCH] D45573: Report more precise error message when attach fails

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

lgtm


https://reviews.llvm.org/D45573



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

I'm under the impression that we should either merge `log timers` with `stats` 
or just remove log timers altogether and start from scratch.
From what I hear from Jim, it was really useful for a few people, so maybe a 
fresh start would be a better way of handling things. Thanks!


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide added inline comments.



Comment at: lldb/source/Commands/CommandObjectStats.cpp:43-46
+target->RegisterStats(StatisticKind::ExpressionSuccessful);
+target->RegisterStats(StatisticKind::ExpressionFailure);
+target->RegisterStats(StatisticKind::FrameVarSuccess);
+target->RegisterStats(StatisticKind::FrameVarFailure);

labath wrote:
> davide wrote:
> > labath wrote:
> > > Are you planning to have some kind of decentralized method of registering 
> > > statistics?
> > > If not, then I don't see a need for explicitly registering the statistic 
> > > kinds here, since the single source of truth about the available kinds 
> > > can be StatisticKind enum, and Target can just get the list from there 
> > > automatically when need (when stats are enabled?)
> > > 
> > > This way we could even simplify the code by avoiding the "statistic is 
> > > not registered but someone still called IncrementStats" state altogether) 
> > > and the stats map could be a simple array with NumStatistics elements.
> > No. I originally thought to make it that way but I have to say that maybe 
> > it's easier to have a centralized mechanism. I'm still unsure whether this 
> > should be a vector or a map, I'll think about it more.
> llvm manual recommends 
> 
>  vector as a first choice data structure for a map. :P
Allright, you convinced me :)


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

[Eugene Zemtsov via Phabricator via lldb-commits]

eugene updated this revision to Diff 142209.
eugene marked 2 inline comments as done.
eugene added a comment.

add comment to the test


https://reviews.llvm.org/D45554

Files:
  
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
  
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c
  source/Breakpoint/BreakpointList.cpp


Index: source/Breakpoint/BreakpointList.cpp
===
--- source/Breakpoint/BreakpointList.cpp
+++ source/Breakpoint/BreakpointList.cpp
@@ -99,7 +99,7 @@
 
 void BreakpointList::RemoveAllowed(bool notify) {
   std::lock_guard guard(m_mutex);
-  
+
   bp_collection::iterator pos, end = m_breakpoints.end();
   if (notify) {
 for (pos = m_breakpoints.begin(); pos != end; ++pos) {
@@ -116,10 +116,12 @@
   }
   pos = m_breakpoints.begin();
   while ( pos != end) {
-  if((*pos)->AllowDelete())
-pos = m_breakpoints.erase(pos);
-  else
-pos++;
+auto bp = *pos;
+if (bp->AllowDelete()) {
+  bp->ClearAllBreakpointSites();
+  pos = m_breakpoints.erase(pos);
+} else
+  pos++;
   }
 }
 
Index: 
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c
===
--- 
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c
+++ 
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c
@@ -6,8 +6,12 @@
 // License. See LICENSE.TXT for details.
 //
 
//===--===//
+#include 
 
 int main (int argc, char const *argv[])
 {
+// This line adds a real body to the function, so we can set more than one
+// breakpoint in it.
+printf("Observable side effect\n");
 return 0; // Set break point at this line.
 }
Index: 
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- 
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ 
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -45,6 +45,25 @@
 self.addTearDownHook(
 lambda: self.runCmd("settings clear auto-confirm"))
 
+@expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24528")
+def test_delete_all_breakpoints(self):
+"""Test that deleting all breakpoints works."""
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_symbol(self, "main")
+lldbutil.run_break_set_by_file_and_line(
+self, "main.c", self.line, num_expected_locations=1, 
loc_exact=True)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+self.runCmd("breakpoint delete")
+self.runCmd("process continue")
+self.expect("process status", PROCESS_STOPPED,
+patterns=['Process .* exited with status = 0'])
+
+
 def breakpoint_command_sequence(self):
 """Test a sequence of breakpoint command add, list, and delete."""
 exe = self.getBuildArtifact("a.out")


Index: source/Breakpoint/BreakpointList.cpp
===
--- source/Breakpoint/BreakpointList.cpp
+++ source/Breakpoint/BreakpointList.cpp
@@ -99,7 +99,7 @@
 
 void BreakpointList::RemoveAllowed(bool notify) {
   std::lock_guard guard(m_mutex);
-  
+
   bp_collection::iterator pos, end = m_breakpoints.end();
   if (notify) {
 for (pos = m_breakpoints.begin(); pos != end; ++pos) {
@@ -116,10 +116,12 @@
   }
   pos = m_breakpoints.begin();
   while ( pos != end) {
-  if((*pos)->AllowDelete())
-pos = m_breakpoints.erase(pos);
-  else
-pos++;
+auto bp = *pos;
+if (bp->AllowDelete()) {
+  bp->ClearAllBreakpointSites();
+  pos = m_breakpoints.erase(pos);
+} else
+  pos++;
   }
 }
 
Index: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c
===
--- packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c
+++ packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c
@@ -6,8 +6,12 @@
 // License. See LICENSE.TXT for details.
 //
 //===--===//
+#include 
 
 int main (int argc, char const *argv[])
 {
+// This line adds a real body to the function, so we can set more than one
+// breakpoint in it.
+printf("Observable side effect\n");
 return 0; // Set break point at this line.
 }
Index: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCo

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

[Eugene Zemtsov via Phabricator via lldb-commits]

eugene added inline comments.



Comment at: 
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c:13
 {
+printf("Observable side effect\n");
 return 0; // Set break point at this line.

labath wrote:
> Why did you need to add this? This seems like something that could easily be 
> removed/reshuffled in the future (breaking your test, if it depends on it).
I just need more than one bp location in the function. "bp main" and a 
breakpoint on the line 14 were the same thing before that.
I'll add a comment.



Comment at: source/Breakpoint/BreakpointList.cpp:120-121
+auto bp = *pos;
+if (bp->AllowDelete()) {
+  bp->ClearAllBreakpointSites();
+  pos = m_breakpoints.erase(pos);

labath wrote:
> Don't we need to do the same thing in the other "remove" functions (`Remove`, 
> `RemoveAll`).
RemoveAll does the same at the line 83. 
With Remove it's a more complicated. Target removes sites manually by disabling 
breakpoint, but it has some extra logic that I don't fully understand and not 
sure if I should touch. 


https://reviews.llvm.org/D45554



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

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

[Davide Italiano via Phabricator via lldb-commits]

davide added inline comments.



Comment at: 
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c:14-15
+// This line adds a real body to the function, so we can set more than one
+// breakpoint in it.
+printf("Observable side effect\n");
 return 0; // Set break point at this line.

you don't really need printf.
You can just declare a set of few new fresh variables, one per line, I think.


https://reviews.llvm.org/D45554



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

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

[Davide Italiano via Phabricator via lldb-commits]

davide added inline comments.



Comment at: 
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c:14-15
+// This line adds a real body to the function, so we can set more than one
+// breakpoint in it.
+printf("Observable side effect\n");
 return 0; // Set break point at this line.

davide wrote:
> you don't really need printf.
> You can just declare a set of few new fresh variables, one per line, I think.
The reason why I'm saying this is because this test doesn't really rely on 
`stdio.h` and I'd rather leave it as is.


https://reviews.llvm.org/D45554



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Alex Langford via Phabricator via lldb-commits]

xiaobai added subscribers: sas, xiaobai.
xiaobai added a comment.

I really like this idea! It will be very helpful for @sas and I. I'd like to +1 
creating a separate `stats dump` subcommand instead of dumping stats on `stats 
disable`.




Comment at: lldb/source/Commands/CommandObjectStats.cpp:37
+
+if (target->GetCollectingStats() == true) {
+  result.AppendError("stats already enabled");

nit: You can drop the `== true`



Comment at: lldb/source/Commands/CommandObjectStats.cpp:88
+
+if (target->GetCollectingStats() == false) {
+  result.AppendError("need to enable stats before disabling them");

nit: 
```
 if (!target->GetCollectingStats()) {
```


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45586: Prevent deadlock in OS Plugins

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: clayborg, jingham, labath.

When performing a synchronous resume, the API mutex is held until the
process is stopped. This is fine, except for when the OS plugins are processing
an event before the main thread is aware of it, in which case we end up with a
deadlock because in the internal thread we acquire a resource lock first, and
then wait for the API lock, while in the main thread we do the opposite, we
already hold the API mutex but are now waiting for the event mutex to handle
the event.

This patch fixes this by relaxing the need for the API lock in the OS plugins.
We can get away with this because we now this code is executed in the main
thread. As stated in the comment above, we just want to ensure nobody else
messes with the API while we're making a change. In theory it's possible that
the main thread would release the lock while we're executing the function, but
prevent this would require a more structural solution (which we want, but do
not have today).

The same workaround was already present, but this patch generalizes it to the
whole file.

This will allow me to re-land r329891.


https://reviews.llvm.org/D45586

Files:
  source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp


Index: source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
===
--- source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
+++ source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
@@ -156,19 +156,20 @@
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_OS));
 
-  // First thing we have to do is to try to get the API lock, and the run lock.
-  // We're going to change the thread content of the process, and we're going
-  // to use python, which requires the API lock to do it.
+  // First thing we have to do is to try to get the API lock, and the
+  // interpreter lock. We're going to change the thread content of the process,
+  // and we're going to use python, which requires the API lock to do it. We
+  // need the interpreter lock to make sure thread_info_dict stays alive.
   //
   // If someone already has the API lock, that is ok, we just want to avoid
   // external code from making new API calls while this call is happening.
   //
   // This is a recursive lock so we can grant it to any Python code called on
   // the stack below us.
   Target &target = m_process->GetTarget();
-  std::unique_lock lock(target.GetAPIMutex(),
-  std::defer_lock);
-  lock.try_lock();
+  std::unique_lock api_lock(target.GetAPIMutex(),
+  std::defer_lock);
+  api_lock.try_lock();
 
   if (log)
 log->Printf("OperatingSystemPython::UpdateThreadList() fetching thread "
@@ -301,20 +302,24 @@
   if (!IsOperatingSystemPluginThread(thread->shared_from_this()))
 return reg_ctx_sp;
 
-  // First thing we have to do is get the API lock, and the run lock.  We're
-  // going to change the thread
-  // content of the process, and we're going to use python, which requires the
-  // API lock to do it.
-  // So get & hold that.  This is a recursive lock so we can grant it to any
-  // Python code called on the stack below us.
+  // First thing we have to do is to try to get the API lock, and the
+  // interpreter lock. We're going to change the thread content of the process,
+  // and we're going to use python, which requires the API lock to do it. We
+  // need the interpreter lock to make sure thread_info_dict stays alive.
+  //
+  // If someone already has the API lock, that is ok, we just want to avoid
+  // external code from making new API calls while this call is happening.
+  //
+  // This is a recursive lock so we can grant it to any Python code called on
+  // the stack below us.
   Target &target = m_process->GetTarget();
-  std::lock_guard guard(target.GetAPIMutex());
+  std::unique_lock api_lock(target.GetAPIMutex(),
+  std::defer_lock);
+  api_lock.try_lock();
+  auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD));
 
-  auto lock =
-  m_interpreter
-  ->AcquireInterpreterLock(); // to make sure python objects stays 
alive
   if (reg_data_addr != LLDB_INVALID_ADDRESS) {
 // The registers data is in contiguous memory, just create the register
 // context using the address provided
@@ -383,18 +388,21 @@
 tid, context);
 
   if (m_interpreter && m_python_object_sp) {
-// First thing we have to do is get the API lock, and the run lock.  We're
-// going to change the thread
-// content of the process, and we're going to use python, which requires 
the
-// API lock to do it.
-// So get & hold that.  This is a recursive lock so we can grant it to any
-// Python code called on

[Lldb-commits] [PATCH] D45586: Prevent deadlock in OS Plugins

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 142239.
JDevlieghere added a comment.

Make things a little more consistent.


https://reviews.llvm.org/D45586

Files:
  source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp

Index: source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
===
--- source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
+++ source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
@@ -156,32 +156,30 @@
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_OS));
 
-  // First thing we have to do is to try to get the API lock, and the run lock.
-  // We're going to change the thread content of the process, and we're going
-  // to use python, which requires the API lock to do it.
+  // First thing we have to do is to try to get the API lock, and the
+  // interpreter lock. We're going to change the thread content of the process,
+  // and we're going to use python, which requires the API lock to do it. We
+  // need the interpreter lock to make sure thread_info_dict stays alive.
   //
   // If someone already has the API lock, that is ok, we just want to avoid
   // external code from making new API calls while this call is happening.
   //
   // This is a recursive lock so we can grant it to any Python code called on
   // the stack below us.
   Target &target = m_process->GetTarget();
-  std::unique_lock lock(target.GetAPIMutex(),
-  std::defer_lock);
-  lock.try_lock();
+  std::unique_lock api_lock(target.GetAPIMutex(),
+  std::defer_lock);
+  api_lock.try_lock();
+  auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
   if (log)
 log->Printf("OperatingSystemPython::UpdateThreadList() fetching thread "
 "data from python for pid %" PRIu64,
 m_process->GetID());
 
   // The threads that are in "new_thread_list" upon entry are the threads from
-  // the
-  // lldb_private::Process subclass, no memory threads will be in this list.
-
-  auto interpreter_lock =
-  m_interpreter
-  ->AcquireInterpreterLock(); // to make sure threads_list stays alive
+  // the lldb_private::Process subclass, no memory threads will be in this
+  // list.
   StructuredData::ArraySP threads_list =
   m_interpreter->OSPlugin_ThreadsInfo(m_python_object_sp);
 
@@ -301,20 +299,24 @@
   if (!IsOperatingSystemPluginThread(thread->shared_from_this()))
 return reg_ctx_sp;
 
-  // First thing we have to do is get the API lock, and the run lock.  We're
-  // going to change the thread
-  // content of the process, and we're going to use python, which requires the
-  // API lock to do it.
-  // So get & hold that.  This is a recursive lock so we can grant it to any
-  // Python code called on the stack below us.
+  // First thing we have to do is to try to get the API lock, and the
+  // interpreter lock. We're going to change the thread content of the process,
+  // and we're going to use python, which requires the API lock to do it. We
+  // need the interpreter lock to make sure thread_info_dict stays alive.
+  //
+  // If someone already has the API lock, that is ok, we just want to avoid
+  // external code from making new API calls while this call is happening.
+  //
+  // This is a recursive lock so we can grant it to any Python code called on
+  // the stack below us.
   Target &target = m_process->GetTarget();
-  std::lock_guard guard(target.GetAPIMutex());
+  std::unique_lock api_lock(target.GetAPIMutex(),
+  std::defer_lock);
+  api_lock.try_lock();
+  auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD));
 
-  auto lock =
-  m_interpreter
-  ->AcquireInterpreterLock(); // to make sure python objects stays alive
   if (reg_data_addr != LLDB_INVALID_ADDRESS) {
 // The registers data is in contiguous memory, just create the register
 // context using the address provided
@@ -383,18 +385,21 @@
 tid, context);
 
   if (m_interpreter && m_python_object_sp) {
-// First thing we have to do is get the API lock, and the run lock.  We're
-// going to change the thread
-// content of the process, and we're going to use python, which requires the
-// API lock to do it.
-// So get & hold that.  This is a recursive lock so we can grant it to any
-// Python code called on the stack below us.
+// First thing we have to do is to try to get the API lock, and the
+// interpreter lock. We're going to change the thread content of the
+// process, and we're going to use python, which requires the API lock to
+// do it. We need the interpreter lock to make sure thread_info_dict stays
+// alive.
+//
+// If someone already has the API lock, that is o

[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Greg Clayton via Phabricator via lldb-commits]

clayborg created this revision.
clayborg added reviewers: jingham, jasonmolenda, labath, lldb-commits.
Herald added subscribers: JDevlieghere, aprantl.

Many IDEs set breakpoints using absolute paths and this causes problems when 
the full path of the source file path doesn't match what is in the debug info. 
This can be due to different build systems and do or do not resolve symlinks. 
This patch allows relative breakpoint to be set correctly without needing to do 
any target.source-map tricks. If IDEs want to, they can send down relative 
paths like:

  ./main.c
  ./src/main.c
  src/main.c
  foo/bar/src/main.c

I used the breakpoint resolver to match on the file basename and then we weed 
out anything whose relative paths don't match. This will be a huge improvement 
for IDEs as they can specify as much of a relative path as desired to uniquely 
identify a source file in the current project.


https://reviews.llvm.org/D45592

Files:
  include/lldb/Breakpoint/BreakpointResolverFileLine.h
  
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
  packages/Python/lldbsuite/test/lldbutil.py
  source/Breakpoint/BreakpointResolverFileLine.cpp

Index: source/Breakpoint/BreakpointResolverFileLine.cpp
===
--- source/Breakpoint/BreakpointResolverFileLine.cpp
+++ source/Breakpoint/BreakpointResolverFileLine.cpp
@@ -115,15 +115,35 @@
 // here is handling inlined functions -- in this case we need to make sure we
 // look at the declaration line of the inlined function, NOT the function it was
 // inlined into.
-void BreakpointResolverFileLine::FilterContexts(SymbolContextList &sc_list) {
+void BreakpointResolverFileLine::FilterContexts(SymbolContextList &sc_list,
+bool is_relative) {
   if (m_exact_match)
 return; // Nothing to do. Contexts are precise.
 
+  llvm::StringRef relative_path;
+  if (is_relative)
+relative_path = m_file_spec.GetNormalizedPath().GetDirectory().GetStringRef();
+
   Log * log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS);
   for(uint32_t i = 0; i < sc_list.GetSize(); ++i) {
 SymbolContext sc;
 sc_list.GetContextAtIndex(i, sc);
-if (! sc.block)
+if (is_relative) {
+  // If the path was reltive, make sure any matches match as long as the
+  // relative parts of the path match the path from support files
+  auto sc_dir = sc.line_entry.file.GetDirectory().GetStringRef();
+  if (!sc_dir.endswith(relative_path)) {
+// We had a relative path specified and the relative directory
+// doesn't match so remove this one
+LLDB_LOG(log, "removing not matching relative path {0} since it "
+"doesn't end with {1}", sc_dir, relative_path);
+sc_list.RemoveContextAtIndex(i);
+--i;
+continue;
+  }
+}
+
+if (!sc.block)
   continue;
 
 FileSpec file;
@@ -194,18 +214,23 @@
   // through the match list and pull out the sets that have the same file spec
   // in their line_entry and treat each set separately.
 
+  FileSpec search_file_spec = m_file_spec;
+  const bool is_relative = m_file_spec.IsRelative();
+  if (is_relative) {
+search_file_spec.GetDirectory().Clear();
+  }
   const size_t num_comp_units = context.module_sp->GetNumCompileUnits();
   for (size_t i = 0; i < num_comp_units; i++) {
 CompUnitSP cu_sp(context.module_sp->GetCompileUnitAtIndex(i));
 if (cu_sp) {
   if (filter.CompUnitPasses(*cu_sp))
-cu_sp->ResolveSymbolContext(m_file_spec, m_line_number, m_inlines,
+cu_sp->ResolveSymbolContext(search_file_spec, m_line_number, m_inlines,
 m_exact_match, eSymbolContextEverything,
 sc_list);
 }
   }
 
-  FilterContexts(sc_list);
+  FilterContexts(sc_list, is_relative);
 
   StreamString s;
   s.Printf("for %s:%d ", m_file_spec.GetFilename().AsCString(""),
Index: packages/Python/lldbsuite/test/lldbutil.py
===
--- packages/Python/lldbsuite/test/lldbutil.py
+++ packages/Python/lldbsuite/test/lldbutil.py
@@ -576,7 +576,7 @@
 if 'file' in break_results:
 out_file_name = break_results['file']
 test.assertTrue(
-file_name == out_file_name,
+out_file_name in file_name,
 "Breakpoint file name '%s' doesn't match resultant name '%s'." %
 (file_name,
  out_file_name))
Index: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -62,7 +62,31 @@
 # set

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Calling ClearAllBreakpointSites twice does no harm, it just sees the list is 
empty and goes on.  So even though Target::RemoveBreakpointByID clears the 
breakpoint sites by disabling them and then calls BreakpointList::Remove, it is 
fine for BreakpointList::Remove to also call ClearAllBreakpointSites and it 
probably should do so.  It currently has no other callers than 
Target::RemoveBreakpointByID , but if somebody else did it would be good for it 
to work properly.

On the lifetime question, the policy over what we should do if somebody is 
holding on to an SP to the breakpoint when the breakpoint gets removed from the 
target's list isn't solid.  I originally used Shared Pointers because I wasn't 
sure what a good mechanism was for preserving breakpoints against general 
deletion ("break delete").  But actually a breakpoint that isn't in the Target 
breakpoint list should just go away, I didn't end up needing to do anything 
useful with them after they are removed.  So longer term it is a good idea to 
remove Breakpoint SP's, make the breakpoint list own the breakpoints, then make 
it have fast access for ID->Breakpoint * lookups, and have all external clients 
hold onto Breakpoint ID's instead.  You can use the Internal list, or you can 
now use the hidden names feature to keep breakpoints you care about from 
getting inadvertently deleted.  So I don't think we'll ever need another clever 
way to keep breakpoints alive.

But in practice I don't think this is a big problem.  As long as we get rid of 
the BreakpointSite's when the breakpoint gets removed from the list, it becomes 
inert.  Breakpoints outside the breakpoint list won't be told to update 
themselves, so they won't re-acquire sites.

I am a little surprised however that in such a simple scenario there is another 
agent holding onto the BreakpointSP.  After all, if there weren't another 
reference, erasing the SP from the list should have destroyed the last 
instance, and the Breakpoint destructor will then destroy the location list, 
which removes the sites.  Eugene, did you see who that was when you were poking 
around at this?


https://reviews.llvm.org/D45554



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Timers seemed like they would be really useful for collection of data about 
operations in lldb, but for most things I think they end up being hard to use 
because actual wall-clock time is so variable from run to run, and especially 
for disk and inter-process heavy operations, which lldb tends to be.  I'm not 
sure we should give up on timers, sometimes you want to know "how many times 
did I do X" and other times "how long did X take" and the Timers are more 
useful for this than just a sample or wall-clock times because you can find out 
how long it took "in the Dwarf parser", etc.

But in many cases our performance is more driven by unnecessary lookups, and 
that sort of error.  For that sort of error it will be much more useful to say 
"given program A and expression B, how many DWARF DIE lookups did I do" than 
"how long did I spent wall-clock in the DWARF parser..."  When the former goes 
from 20 to 2000, that will be a much clearer symbol that we probably introduced 
a performance regression.


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Except for a minor comment, this seems fine to me, and is a useful addition!




Comment at: packages/Python/lldbsuite/test/lldbutil.py:579
 test.assertTrue(
-file_name == out_file_name,
+out_file_name in file_name,
 "Breakpoint file name '%s' doesn't match resultant name '%s'." %

The output file name should always END with the original file name, right?  Can 
we make this check reflect that more closely?


https://reviews.llvm.org/D45592



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

[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: packages/Python/lldbsuite/test/lldbutil.py:579
 test.assertTrue(
-file_name == out_file_name,
+out_file_name in file_name,
 "Breakpoint file name '%s' doesn't match resultant name '%s'." %

jingham wrote:
> The output file name should always END with the original file name, right?  
> Can we make this check reflect that more closely?
sure thing


https://reviews.llvm.org/D45592



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

[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Greg Clayton via Phabricator via lldb-commits]

clayborg updated this revision to Diff 142289.
clayborg added a comment.

Fixed the breakpoint verification in lldbutil to make sure the file name ends 
with the right thing


https://reviews.llvm.org/D45592

Files:
  include/lldb/Breakpoint/BreakpointResolverFileLine.h
  
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
  packages/Python/lldbsuite/test/lldbutil.py
  source/Breakpoint/BreakpointResolverFileLine.cpp

Index: source/Breakpoint/BreakpointResolverFileLine.cpp
===
--- source/Breakpoint/BreakpointResolverFileLine.cpp
+++ source/Breakpoint/BreakpointResolverFileLine.cpp
@@ -115,15 +115,35 @@
 // here is handling inlined functions -- in this case we need to make sure we
 // look at the declaration line of the inlined function, NOT the function it was
 // inlined into.
-void BreakpointResolverFileLine::FilterContexts(SymbolContextList &sc_list) {
+void BreakpointResolverFileLine::FilterContexts(SymbolContextList &sc_list,
+bool is_relative) {
   if (m_exact_match)
 return; // Nothing to do. Contexts are precise.
 
+  llvm::StringRef relative_path;
+  if (is_relative)
+relative_path = m_file_spec.GetNormalizedPath().GetDirectory().GetStringRef();
+
   Log * log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS);
   for(uint32_t i = 0; i < sc_list.GetSize(); ++i) {
 SymbolContext sc;
 sc_list.GetContextAtIndex(i, sc);
-if (! sc.block)
+if (is_relative) {
+  // If the path was reltive, make sure any matches match as long as the
+  // relative parts of the path match the path from support files
+  auto sc_dir = sc.line_entry.file.GetDirectory().GetStringRef();
+  if (!sc_dir.endswith(relative_path)) {
+// We had a relative path specified and the relative directory
+// doesn't match so remove this one
+LLDB_LOG(log, "removing not matching relative path {0} since it "
+"doesn't end with {1}", sc_dir, relative_path);
+sc_list.RemoveContextAtIndex(i);
+--i;
+continue;
+  }
+}
+
+if (!sc.block)
   continue;
 
 FileSpec file;
@@ -194,18 +214,23 @@
   // through the match list and pull out the sets that have the same file spec
   // in their line_entry and treat each set separately.
 
+  FileSpec search_file_spec = m_file_spec;
+  const bool is_relative = m_file_spec.IsRelative();
+  if (is_relative) {
+search_file_spec.GetDirectory().Clear();
+  }
   const size_t num_comp_units = context.module_sp->GetNumCompileUnits();
   for (size_t i = 0; i < num_comp_units; i++) {
 CompUnitSP cu_sp(context.module_sp->GetCompileUnitAtIndex(i));
 if (cu_sp) {
   if (filter.CompUnitPasses(*cu_sp))
-cu_sp->ResolveSymbolContext(m_file_spec, m_line_number, m_inlines,
+cu_sp->ResolveSymbolContext(search_file_spec, m_line_number, m_inlines,
 m_exact_match, eSymbolContextEverything,
 sc_list);
 }
   }
 
-  FilterContexts(sc_list);
+  FilterContexts(sc_list, is_relative);
 
   StreamString s;
   s.Printf("for %s:%d ", m_file_spec.GetFilename().AsCString(""),
Index: packages/Python/lldbsuite/test/lldbutil.py
===
--- packages/Python/lldbsuite/test/lldbutil.py
+++ packages/Python/lldbsuite/test/lldbutil.py
@@ -576,7 +576,7 @@
 if 'file' in break_results:
 out_file_name = break_results['file']
 test.assertTrue(
-file_name == out_file_name,
+file_name.endswith(out_file_name),
 "Breakpoint file name '%s' doesn't match resultant name '%s'." %
 (file_name,
  out_file_name))
Index: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -62,7 +62,31 @@
 # setting breakpoint commands on two breakpoints at a time
 lldbutil.run_break_set_by_file_and_line(
 self, None, self.line, num_expected_locations=1, loc_exact=True)
-
+# Make sure relative path source breakpoints work as expected. We test
+# with partial paths with and without "./" prefixes.
+lldbutil.run_break_set_by_file_and_line(
+self, "./main.c", self.line,
+num_expected_locations=1, loc_exact=True)
+lldbutil.run_break_set_by_file_and_line(
+self, "breakpoint_command/main.c", self.line,
+num_expected_locations=1, loc_exact=True)
+lldbutil.run_break_set_by_file_and_line(
+self, "./breakpoint_comman

[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.

Looks good.


https://reviews.llvm.org/D45592



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

[Lldb-commits] [PATCH] D45573: Report more precise error message when attach fails

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.

That looks fine.


https://reviews.llvm.org/D45573



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide updated this revision to Diff 142301.

https://reviews.llvm.org/D45547

Files:
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/lldb-private-enumerations.h
  lldb/packages/Python/lldbsuite/test/functionalities/stats/Makefile
  lldb/packages/Python/lldbsuite/test/functionalities/stats/TestStats.py
  lldb/packages/Python/lldbsuite/test/functionalities/stats/main.c
  lldb/source/Commands/CommandObjectExpression.cpp
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/source/Commands/CommandObjectStats.cpp
  lldb/source/Commands/CommandObjectStats.h
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Target/Target.cpp

Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -87,13 +87,14 @@
   Broadcaster(debugger.GetBroadcasterManager(),
   Target::GetStaticBroadcasterClass().AsCString()),
   ExecutionContextScope(), m_debugger(debugger), m_platform_sp(platform_sp),
-  m_mutex(), m_arch(target_arch),
-  m_images(this), m_section_load_history(), m_breakpoint_list(false),
-  m_internal_breakpoint_list(true), m_watchpoint_list(), m_process_sp(),
-  m_search_filter_sp(), m_image_search_paths(ImageSearchPathsChanged, this),
-  m_ast_importer_sp(), m_source_manager_ap(), m_stop_hooks(),
-  m_stop_hook_next_id(0), m_valid(true), m_suppress_stop_hooks(false),
-  m_is_dummy_target(is_dummy_target)
+  m_mutex(), m_arch(target_arch), m_images(this), m_section_load_history(),
+  m_breakpoint_list(false), m_internal_breakpoint_list(true),
+  m_watchpoint_list(), m_process_sp(), m_search_filter_sp(),
+  m_image_search_paths(ImageSearchPathsChanged, this), m_ast_importer_sp(),
+  m_source_manager_ap(), m_stop_hooks(), m_stop_hook_next_id(0),
+  m_valid(true), m_suppress_stop_hooks(false),
+  m_is_dummy_target(is_dummy_target),
+  m_stats_storage(static_cast(StatisticKind::StatisticMax))
 
 {
   SetEventName(eBroadcastBitBreakpointChanged, "breakpoint-changed");
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -425,7 +425,7 @@
   CommandObjectSP(new CommandObjectMultiwordSettings(*this));
   m_command_dict["source"] =
   CommandObjectSP(new CommandObjectMultiwordSource(*this));
-  m_command_dict["stats"] = CommandObjectSP(new CommandObjectStats(*this));
+  m_command_dict["statistics"] = CommandObjectSP(new CommandObjectStats(*this));
   m_command_dict["target"] =
   CommandObjectSP(new CommandObjectMultiwordTarget(*this));
   m_command_dict["thread"] =
Index: lldb/source/Commands/CommandObjectStats.h
===
--- lldb/source/Commands/CommandObjectStats.h
+++ lldb/source/Commands/CommandObjectStats.h
@@ -11,16 +11,14 @@
 #define liblldb_CommandObjectStats_h_
 
 #include "lldb/Interpreter/CommandObject.h"
+#include "lldb/Interpreter/CommandObjectMultiword.h"
 
 namespace lldb_private {
-class CommandObjectStats : public CommandObjectParsed {
+class CommandObjectStats : public CommandObjectMultiword {
 public:
   CommandObjectStats(CommandInterpreter &interpreter);
 
   ~CommandObjectStats() override;
-
-protected:
-  bool DoExecute(Args &command, CommandReturnObject &result) override;
 };
 } // namespace lldb_private
 
Index: lldb/source/Commands/CommandObjectStats.cpp
===
--- lldb/source/Commands/CommandObjectStats.cpp
+++ lldb/source/Commands/CommandObjectStats.cpp
@@ -11,18 +11,118 @@
 #include "lldb/Host/Host.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
+#include "lldb/Target/Target.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
-CommandObjectStats::CommandObjectStats(CommandInterpreter &interpreter)
-: CommandObjectParsed(
-  interpreter, "stats", "Print statistics about a debugging session",
-  nullptr) {
-}
+class CommandObjectStatsEnable : public CommandObjectParsed {
+public:
+  CommandObjectStatsEnable(CommandInterpreter &interpreter)
+  : CommandObjectParsed(
+interpreter, "enable", "Enable statistics collection", nullptr,
+eCommandRequiresTarget | eCommandProcessMustBePaused) {}
+
+  ~CommandObjectStatsEnable() override = default;
+
+protected:
+  bool DoExecute(Args &command, CommandReturnObject &result) override {
+auto exe_ctx = m_interpreter.GetExecutionContext();
+Target *target = exe_ctx.GetTargetPtr();
+
+if (target->GetCollectingStats() == true) {
+  result.AppendError("statistics already enabled");
+  result.SetStatus(eReturnStatusFailed);
+  return false;
+}
+
+target->SetCollectingStats(true);
+result.SetStatus(eReturn

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

In https://reviews.llvm.org/D45547#1066348, @jingham wrote:

> Timers seemed like they would be really useful for collection of data about 
> operations in lldb, but for most things I think they end up being hard to use 
> because actual wall-clock time is so variable from run to run, and especially 
> for disk and inter-process heavy operations, which lldb tends to be.  I'm not 
> sure we should give up on timers, sometimes you want to know "how many times 
> did I do X" and other times "how long did X take" and the Timers are more 
> useful for this than just a sample or wall-clock times because you can find 
> out how long it took "in the Dwarf parser", etc.
>
> But in many cases our performance is more driven by unnecessary lookups, and 
> that sort of error.  For that sort of error it will be much more useful to 
> say "given program A and expression B, how many DWARF DIE lookups did I do" 
> than "how long did I spent wall-clock in the DWARF parser..."  When the 
> former goes from 20 to 2000, that will be a much clearer symbol that we 
> probably introduced a performance regression.


Yes, I agree. I addressed all the comments.
Jim, can you please take a look? Thanks!


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide added inline comments.



Comment at: lldb/source/Commands/CommandObjectStats.cpp:37
+
+if (target->GetCollectingStats() == true) {
+  result.AppendError("stats already enabled");

xiaobai wrote:
> nit: You can drop the `== true`
Thanks, I'll fix these!


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I made a few comments all to the same effect, you can use 
CommandObject::GetSelectedOrDummyTarget to get the target that the command is 
operating on, or you can get is straight from m_exe_ctx if you want to make 
sure that you are getting a real target.  In the case of stats, I think running 
with the dummy target is fine, since for instance you can run expressions in 
the dummy target...

Other than that I think this is a good place to start from.




Comment at: lldb/source/Commands/CommandObjectExpression.cpp:626
 
+  Target *target = m_interpreter.GetExecutionContext().GetTargetPtr();
+  if (!target)

This is what CommandObject::GetSelectedOrDummy target is for.



Comment at: lldb/source/Commands/CommandObjectStats.cpp:30-31
+  bool DoExecute(Args &command, CommandReturnObject &result) override {
+auto exe_ctx = m_interpreter.GetExecutionContext();
+Target *target = exe_ctx.GetTargetPtr();
+

By the time the DoExecute is run, m_exe_ctx has the execution context, so you 
can get the target from there, or use GetSelectedOrDummyTarget.  You can run 
expressions in the dummy target so there's no reason not to collect stats from 
it.


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide added inline comments.



Comment at: lldb/source/Commands/CommandObjectExpression.cpp:626
 
+  Target *target = m_interpreter.GetExecutionContext().GetTargetPtr();
+  if (!target)

jingham wrote:
> This is what CommandObject::GetSelectedOrDummy target is for.
I just moved existing code, but I agree it's better using this API.


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

Thanks for this! Just one minor inline.




Comment at: source/Breakpoint/BreakpointResolverFileLine.cpp:132
+if (is_relative) {
+  // If the path was reltive, make sure any matches match as long as the
+  // relative parts of the path match the path from support files

Just one typo, "reltive" -> "relative".


https://reviews.llvm.org/D45592



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

[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Davide Italiano via Phabricator via lldb-commits]

davide added inline comments.



Comment at: source/Breakpoint/BreakpointResolverFileLine.cpp:219-221
+  if (is_relative) {
+search_file_spec.GetDirectory().Clear();
+  }

For consistency with the rest of the codestyle (and what you use above), can 
you remove the braces before committing?


https://reviews.llvm.org/D45592



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

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

[Eugene Zemtsov via Phabricator via lldb-commits]

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

Got rid of the printf in the test


https://reviews.llvm.org/D45554

Files:
  
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
  
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c
  source/Breakpoint/BreakpointList.cpp


Index: source/Breakpoint/BreakpointList.cpp
===
--- source/Breakpoint/BreakpointList.cpp
+++ source/Breakpoint/BreakpointList.cpp
@@ -99,7 +99,7 @@
 
 void BreakpointList::RemoveAllowed(bool notify) {
   std::lock_guard guard(m_mutex);
-  
+
   bp_collection::iterator pos, end = m_breakpoints.end();
   if (notify) {
 for (pos = m_breakpoints.begin(); pos != end; ++pos) {
@@ -116,10 +116,12 @@
   }
   pos = m_breakpoints.begin();
   while ( pos != end) {
-  if((*pos)->AllowDelete())
-pos = m_breakpoints.erase(pos);
-  else
-pos++;
+auto bp = *pos;
+if (bp->AllowDelete()) {
+  bp->ClearAllBreakpointSites();
+  pos = m_breakpoints.erase(pos);
+} else
+  pos++;
   }
 }
 
Index: 
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c
===
--- 
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c
+++ 
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c
@@ -9,5 +9,9 @@
 
 int main (int argc, char const *argv[])
 {
+// Add a body to the function, so we can set more than one
+// breakpoint in it.
+static volatile int var = 0;
+var++;
 return 0; // Set break point at this line.
 }
Index: 
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- 
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ 
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -45,6 +45,25 @@
 self.addTearDownHook(
 lambda: self.runCmd("settings clear auto-confirm"))
 
+@expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24528")
+def test_delete_all_breakpoints(self):
+"""Test that deleting all breakpoints works."""
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_symbol(self, "main")
+lldbutil.run_break_set_by_file_and_line(
+self, "main.c", self.line, num_expected_locations=1, 
loc_exact=True)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+self.runCmd("breakpoint delete")
+self.runCmd("process continue")
+self.expect("process status", PROCESS_STOPPED,
+patterns=['Process .* exited with status = 0'])
+
+
 def breakpoint_command_sequence(self):
 """Test a sequence of breakpoint command add, list, and delete."""
 exe = self.getBuildArtifact("a.out")


Index: source/Breakpoint/BreakpointList.cpp
===
--- source/Breakpoint/BreakpointList.cpp
+++ source/Breakpoint/BreakpointList.cpp
@@ -99,7 +99,7 @@
 
 void BreakpointList::RemoveAllowed(bool notify) {
   std::lock_guard guard(m_mutex);
-  
+
   bp_collection::iterator pos, end = m_breakpoints.end();
   if (notify) {
 for (pos = m_breakpoints.begin(); pos != end; ++pos) {
@@ -116,10 +116,12 @@
   }
   pos = m_breakpoints.begin();
   while ( pos != end) {
-  if((*pos)->AllowDelete())
-pos = m_breakpoints.erase(pos);
-  else
-pos++;
+auto bp = *pos;
+if (bp->AllowDelete()) {
+  bp->ClearAllBreakpointSites();
+  pos = m_breakpoints.erase(pos);
+} else
+  pos++;
   }
 }
 
Index: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c
===
--- packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c
+++ packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c
@@ -9,5 +9,9 @@
 
 int main (int argc, char const *argv[])
 {
+// Add a body to the function, so we can set more than one
+// breakpoint in it.
+static volatile int var = 0;
+var++;
 return 0; // Set break point at this line.
 }
Index: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -45,6 +45,25 @@
 self.addTearDownH

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

[Eugene Zemtsov via Phabricator via lldb-commits]

eugene marked 2 inline comments as done.
eugene added a comment.

There is an ownership cycle between BreakpointSite::m_owners and 
BreakpointLocation::m_bp_site_sp.
We should probably make m_owners a collection of weak references. 
But currently most of the code just works it around by calling 
Breakpoint::ClearAllBreakpointSites() before deleting a breakpoint.


https://reviews.llvm.org/D45554



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