[lldb] [NFC] Remove some dead code from Watchpoint class, and a method that makes no sense

[jasonmolenda]

Summary

Johnny Chen added this code to Watchpoint originally in 2012 via 25c0eb4a38d20a4ac37714312cfcabdd082ff74f / llvm-svn: 161892 including the method Watchpoint::IncrementFalseAlarmsAndReviseHitCount. Most of the code from this change has slowly been removed over the years, but that method remained, and was used for watchpoints hit on a MIPS target where the access was *near* a watchpoint, but not actually within a watched region (the hardware doesn't distinguish the low 3 bits, and lldb has an emulation of the LD/ST instruction to disambiguate). The MIPS case was trying to use this method to decrement the hit count, which had already been incremented earlier when the watchpoint hit was first broadcast. The method is doing signed math on unsigned counters and I can't honestly tell what it was meant to be doing in the first place.

We have a method to decrement the hit count. Use it.

Also remove a couple of ivars in Watchpoint that aren't used anywhere.

[DavidSpickett]

I don't understand the logic for if the hit counter is < the number of false alarms. I'm not even sure that's possible given that a false alarm would have to come from a reported hit, wouldn't it?

Maybe there is a behaviour difference when using UndohitCount but I don't think it's worth worrying about. LGTM.

[jasonmolenda]

Thanks David, yeah I came to the same conclusion about that method. I made hacked up lldb that behaved like the MIPS case to confirm that the hit count is incremented before we decide it's a fake stop & decrement it again.

lldb/include/lldb/Breakpoint/Watchpoint.h

private:
     const WatchpointEventData &operator=(const WatchpointEventData &) = delete;
   };
 
   Watchpoint(Target &target, lldb::addr_t addr, uint32_t size,
              const CompilerType *type, bool hardware = true);
 
   ~Watchpoint() override;
 
-  void IncrementFalseAlarmsAndReviseHitCount();
-
   bool IsEnabled() const;
 
   // This doesn't really enable/disable the watchpoint.   It is currently just
   // for use in the Process plugin's {Enable,Disable}Watchpoint, which should
   // be used instead.
   void SetEnabled(bool enabled, bool notify = true);
 
   bool IsHardware() const override;
private:
   // undergoing a pair of temporary disable/enable actions to avoid recursively
   // triggering further watchpoint events.
   uint32_t m_disabled_count; // Keep track of the count that the watchpoint is
                              // disabled while in ephemeral mode.
   // At the end of the ephemeral mode when the watchpoint is to be enabled
   // again, we check the count, if it is more than 1, it means the user-
   // supplied actions actually want the watchpoint to be disabled!
   uint32_t m_watch_read : 1, // 1 if we stop when the watched data is read from
-      m_watch_write : 1,     // 1 if we stop when the watched data is written to
-      m_watch_was_read : 1, // Set to 1 when watchpoint is hit for a read access
-      m_watch_was_written : 1;  // Set to 1 when watchpoint is hit for a write
-                                // access
+      m_watch_write : 1;     // 1 if we stop when the watched data is written to
   uint32_t m_ignore_count;      // Number of times to ignore this watchpoint
-  uint32_t m_false_alarms;      // Number of false alarms.
   std::string m_decl_str;       // Declaration information, if any.
   std::string m_watch_spec_str; // Spec for the watchpoint.
   lldb::ValueObjectSP m_old_value_sp;
   lldb::ValueObjectSP m_new_value_sp;
   CompilerType m_type;
   Status m_error; // An error object describing errors associated with this
                   // watchpoint.
   WatchpointOptions

lldb/source/Breakpoint/Watchpoint.cpp

 using namespace lldb;
 using namespace lldb_private;
 
 Watchpoint::Watchpoint(Target &target, lldb::addr_t addr, uint32_t size,
                        const CompilerType *type, bool hardware)
     : StoppointSite(0, addr, size, hardware), m_target(target),
       m_enabled(false), m_is_hardware(hardware), m_is_watch_variable(false),
       m_is_ephemeral(false), m_disabled_count(0), m_watch_read(0),
-      m_watch_write(0), m_watch_was_read(0), m_watch_was_written(0),
-      m_ignore_count(0), m_false_alarms(0), m_being_created(true) {
+      m_watch_write(0), m_ignore_count(0), m_being_created(true) {
 
   if (type && type->IsValid())
     m_type = *type;
   else {
     // If we don't have a known type, then we force it to unsigned int of the
     // right size.
     auto type_system_or_err =
         target.GetScratchTypeSystemForLanguage(eLanguageTypeC);
bool Watchpoint::CaptureWatchedValue(const ExecutionContext &exe_ctx) {
   }
   m_new_value_sp = ValueObjectMemory::Create(
       exe_ctx.GetBestExecutionContextScope(), watch_name.GetStringRef(),
       watch_address, m_type);
   m_new_value_sp = m_new_value_sp->CreateConstantValue(watch_name);
   return (m_new_value_sp && m_new_value_sp->GetError().Success());
 }
 
-void Watchpoint::IncrementFalseAlarmsAndReviseHitCount() {
-  ++m_false_alarms;
-  if (m_false_alarms) {
-    if (m_hit_counter.GetValue() >= m_false_alarms) {
-      m_hit_counter.Decrement(m_false_alarms);
-      m_false_alarms = 0;
-    } else {
-      m_false_alarms -= m_hit_counter.GetValue();
-      m_hit_counter.Reset();
-    }
-  }
-}
-
 // RETURNS - true if we should stop at this breakpoint, false if we
 // should continue.
 
 bool Watchpoint::ShouldStop(StoppointCallbackContext *context) {
   m_hit_counter.Increment();
 
   return IsEnabled();
 }

lldb/source/Target/StopInfo.cpp

if (thread_sp) {
         // are finished.
         ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0));
         ProcessSP process_sp = exe_ctx.GetProcessSP();
 
         WatchpointSentry sentry(process_sp, wp_sp);
 
         if (m_silently_skip_wp) {
           m_should_stop = false;
-          wp_sp->IncrementFalseAlarmsAndReviseHitCount();
+          wp_sp->UndoHitCount();
         }
 
         if (wp_sp->GetHitCount() <= wp_sp->GetIgnoreCount()) {
           m_should_stop = false;
           m_should_stop_is_valid = true;
         }
 
         Debugger &debugger = exe_ctx.GetTargetRef().GetDebugger();