Implement hardware breakpoint support for Linux-x86 #243

fjricci · 2016-03-31T19:19:54Z

No description provided.

facebook-github-bot · 2016-03-31T19:29:51Z

By analyzing the blame information on this pull request, we identified @sas, @fjricci and @a20012251 to be potential reviewers.

sas · 2016-04-04T21:57:14Z

Sources/Architecture/ARM/SoftwareBreakpointManager.cpp

@@ -35,7 +35,10 @@ void SoftwareBreakpointManager::clear() {
 }

 ErrorCode SoftwareBreakpointManager::add(Address const &address, Type type,
-                                         size_t size) {
+                                         size_t size, Mode mode) {
+  if (mode != kModeExec)


I think you can make this a DS2ASSERT, given that we don't pass user data directly here, and that all callers of ::add will always call it with kModeExec.

I think the issue with doing that is that you could theoretically have software watchpoints (gdb does), and so I'm not sure that it's really an assertion-level failure to try to create one, even if we don't support them currently?

Yeah that's a good point.

On Mon, Apr 4, 2016 at 3:54 PM, Francis Ricci notifications@github.com
wrote:

In Sources/Architecture/ARM/SoftwareBreakpointManager.cpp
#243 (comment):

@@ -35,7 +35,10 @@ void SoftwareBreakpointManager::clear() {
}

ErrorCode SoftwareBreakpointManager::add(Address const &address, Type type,

size_t size) {

size_t size, Mode mode) {

if (mode != kModeExec)

I think the issue with doing that is that you could theoretically have
software watchpoints (gdb does), and so I'm not sure that it's really an
assertion-level failure to try to create one, even if we don't support them
currently?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/facebook/ds2/pull/243/files/2b7add545bce75bd539f7f39cb938aa59689d8ae#r58462758

Stephane Sezer

You can move this check in super::add() and avoid having to duplicate it in the x86 counterpart of this file.

Also, I looked at software watchpoints a little bit, and they are handled by the client directly. The debug server has nothing to do with them, and the protocol doesn't support setting "software watchpoints". PR#268 should make us safe to put an assertion there.

There is no SoftwareBreakpointManager superclass, "super" here is BreakpointManager, and I can't put it there, or it will break hardware breakpoints. But I can change it to an assert.

Ah true.

On Friday, April 8, 2016, Francis Ricci notifications@github.com wrote:

In Sources/Architecture/ARM/SoftwareBreakpointManager.cpp
#243 (comment):

@@ -35,7 +35,10 @@ void SoftwareBreakpointManager::clear() {
}

ErrorCode SoftwareBreakpointManager::add(Address const &address, Type type,

size_t size) {

size_t size, Mode mode) {

if (mode != kModeExec)

There is no SoftwareBreakpointManager superclass, "super" here is
BreakpointManager, and I can't put it there, or it will break hardware
breakpoints. But I can change it to an assert.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/facebook/ds2/pull/243/files/2b7add545bce75bd539f7f39cb938aa59689d8ae#r59102598

Stephane Sezer

facebook-github-bot · 2016-04-07T00:06:20Z

@fjricci updated the pull request.

sas · 2016-08-09T00:31:35Z

Committed some more patches as 3ccacba..6b668bd.

sas · 2016-08-09T00:36:23Z

Sources/Target/Common/ThreadBase.cpp

+
+ErrorCode ThreadBase::beforeResume() {
+  if (!_process->isAlive())
+    return kErrorProcessNotFound;


Braces and stuff :p

sas · 2016-08-09T18:29:31Z

Sources/GDBRemote/DebugSessionImpl.cpp

@@ -1004,6 +1004,10 @@ DebugSessionImplBase::onResume(Session &session,
 #endif

      case StopInfo::kReasonThreadEntry:
+        error = _process->currentThread()->beforeResume();


Pro-tip: You can use a fancy macro here.

sas · 2016-08-16T23:52:34Z

Committed the CHK patch as 9d0f300.

Fixes problem where hardware breakpoints were not enabled on newly created threads.

sas · 2016-09-23T19:18:23Z

Sources/Core/SoftwareBreakpointManager.cpp

+}
+
+bool SoftwareBreakpointManager::enabled(Target::Thread *thread) const {
+  if (thread != nullptr) {


This should be a DS2_ASSERT.

sas · 2016-09-23T19:19:42Z

Sources/Core/SoftwareBreakpointManager.cpp

+}
+
+void SoftwareBreakpointManager::disable(Target::Thread *thread) {
+  super::disable(thread);


Do we have to pass thread here? Can't we just DS2_ASSERT(thread == nullptr) and call supper::disable()?

I believe this was to unify the api with hardware breakpoints.

sas · 2016-09-23T19:20:11Z

Sources/Core/SoftwareBreakpointManager.cpp

  ErrorCode error;
  ByteVector old = _insns[site.address];

+  if (thread != nullptr) {
+    DS2LOG(Warning, "thread-specific software breakpoints are unsupported");


This should be a DS2_ASSERT as well.

We had a conversation with this one, and I think the conclusion was that since gdb supports these, we should leave the option available.

sas · 2016-09-23T21:04:26Z

Merged as fc285ce..3471878.

🎉 🎉 🎉 🎉 🎉

facebook-github-bot added GH Review: review-needed CLA Signed labels Mar 31, 2016

fjricci force-pushed the x86_impl branch from c2c60ad to df624fb Compare March 31, 2016 20:05

fjricci changed the title ~~Add breakpoint mode to breakpoint manager api~~ Progress toward supporting hardware breakpoints Apr 1, 2016

fjricci force-pushed the x86_impl branch 8 times, most recently from eaec15d to 5e8356f Compare April 4, 2016 20:30

sas reviewed Apr 4, 2016
View reviewed changes

fjricci force-pushed the x86_impl branch 5 times, most recently from 2b454aa to 523f690 Compare April 7, 2016 00:05

fjricci force-pushed the x86_impl branch 3 times, most recently from 76df0f2 to 4592851 Compare April 7, 2016 22:05

sas force-pushed the master branch from 7045acc to ff1e56c Compare April 7, 2016 22:14

fjricci force-pushed the x86_impl branch 2 times, most recently from 366459a to 6971fd7 Compare April 8, 2016 20:39

fjricci closed this Jul 5, 2016

fjricci reopened this Jul 5, 2016

fjricci force-pushed the x86_impl branch 4 times, most recently from 25b0271 to 0ef37b1 Compare July 7, 2016 13:50

ghost added the CLA Signed label Jul 12, 2016

sas reviewed Aug 9, 2016
View reviewed changes

fjricci force-pushed the x86_impl branch 2 times, most recently from 0ee677d to dbb0683 Compare August 9, 2016 17:11

sas reviewed Aug 9, 2016
View reviewed changes

fjricci force-pushed the x86_impl branch 4 times, most recently from 10ba488 to 0123613 Compare August 16, 2016 21:58

fjricci force-pushed the x86_impl branch from 0123613 to 309ae8e Compare August 17, 2016 17:08

fjricci added 3 commits September 23, 2016 10:38

Implement thread-specific hardware breakpoint enabling

2f37466

Enable hardware breakpoints on each individual thread

bf0dd1a

Fixes problem where hardware breakpoints were not enabled on newly created threads.

Re-enable all watchpoint tests

5915328

fjricci force-pushed the x86_impl branch from 309ae8e to 5915328 Compare September 23, 2016 17:38

sas removed the GH Review: review-needed label Sep 23, 2016

sas reviewed Sep 23, 2016

View reviewed changes

sas suggested changes Sep 23, 2016

View reviewed changes

sas closed this Sep 23, 2016

fjricci deleted the x86_impl branch September 23, 2016 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement hardware breakpoint support for Linux-x86 #243

Implement hardware breakpoint support for Linux-x86 #243

fjricci commented Mar 31, 2016

facebook-github-bot commented Mar 31, 2016

sas Apr 4, 2016

fjricci Apr 4, 2016

sas Apr 4, 2016

sas Apr 8, 2016

sas Apr 8, 2016

fjricci Apr 8, 2016

sas Apr 9, 2016

facebook-github-bot commented Apr 7, 2016

sas commented Aug 9, 2016

sas Aug 9, 2016

sas Aug 9, 2016

sas commented Aug 16, 2016

sas Sep 23, 2016

sas Sep 23, 2016

fjricci Sep 23, 2016

sas Sep 23, 2016

fjricci Sep 23, 2016

sas commented Sep 23, 2016

Implement hardware breakpoint support for Linux-x86 #243

Implement hardware breakpoint support for Linux-x86 #243

Conversation

fjricci commented Mar 31, 2016

facebook-github-bot commented Mar 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 7, 2016

sas commented Aug 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sas commented Aug 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sas commented Sep 23, 2016