address @fjricci's review comments

facebookarchive · Aug 22, 2017 · 72c604c · 72c604c
1 parent 278515d
commit 72c604c
Show file tree

Hide file tree

Showing 4 changed files with 8 additions and 51 deletions.
diff --git a/Headers/DebugServer2/Core/HardwareBreakpointManager.h b/Headers/DebugServer2/Core/HardwareBreakpointManager.h
@@ -42,15 +42,16 @@ class HardwareBreakpointManager : public BreakpointManager {
   virtual ErrorCode disableLocation(int idx, Target::Thread *thread);
 
 protected:
-  bool hardwareReadonlyWorkaround(Address const &address, Site &site);
+  bool softwareImplementationOfReadonlyWatchpoints(Address const &address,
+                                                   Site &site);
   ErrorCode storeNewValueAtAddress(Address const &address);
 
 protected:
   bool enabled(Target::Thread *thread = nullptr) const override;
 
 public:
   virtual size_t maxWatchpoints();
-  virtual bool checkIfWrittenTo(Address const &address);
+  virtual bool wasWritten(Address const &address);
 
 public:
   void enable(Target::Thread *thread = nullptr) override;

diff --git a/Sources/Core/ARM/HardwareBreakpointManager.cpp b/Sources/Core/ARM/HardwareBreakpointManager.cpp
@@ -35,10 +35,8 @@ size_t HardwareBreakpointManager::maxWatchpoints() {
   return _process->getMaxWatchpoints();
 }
 
-bool HardwareBreakpointManager::checkIfWrittenTo(Address const &address) {
-  bool implemented = false;
-  DS2ASSERT(implemented);
-  return implemented;
+bool HardwareBreakpointManager::wasWritten(Address const &address) {
+  DS2BUG("not implemented");
 }
 
 ErrorCode HardwareBreakpointManager::isValid(Address const &address,

diff --git a/Sources/Core/HardwareBreakpointManager.cpp b/Sources/Core/HardwareBreakpointManager.cpp
@@ -21,7 +21,6 @@ HardwareBreakpointManager::HardwareBreakpointManager(
     Target::ProcessBase *process)
     : super(process), _locations(maxWatchpoints()) {}
 
-
 HardwareBreakpointManager::~HardwareBreakpointManager() {}
 
 ErrorCode HardwareBreakpointManager::add(Address const &address, Type type,
@@ -105,7 +104,7 @@ HardwareBreakpointManager::softwareImplementationOfReadonlyWatchpoints(
   // if this is a readonly hardware watchpoint
   case kModeRead:
     // if written to, return false. if not, invoke super.
-    return checkIfWrittenTo(address) ? false : super::hit(address, site);
+    return wasWritten(address) ? false : super::hit(address, site);
     break;
   default:
     return super::hit(address, site);
@@ -133,47 +132,6 @@ bool HardwareBreakpointManager::hit(Address const &address, Site &site) {
   return ret;
 }
 
-// workaround for hardware readonly watchpoints not being implememted in
-// hardware ¯\_(ツ)_/¯ https://github.com/facebook/ds2/issues/257
-bool HardwareBreakpointManager::hardwareReadonlyWorkaround(
-  Address const &address, Site &site) {
-  switch (site.mode) {
-  // if this is a readonly hardware watchpoint
-  case kModeRead:
-    // if written to, return false. if not, invoke super.
-    return checkIfWrittenTo(address) ? false : super::hit(address, site);
-    break;
-  default:
-    return super::hit(address, site);
-  }
-}
-
-ErrorCode HardwareBreakpointManager::storeNewValueAtAddress(
-  Address const &address) {
-  uint64_t value;
-  CHK(_process->readMemory(address, &value, sizeof(value)));
-
-  // store the new value at address in _locations
-  auto loc = std::find_if(_locations.begin(), _locations.end(),
-                          [address](const Location &l){
-                            return l.address == address;
-                          });
-  if (loc == _locations.end()) {
-     // exception. It isn't  possible to hit at an address that isn't enabled
-    return kErrorInvalidAddress;
-  } else {
-    _locations[loc - _locations.begin()].value = value;
-    return kSuccess;
-  }
-}
-
-bool HardwareBreakpointManager::hit(Address const &address, Site &site) {
-  auto ret = hardwareReadonlyWorkaround(address, site);
-  DS2ASSERT(storeNewValueAtAddress(address) == kSuccess);
-
-  return ret;
-}
-
 int HardwareBreakpointManager::getAvailableLocation() {
   DS2ASSERT(_locations.size() == maxWatchpoints());
   if (_sites.size() == maxWatchpoints()) {
@@ -183,7 +141,7 @@ int HardwareBreakpointManager::getAvailableLocation() {
   auto it = std::find(_locations.begin(), _locations.end(), 0);
   DS2ASSERT(it != _locations.end());
 
-  return (it - _locations.begin());
+  return it - _locations.begin();
 }
 
 void HardwareBreakpointManager::enumerateThreads(

diff --git a/Sources/Core/X86/HardwareBreakpointManager.cpp b/Sources/Core/X86/HardwareBreakpointManager.cpp
@@ -32,7 +32,7 @@ size_t HardwareBreakpointManager::maxWatchpoints() {
   return 4; // dr0, dr1, dr2, dr3
 }
 
-bool HardwareBreakpointManager::checkIfWrittenTo(Address const &address) {
+bool HardwareBreakpointManager::wasWritten(Address const &address) {
   Site site;
   try {
     site = _sites.at(address);