Support read-only hardware watchpoints on x86 #510

AndrewSB · 2017-08-15T23:07:25Z

No description provided.

AndrewSB · 2017-08-18T20:17:36Z

Sources/Core/HardwareBreakpointManager.cpp

+                          });
+  if (loc == _locations.end()) {
+     // exception. It isn't  possible to hit at an address that isn't enabled
+    return kErrorInvalidAddress;


not sure if this is the right error to return

I'm fine with a DS2ASSERT(loc != _locations.end());

fjricci · 2017-08-18T20:27:16Z

Headers/DebugServer2/Core/HardwareBreakpointManager.h

-  std::vector<uint64_t> _locations;
+  struct Location {
+    Location()
+      :address(0), value(0){}


You'll need to run clang-format on this. You can either:

Install and run clang-format manually (I don't think this works on devservers)

Look at the Travis "STYLE" failures and manually apply

install clang-format and run: TARGET=Style ./Support/Testing/Travis/script.sh (see previous comment about no devservers, this will probably require temporarily changing script.sh to use an unversioned clang-format)

fjricci · 2017-08-18T20:33:31Z

Sources/Core/HardwareBreakpointManager.cpp

+  auto loc = std::find_if(_locations.begin(), _locations.end(),
+                          [address](const Location &l){
+                            return l.address == address;
+                          });


Please factor out the comparator and pass around as a function pointer.

you mean define a function like

inline bool addressInMap(const HardwareBreakpointManager::Location &location, const u_int64_t address) __attribute__((always_inline)) { return location.address == address; }

and use it everywhere like

std::find_if(_locations.begin(), _locations.end(), [address](const Location &location) { return addressInMap(location, address); });

?

Actually, I have a different idea. We could just use a std::map for _locations, and remove the DS2ASSERT(_locations.size() == maxWatchpoints()); (since you can't fix the size of a map). we already check _sites.size() >= maxWatchpoints() in HardwareBreakpointManager::add, so there's really no reason to also restrict the size of _locations. If you use <address, value> as <key, value> in a map, you don't need the find_if's anymore.

thats great, I'll do that in a commit in the next hour or so :)

fjricci · 2017-08-18T20:35:05Z

Sources/Core/ARM/HardwareBreakpointManager.cpp

+bool HardwareBreakpointManager::checkIfWrittenTo(Address const &address) {
+  bool implemented = false;
+  DS2ASSERT(implemented);
+  return implemented;


You can just put a DS2BUG("not implemented") instead

fjricci · 2017-08-18T20:35:23Z

Sources/Core/ARM/HardwareBreakpointManager.cpp

@@ -35,6 +35,12 @@ size_t HardwareBreakpointManager::maxWatchpoints() {
  return _process->getMaxWatchpoints();
 }

+bool HardwareBreakpointManager::checkIfWrittenTo(Address const &address) {


wasWritten()

fjricci · 2017-08-18T20:36:32Z

Sources/Core/HardwareBreakpointManager.cpp

@@ -102,6 +104,44 @@ ErrorCode HardwareBreakpointManager::disableLocation(Site const &site,
  return error;
 }

+// workaround for hardware readonly watchpoints not being implememted in
+// hardware ¯\_(ツ)_/¯ https://github.com/facebook/ds2/issues/257


I don't think this really qualifies as a workaround, since the lack of hardware implementation is an intentional feature. You could just say that this is a software implementation of read-only watchpoints or something.

fjricci · 2017-08-18T20:37:00Z

Sources/Core/HardwareBreakpointManager.cpp

@@ -102,6 +104,44 @@ ErrorCode HardwareBreakpointManager::disableLocation(Site const &site,
  return error;
 }

+// workaround for hardware readonly watchpoints not being implememted in
+// hardware ¯\_(ツ)_/¯ https://github.com/facebook/ds2/issues/257
+bool HardwareBreakpointManager::hardwareReadonlyWorkaround(


You can probably just inline this function.

fjricci · 2017-08-18T20:37:21Z

Sources/Core/HardwareBreakpointManager.cpp

+  auto loc = std::find_if(_locations.begin(), _locations.end(),
+                          [address](const Location &l){
+                            return l.address == address;
+                          });


see previous comment about factoring this out.

fjricci · 2017-08-18T20:37:49Z

Sources/Core/HardwareBreakpointManager.cpp

@@ -113,7 +153,7 @@ int HardwareBreakpointManager::getAvailableLocation() {
                              return l.address == 0;
                            });
  DS2ASSERT(it != _locations.end());
-  
+


NIT: remove accidental whitespace change

fjricci · 2017-08-18T20:38:09Z

Sources/Core/X86/HardwareBreakpointManager.cpp

@@ -32,6 +32,27 @@ size_t HardwareBreakpointManager::maxWatchpoints() {
  return 4; // dr0, dr1, dr2, dr3
 }

+bool HardwareBreakpointManager::checkIfWrittenTo(Address const &address) {


wasWritten()

fjricci · 2017-08-18T20:39:12Z

Sources/Core/HardwareBreakpointManager.cpp

+                          });
+  if (loc == _locations.end()) {
+     // exception. It isn't  possible to hit at an address that isn't enabled
+    return kErrorInvalidAddress;


I'm fine with a DS2ASSERT(loc != _locations.end());

AndrewSB · 2017-08-21T22:11:01Z

@fjricci should I squash all/some of my commits?

fjricci · 2017-08-21T22:21:49Z

Stacked commits are good (much easier to review and understand than one big blob), but they should be stacked by functionality and not by time (so a commit like fix style or address comments should be squashed). Each point in the stack should have correct style and be a logical step toward the end result. For example, we don't want a commit to add value to _locations and then another commit to remove it, but it's fine/good to add one level of functionality at a time.

AndrewSB · 2017-08-21T23:19:21Z

sounds good! I'll rebase functionally 😄

fjricci · 2017-08-22T15:23:32Z

Pro tip: use git rebase --ignore-date on your local branch to make the commits show up in the correct order here after squashing.

AndrewSB · 2017-08-23T03:04:03Z

@fjricci I'm currently failing on Travis for 2 reasons. One is related to STYLE, I'm using clang-format v5 on my machine, would it make sense to update Travis' version of clang format from 4 to 5 as well?

For the second failure, I'll look at it closer once I fix the style issue.

fjricci · 2017-08-23T13:31:06Z

We like to keep our llvm tools all on the same version, and the stable version of llvm 5 hasn't been released yet (slated for next month I think). We should wait until then to update our clang-format. Do you have access to clang-format-4?

AndrewSB · 2017-08-23T21:25:04Z

I can build it 😄

fjricci · 2017-08-23T21:30:47Z

Yeah, I mean, that's what I do. (I even have it as a command in my bashrc, which I find helpful: https://github.com/fjricci/configfiles/blob/1be83f5e813612d382c3c47d734d68efcc3c5d58/bash/bashrc#L28-L37) If you're going to be doing more toolchain work, you'll end up needing llvm builds anyway.

sas

Couple of general comments on this PR before I do a real review:

Please try to respect the commit message format of git as much as possible: https://chris.beams.io/posts/git-commit/#seven-rules
Please make sure that each commit you submit is atomic and can be merged as-is when we're done with the review. For instance, the commit titled "address @fjricci's review comments" should be split squashed with previous commits in the branch where applicable.

AndrewSB · 2017-08-28T16:48:38Z

@sas I'll try to clean up my commits & their messages today

fjricci · 2017-09-18T19:00:21Z

What's the status of this?

sas · 2017-09-28T18:09:52Z

Bump.

AndrewSB · 2017-10-03T22:18:44Z

This flew under my radar. Checking this out now!

TODO

make sure the git commits are sensical
fix STYLE by landing [WIP] Upgrade our llvm (clang+lldb) to 5.0.0 #525 and using clang-format 5.0 instead
rebase
fix actual test breakages (like in https://travis-ci.org/facebook/ds2/jobs/269802643)

fjricci · 2017-10-03T22:22:07Z

😮 TIL about TODO lists on github 😮

For the software implementation of readonly hardware breakpoints, we need to keep an up-to-date map of the values at memory addresses. That way when our read|write breakpoint is triggered, we can check to see if the value at that memory address has changed before deciding whether or not to halt at that watchpoint The downside with this approach is if one re-writes the existing value at one of these hardware readonly watchpoints, this implementation will halt there, thinking the value was read from (since the address was halted by our read|write breakpoint and the value is unchanged), instead of correctly ignoring that halt as a write.

So one can still run the travis script targeting STYlE even without having "clang-format-4.0" in one's PATH

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: Blame Revision:

lanza · 2018-06-27T23:44:11Z

no say bone

AndrewSB · 2018-06-28T00:43:09Z

😱😥

facebook-github-bot added the CLA Signed label Aug 15, 2017

AndrewSB force-pushed the master branch from 9de7bad to 9c8e965 Compare August 15, 2017 23:10

AndrewSB mentioned this pull request Aug 16, 2017

Support read-only hardware watchpoints on x86 #257

Closed

AndrewSB force-pushed the master branch 2 times, most recently from b35cb8f to c0c2ff3 Compare August 18, 2017 20:16

AndrewSB commented Aug 18, 2017

View reviewed changes

AndrewSB changed the title ~~[WIP] Support read-only hardware watchpoints on x86~~ Support read-only hardware watchpoints on x86 Aug 18, 2017

fjricci self-requested a review August 18, 2017 20:22

fjricci suggested changes Aug 18, 2017

View reviewed changes

fjricci requested a review from sas August 18, 2017 20:43

AndrewSB force-pushed the master branch 2 times, most recently from 5d04c9c to 72c604c Compare August 22, 2017 03:18

sas suggested changes Aug 28, 2017

View reviewed changes

AndrewSB force-pushed the master branch 4 times, most recently from cf4001a to 7bf2e61 Compare August 29, 2017 23:21

AndrewSB force-pushed the master branch 2 times, most recently from 49ae141 to 2d800ab Compare January 25, 2018 23:07

AndrewSB force-pushed the master branch from 2d800ab to 3f8f9ca Compare February 23, 2018 01:27

AndrewSB force-pushed the master branch 2 times, most recently from 8209681 to a2cb333 Compare April 7, 2018 02:44

AndrewSB and others added 6 commits May 21, 2018 15:19

Implement readonly watchpoints in BPManager::hit

351fabb

Implement storing current value at address in _sites

e023855

[travis] Add optional $CFORMAT env variable

72c6a50

So one can still run the travis script targeting STYlE even without having "clang-format-4.0" in one's PATH

Address @fjricci's review comments

34441b0

Remove NYI warning for readonly hardware watchpoints

d7744f2

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: Blame Revision:

AndrewSB force-pushed the master branch from a2cb333 to d7744f2 Compare May 21, 2018 22:19

lanza closed this Jun 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support read-only hardware watchpoints on x86 #510

Support read-only hardware watchpoints on x86 #510

AndrewSB commented Aug 15, 2017

AndrewSB Aug 18, 2017

fjricci Aug 18, 2017

fjricci Aug 18, 2017

fjricci Aug 18, 2017

AndrewSB Aug 19, 2017 •

edited

fjricci Aug 21, 2017

AndrewSB Aug 21, 2017

fjricci Aug 18, 2017

fjricci Aug 18, 2017

fjricci Aug 18, 2017

fjricci Aug 18, 2017

fjricci Aug 18, 2017

fjricci Aug 18, 2017

fjricci Aug 18, 2017

fjricci Aug 18, 2017

AndrewSB commented Aug 21, 2017

fjricci commented Aug 21, 2017

AndrewSB commented Aug 21, 2017

fjricci commented Aug 22, 2017

AndrewSB commented Aug 23, 2017

fjricci commented Aug 23, 2017

AndrewSB commented Aug 23, 2017

fjricci commented Aug 23, 2017

sas left a comment

AndrewSB commented Aug 28, 2017

fjricci commented Sep 18, 2017

sas commented Sep 28, 2017

AndrewSB commented Oct 3, 2017 •

edited by lanza

fjricci commented Oct 3, 2017

lanza commented Jun 27, 2018

AndrewSB commented Jun 28, 2018

Support read-only hardware watchpoints on x86 #510

Support read-only hardware watchpoints on x86 #510

Conversation

AndrewSB commented Aug 15, 2017

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

AndrewSB Aug 19, 2017 • edited

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

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

AndrewSB commented Aug 21, 2017

fjricci commented Aug 21, 2017

AndrewSB commented Aug 21, 2017

fjricci commented Aug 22, 2017

AndrewSB commented Aug 23, 2017

fjricci commented Aug 23, 2017

AndrewSB commented Aug 23, 2017

fjricci commented Aug 23, 2017

sas left a comment

Choose a reason for hiding this comment

AndrewSB commented Aug 28, 2017

fjricci commented Sep 18, 2017

sas commented Sep 28, 2017

AndrewSB commented Oct 3, 2017 • edited by lanza

TODO

fjricci commented Oct 3, 2017

lanza commented Jun 27, 2018

AndrewSB commented Jun 28, 2018

AndrewSB Aug 19, 2017 •

edited

AndrewSB commented Oct 3, 2017 •

edited by lanza