[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D133858#3818755 , @jingham wrote:

> If we use Target::CreateProcess to handle this task, how do we ensure that 
> that will always get called any time we make a new process?

I'm not really sure how to answer that.

Clearly something like that can happen, but I think the overall risk is low. 
The most likely scenario for that happening would be if someone adds a new, 
fourth, method of initiating a process (in addition to launching, attaching and 
connecting), but I just don't know what would that be. But if one is doing 
that, I think it's at least as likely that he will forget to create the 
`DoWillInitiate` method and call the breakpoint reset function from there.

I think the only way to ensure that can't happen is to store the actual hit 
counts within the Process class itself, but I don't think anyone has an apetite 
to design something like that.

Anyone trying to create a function by bypassing a function called CreateProcess 
should think really hard before proceeding. And the more stuff we put into that 
function, the harder it will be for someone to bypass it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

If we use Target::CreateProcess to handle this task, how do we ensure that that 
will always get called any time we make a new process?  That function doesn't 
do all that much special, it's only a couple lines long so it could easily get 
inlined somewhere.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D133858#3816105 , @fdeazeve wrote:

> In D133858#3805630 , @labath wrote:
>
>> I am afraid that this patch misses one method of initiating a debug session 
>> -- connecting to a running debug server (`process connect`, 
>> `SBTarget::ConnectRemote`). The breakpoint hit counts don't get reset in 
>> case of a reconnect. This isn't a particularly common use case (and the only 
>> reason I've noticed it is that for `PlatformQemuUser`, all "launches" are 
>> actually "connects" under the hood 
>> ),
>>  but I've verified that this problem can be reproduced by issuing connect 
>> commands manually (on the regular host platform). I'm pretty sure that was 
>> not intentional.
>>
>> Fixing this by adding another callout to `ResetBreakpointHitCounts` would be 
>> easy enough, but I'm also thinking if there isn't a better place from which 
>> to call this function, one that would capture all three scenarios in a 
>> single statement. I think that one such place could be 
>> `Target::CreateProcess`. This function is called by all three code paths, 
>> and it's a very good indicator that we will be starting a new debug session.
>>
>> What do you think?
>
> My understanding is that there is an obligation of calling the WillXX methods 
> before calling XX, so by placing the reset code in the WillXX functions we 
> can rely on that guarantee. Right now, the same is implicit for "one must 
> call Target::CreateProcess before launching or attaching". We could instead 
> define a WillConnect and have the usual contract for that.

I wouldn't really call it a "usual" contract, but yes, I'm sure this could be 
fixed by adding a WillConnect method. It might be also sufficient to just call 
WillAttach from at some appropriate place, since a "connect" operation looks 
very similar to an "attach", and a lot of the initialization operations are the 
same. I think we're already something like this somewhere (maybe for 
DidAttach?). However, that still leaves us with three (or two) places that need 
to be kept in sync.

> The code is fairly new to me, so I'm not confident enough to make a judgement 
> call here. Thoughts?

I don't see any advantage in doing this particular action "just before" a 
launch, as opposed to doing in on process creation, so I would definitely do it 
that way.

I also find it weird to be going through the Process class just to call another 
Target method, when all of the launch/attach/connect operations already go 
through the Target class, and so it should be perfectly capable of calling that 
method on its own.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

In D133858#3805630 , @labath wrote:

> I am afraid that this patch misses one method of initiating a debug session 
> -- connecting to a running debug server (`process connect`, 
> `SBTarget::ConnectRemote`). The breakpoint hit counts don't get reset in case 
> of a reconnect. This isn't a particularly common use case (and the only 
> reason I've noticed it is that for `PlatformQemuUser`, all "launches" are 
> actually "connects" under the hood 
> ),
>  but I've verified that this problem can be reproduced by issuing connect 
> commands manually (on the regular host platform). I'm pretty sure that was 
> not intentional.
>
> Fixing this by adding another callout to `ResetBreakpointHitCounts` would be 
> easy enough, but I'm also thinking if there isn't a better place from which 
> to call this function, one that would capture all three scenarios in a single 
> statement. I think that one such place could be `Target::CreateProcess`. This 
> function is called by all three code paths, and it's a very good indicator 
> that we will be starting a new debug session.
>
> What do you think?

My understanding is that there is an obligation of calling the WillXX methods 
before calling XX, so by placing the reset code in the WillXX functions we can 
rely on that guarantee. Right now, the same is implicit for "one must call 
Target::CreateProcess before launching or attaching". We could instead define a 
WillConnect and have the usual contract for that.

The code is fairly new to me, so I'm not confident enough to make a judgement 
call here. Thoughts?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I am afraid that this patch misses one method of initiating a debug session -- 
connecting to a running debug server (`process connect`, 
`SBTarget::ConnectRemote`). The breakpoint hit counts don't get reset in case 
of a reconnect. This isn't a particularly common use case (and the only reason 
I've noticed it is that for `PlatformQemuUser`, all "launches" are actually 
"connects" under the hood 
),
 but I've verified that this problem can be reproduced by issuing connect 
commands manually (on the regular host platform). I'm pretty sure that was not 
intentional.

Fixing this by adding another callout to `ResetBreakpointHitCounts` would be 
easy enough, but I'm also thinking if there isn't a better place from which to 
call this function, one that would capture all three scenarios in a single 
statement. I think that one such place could be `Target::CreateProcess`. This 
function is called by all three code paths, and it's a very good indicator that 
we will be starting a new debug session.

What do you think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG974958749827: [lldb] Reset breakpoint hit count before new 
runs (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

Files:
  lldb/include/lldb/Breakpoint/Breakpoint.h
  lldb/include/lldb/Breakpoint/BreakpointList.h
  lldb/include/lldb/Breakpoint/BreakpointLocation.h
  lldb/include/lldb/Breakpoint/BreakpointLocationList.h
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Target/Target.h
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Breakpoint/BreakpointList.cpp
  lldb/source/Breakpoint/BreakpointLocationList.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestAddressBreakpoints.py
  
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
  lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/Makefile
  
lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/TestBreakpointResetUponRun.py
  lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/main.cpp

Index: lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/main.cpp
@@ -0,0 +1,6 @@
+#include 
+
+int main(int argc, char const *argv[]) {
+  printf("Set a breakpoint here.\n");
+  return 0;
+}
Index: lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/TestBreakpointResetUponRun.py
===
--- /dev/null
+++ lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/TestBreakpointResetUponRun.py
@@ -0,0 +1,58 @@
+"""
+Test breakpoint hit count is reset when target runs.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+
+class HitcountResetUponRun(TestBase):
+BREAKPOINT_TEXT = 'Set a breakpoint here'
+
+def check_stopped_at_breakpoint_and_hit_once(self, thread, breakpoint):
+frame0 = thread.GetFrameAtIndex(0)
+location1 = breakpoint.FindLocationByAddress(frame0.GetPC())
+self.assertTrue(location1)
+self.assertEqual(location1.GetHitCount(), 1)
+self.assertEqual(breakpoint.GetHitCount(), 1)
+
+def test_hitcount_reset_upon_run(self):
+self.build()
+
+exe = self.getBuildArtifact("a.out")
+
+target = self.dbg.CreateTarget(exe)
+self.assertTrue(target, VALID_TARGET)
+
+breakpoint = target.BreakpointCreateBySourceRegex(
+self.BREAKPOINT_TEXT, lldb.SBFileSpec('main.cpp'))
+self.assertTrue(
+breakpoint.IsValid() and breakpoint.GetNumLocations() == 1,
+VALID_BREAKPOINT)
+
+process = target.LaunchSimple(
+None, None, self.get_process_working_directory())
+self.assertTrue(process, PROCESS_IS_VALID)
+
+from lldbsuite.test.lldbutil import get_stopped_thread
+
+# Verify 1st breakpoint location is hit.
+thread = get_stopped_thread(process, lldb.eStopReasonBreakpoint)
+self.assertTrue(
+thread.IsValid(),
+"There should be a thread stopped due to breakpoint")
+self.check_stopped_at_breakpoint_and_hit_once(thread, breakpoint)
+
+# Relaunch
+process.Kill()
+process = target.LaunchSimple(
+None, None, self.get_process_working_directory())
+self.assertTrue(process, PROCESS_IS_VALID)
+
+# Verify the hit counts are still one.
+thread = get_stopped_thread(process, lldb.eStopReasonBreakpoint)
+self.assertTrue(
+thread.IsValid(),
+"There should be a thread stopped due to breakpoint")
+self.check_stopped_at_breakpoint_and_hit_once(thread, breakpoint)
Index: lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/Makefile
===
--- /dev/null
+++ lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -313,8 +313,9 @@
 substrs=['stopped',
  

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Jim Ingham via Phabricator via lldb-commits]

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

This version looks good to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

The more we gather up the "things you have to do before an operation starts" or 
after it ends, etc. into WillDoOperation methods, the less ad hoc code you have 
in the callers that you have to remember to copy over if you are introducing a 
different way of doing the same operation.  It also expresses when the 
operation needs to be done, which if you just had a call out in Process::Attach 
or whatever you would not know.  Either way will work, but I think packaging 
these "do before" and "do after" tasks into methods makes everything clearer 
and easier to maintain.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

In D133858#3791290 , @mib wrote:

> I don't think it's necessary to add the `DoWillAttachToProcessWithID` method 
> and override it in each process plugin. I think you could have just reset the 
> `hit_counter` in the top-level `Process::{Attach,Launch}` class.

@jingham Thoughts on this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

In D133858#3790013 , @jingham wrote:

> Resetting the hit counts on rerun is a more useful behavior, so this is all 
> fine.  But the Target is the one that does all the breakpoint management, so 
> I think the resetting should go through the Target, not the Process.  And we 
> generally delegate "do on all breakpoints" operations to the BreakpointList, 
> so it would be more consistent with the current structure to go 
> Process::WillLaunch -> Target::ResetHitCounts -> 
> BreakpointList::ResetHitCounts.

@fdeazeve Pretty cool! But I agree with Jim, breakpoints are handled by the 
target not the process. As a side-note, in the current implementation, I don't 
think it's necessary to add the `DoWillAttachToProcessWithID` method and call 
override it in each process plugin. I think you could have


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 460176.
fdeazeve added a comment.

Also applied Jim's comment to the LocationList class, as this involves a single
lock, instead of one lock per location.
If this is wrong, please let me know and I'll revert.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

Files:
  lldb/include/lldb/Breakpoint/Breakpoint.h
  lldb/include/lldb/Breakpoint/BreakpointList.h
  lldb/include/lldb/Breakpoint/BreakpointLocation.h
  lldb/include/lldb/Breakpoint/BreakpointLocationList.h
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Target/Target.h
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Breakpoint/BreakpointList.cpp
  lldb/source/Breakpoint/BreakpointLocationList.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestAddressBreakpoints.py
  
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
  lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/Makefile
  
lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/TestBreakpointResetUponRun.py
  lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/main.cpp

Index: lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/main.cpp
@@ -0,0 +1,6 @@
+#include 
+
+int main(int argc, char const *argv[]) {
+  printf("Set a breakpoint here.\n");
+  return 0;
+}
Index: lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/TestBreakpointResetUponRun.py
===
--- /dev/null
+++ lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/TestBreakpointResetUponRun.py
@@ -0,0 +1,58 @@
+"""
+Test breakpoint hit count is reset when target runs.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+
+class HitcountResetUponRun(TestBase):
+BREAKPOINT_TEXT = 'Set a breakpoint here'
+
+def check_stopped_at_breakpoint_and_hit_once(self, thread, breakpoint):
+frame0 = thread.GetFrameAtIndex(0)
+location1 = breakpoint.FindLocationByAddress(frame0.GetPC())
+self.assertTrue(location1)
+self.assertEqual(location1.GetHitCount(), 1)
+self.assertEqual(breakpoint.GetHitCount(), 1)
+
+def test_hitcount_reset_upon_run(self):
+self.build()
+
+exe = self.getBuildArtifact("a.out")
+
+target = self.dbg.CreateTarget(exe)
+self.assertTrue(target, VALID_TARGET)
+
+breakpoint = target.BreakpointCreateBySourceRegex(
+self.BREAKPOINT_TEXT, lldb.SBFileSpec('main.cpp'))
+self.assertTrue(
+breakpoint.IsValid() and breakpoint.GetNumLocations() == 1,
+VALID_BREAKPOINT)
+
+process = target.LaunchSimple(
+None, None, self.get_process_working_directory())
+self.assertTrue(process, PROCESS_IS_VALID)
+
+from lldbsuite.test.lldbutil import get_stopped_thread
+
+# Verify 1st breakpoint location is hit.
+thread = get_stopped_thread(process, lldb.eStopReasonBreakpoint)
+self.assertTrue(
+thread.IsValid(),
+"There should be a thread stopped due to breakpoint")
+self.check_stopped_at_breakpoint_and_hit_once(thread, breakpoint)
+
+# Relaunch
+process.Kill()
+process = target.LaunchSimple(
+None, None, self.get_process_working_directory())
+self.assertTrue(process, PROCESS_IS_VALID)
+
+# Verify the hit counts are still one.
+thread = get_stopped_thread(process, lldb.eStopReasonBreakpoint)
+self.assertTrue(
+thread.IsValid(),
+"There should be a thread stopped due to breakpoint")
+self.check_stopped_at_breakpoint_and_hit_once(thread, breakpoint)
Index: lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/Makefile
===
--- /dev/null
+++ lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ 

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/source/Breakpoint/Breakpoint.cpp:334
+  for (size_t i = 0; i < GetNumLocations(); ++i)
+GetLocationAtIndex(i)->ResetHitCount();
+}

@jingham actually, should I apply the same idea to BreakpointLocationList?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

In D133858#3790013 , @jingham wrote:

> Resetting the hit counts on rerun is a more useful behavior, so this is all 
> fine.  But the Target is the one that does all the breakpoint management, so 
> I think the resetting should go through the Target, not the Process.  And we 
> generally delegate "do on all breakpoints" operations to the BreakpointList, 
> so it would be more consistent with the current structure to go 
> Process::WillLaunch -> Target::ResetHitCounts -> 
> BreakpointList::ResetHitCounts.

This makes sense! I've fixed this in the latest revision


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 460165.
fdeazeve added a comment.

Addressed review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

Files:
  lldb/include/lldb/Breakpoint/Breakpoint.h
  lldb/include/lldb/Breakpoint/BreakpointList.h
  lldb/include/lldb/Breakpoint/BreakpointLocation.h
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Target/Target.h
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Breakpoint/BreakpointList.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestAddressBreakpoints.py
  
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
  lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/Makefile
  
lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/TestBreakpointResetUponRun.py
  lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/main.cpp

Index: lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/main.cpp
@@ -0,0 +1,6 @@
+#include 
+
+int main(int argc, char const *argv[]) {
+  printf("Set a breakpoint here.\n");
+  return 0;
+}
Index: lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/TestBreakpointResetUponRun.py
===
--- /dev/null
+++ lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/TestBreakpointResetUponRun.py
@@ -0,0 +1,58 @@
+"""
+Test breakpoint hit count is reset when target runs.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+
+class HitcountResetUponRun(TestBase):
+BREAKPOINT_TEXT = 'Set a breakpoint here'
+
+def check_stopped_at_breakpoint_and_hit_once(self, thread, breakpoint):
+frame0 = thread.GetFrameAtIndex(0)
+location1 = breakpoint.FindLocationByAddress(frame0.GetPC())
+self.assertTrue(location1)
+self.assertEqual(location1.GetHitCount(), 1)
+self.assertEqual(breakpoint.GetHitCount(), 1)
+
+def test_hitcount_reset_upon_run(self):
+self.build()
+
+exe = self.getBuildArtifact("a.out")
+
+target = self.dbg.CreateTarget(exe)
+self.assertTrue(target, VALID_TARGET)
+
+breakpoint = target.BreakpointCreateBySourceRegex(
+self.BREAKPOINT_TEXT, lldb.SBFileSpec('main.cpp'))
+self.assertTrue(
+breakpoint.IsValid() and breakpoint.GetNumLocations() == 1,
+VALID_BREAKPOINT)
+
+process = target.LaunchSimple(
+None, None, self.get_process_working_directory())
+self.assertTrue(process, PROCESS_IS_VALID)
+
+from lldbsuite.test.lldbutil import get_stopped_thread
+
+# Verify 1st breakpoint location is hit.
+thread = get_stopped_thread(process, lldb.eStopReasonBreakpoint)
+self.assertTrue(
+thread.IsValid(),
+"There should be a thread stopped due to breakpoint")
+self.check_stopped_at_breakpoint_and_hit_once(thread, breakpoint)
+
+# Relaunch
+process.Kill()
+process = target.LaunchSimple(
+None, None, self.get_process_working_directory())
+self.assertTrue(process, PROCESS_IS_VALID)
+
+# Verify the hit counts are still one.
+thread = get_stopped_thread(process, lldb.eStopReasonBreakpoint)
+self.assertTrue(
+thread.IsValid(),
+"There should be a thread stopped due to breakpoint")
+self.check_stopped_at_breakpoint_and_hit_once(thread, breakpoint)
Index: lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/Makefile
===
--- /dev/null
+++ lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -313,8 +313,9 @@
 substrs=['stopped',
  'stop reason = breakpoint'])
 
-# The breakpoint should have a hit count of 2.
-lldbutil.check_breakpoint(self, bpno = 1, expected_hit_count = 2)
+  

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Jim Ingham via Phabricator via lldb-commits]

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

Resetting the hit counts on rerun is a more useful behavior, so this is all 
fine.  But the Target is the one that does all the breakpoint management, so I 
think the resetting should go through the Target, not the Process.  And we 
generally delegate "do on all breakpoints" operations to the BreakpointList, so 
it would be more consistent with the current structure to go 
Process::WillLaunch -> Target::ResetHitCounts -> BreakpointList::ResetHitCounts.




Comment at: lldb/source/Target/Process.cpp:2763-2766
+static void ResetHitCounts(Process ) {
+  for (const auto  : Proc.GetTarget().GetBreakpointList().Breakpoints())
+BP->ResetHitCount();
+}

fdeazeve wrote:
> JDevlieghere wrote:
> > Can this be a private member so we don't have to pass in `*this`? 
> No strong preferences on my part, but I had made it a free function because 
> it can be implemented in terms of the public behaviour, i.e. it doesn't rely 
> on implementation details.
Resetting all the hit counts in a BreakpointList seems to me a job of the 
BreakpointList.  Also, while DoWillLaunch/Attach is where this action belongs, 
which is how the Process gets involved, the Target is the one that manages the 
breakpoints in general.  So I think it would be better to have the Target do 
ResetHitCounts, and these Process calls should just dial up it's Target.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/source/Target/Process.cpp:2763-2766
+static void ResetHitCounts(Process ) {
+  for (const auto  : Proc.GetTarget().GetBreakpointList().Breakpoints())
+BP->ResetHitCount();
+}

JDevlieghere wrote:
> Can this be a private member so we don't have to pass in `*this`? 
No strong preferences on my part, but I had made it a free function because it 
can be implemented in terms of the public behaviour, i.e. it doesn't rely on 
implementation details.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Target/Process.cpp:2763-2766
+static void ResetHitCounts(Process ) {
+  for (const auto  : Proc.GetTarget().GetBreakpointList().Breakpoints())
+BP->ResetHitCount();
+}

Can this be a private member so we don't have to pass in `*this`? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/source/Breakpoint/Breakpoint.cpp:332
+void Breakpoint::ResetHitCount() {
+  m_hit_counter.Reset();
+  for (size_t i = 0; i < GetNumLocations(); ++i)

fdeazeve wrote:
> By the way, I don' think this counter is ever incremented anywhere. I'll 
> removing it in a separate patch and see what happens
Ah, nvm, `Breakpoint` is a `friend` of `BreakpointLocation`, and the latter 
increments this counter


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.
Herald added a subscriber: JDevlieghere.



Comment at: lldb/source/Breakpoint/Breakpoint.cpp:332
+void Breakpoint::ResetHitCount() {
+  m_hit_counter.Reset();
+  for (size_t i = 0; i < GetNumLocations(); ++i)

By the way, I don' think this counter is ever incremented anywhere. I'll 
removing it in a separate patch and see what happens


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133858/new/

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve created this revision.
Herald added a project: All.
fdeazeve requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

A common debugging pattern is to set a breakpoint that only stops after
a number of hits is recorded. The current implementation never resets
the hit count of breakpoints; as such, if a user re-`run`s their
program, the debugger will never stop on such a breakpoint again.

This behavior is arguably undesirable, as it renders such breakpoints
ineffective on all but the first run. This commit changes the
implementation of the `Will{Launch, Attach}` methods so that they reset
the _target's_ breakpoint hitcounts.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133858

Files:
  lldb/include/lldb/Breakpoint/Breakpoint.h
  lldb/include/lldb/Breakpoint/BreakpointLocation.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Target/Process.cpp
  
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestAddressBreakpoints.py
  
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
  lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/Makefile
  
lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/TestBreakpointResetUponRun.py
  lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/main.cpp

Index: lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/main.cpp
@@ -0,0 +1,6 @@
+#include 
+
+int main(int argc, char const *argv[]) {
+  printf("Set a breakpoint here.\n");
+  return 0;
+}
Index: lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/TestBreakpointResetUponRun.py
===
--- /dev/null
+++ lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/TestBreakpointResetUponRun.py
@@ -0,0 +1,58 @@
+"""
+Test breakpoint hit count is reset when target runs.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+
+class HitcountResetUponRun(TestBase):
+BREAKPOINT_TEXT = 'Set a breakpoint here'
+
+def check_stopped_at_breakpoint_and_hit_once(self, thread, breakpoint):
+frame0 = thread.GetFrameAtIndex(0)
+location1 = breakpoint.FindLocationByAddress(frame0.GetPC())
+self.assertTrue(location1)
+self.assertEqual(location1.GetHitCount(), 1)
+self.assertEqual(breakpoint.GetHitCount(), 1)
+
+def test_hitcount_reset_upon_run(self):
+self.build()
+
+exe = self.getBuildArtifact("a.out")
+
+target = self.dbg.CreateTarget(exe)
+self.assertTrue(target, VALID_TARGET)
+
+breakpoint = target.BreakpointCreateBySourceRegex(
+self.BREAKPOINT_TEXT, lldb.SBFileSpec('main.cpp'))
+self.assertTrue(
+breakpoint.IsValid() and breakpoint.GetNumLocations() == 1,
+VALID_BREAKPOINT)
+
+process = target.LaunchSimple(
+None, None, self.get_process_working_directory())
+self.assertTrue(process, PROCESS_IS_VALID)
+
+from lldbsuite.test.lldbutil import get_stopped_thread
+
+# Verify 1st breakpoint location is hit.
+thread = get_stopped_thread(process, lldb.eStopReasonBreakpoint)
+self.assertTrue(
+thread.IsValid(),
+"There should be a thread stopped due to breakpoint")
+self.check_stopped_at_breakpoint_and_hit_once(thread, breakpoint)
+
+# Relaunch
+process.Kill()
+process = target.LaunchSimple(
+None, None, self.get_process_working_directory())
+self.assertTrue(process, PROCESS_IS_VALID)
+
+# Verify the hit counts are still one.
+thread = get_stopped_thread(process, lldb.eStopReasonBreakpoint)
+self.assertTrue(
+thread.IsValid(),
+"There should be a thread stopped due to breakpoint")
+self.check_stopped_at_breakpoint_and_hit_once(thread, breakpoint)
Index: lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/Makefile
===
--- /dev/null
+++ lldb/test/API/functionalities/breakpoint/breakpoint_reset_upon_run/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
---