[Lldb-commits] [PATCH] D129814: Fix stepping over watchpoints in architectures that raise the exception before executing the instruction

Thu Jul 14 16:10:03 PDT 2022

jingham created this revision.
jingham added reviewers: JDevlieghere, jasonmolenda, labath, clayborg.
Herald added subscribers: atanasyan, kristof.beyls, arichardson, sdardis.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

On some architectures - e.g. aarch64 - the watchpoint exception is raised before the instruction that would have triggered it gets run.  We want to report old/new values, however, so we need to have executed the instruction before reporting the stop.  That means before reporting the stop, we need to do one stepi on the thread that got the watchpoint exception.

The original version of this code implemented this incorrectly.  It did the step over synchronously in the StopInfoWatchpoint::ShouldStopSynchronous.  You should NEVER run the target in ShouldStopSynchronous.  If you do that, then any other threads that had interesting pending stop info actions won't be able to perform their action because the target will have moved on and invalidated their StopInfo's.

You should always do this kind of thing by pushing the action you want to perform onto the ThreadPlanStack for the thread in question.  That way all the threads get to register what they need to do, and then the ThreadList runner can arbitrate among them and do the right thing.  For instance, in the case of stepping over watchpoints, each thread stopped on the watchpoint pushes a "step instruction" thread plan that re-instates the original watchpoint StopInfo when complete.  If there is more than one thread in this state, lldb notices each of them has a "stop others" plan, and one by one lets each thread step over the watchpoint.  When all this work is done, we report all the threads stopped at their watchpoints.

I also fixed a bug in the same code I noticed while reworking this.  We were not decrementing the HitCount for the watchpoint if the condition fails.  We consider a failed condition to mean "the stop point wasn't hit".  It's weird to see a hit count of 5 when the target only told you about one stop...  But that wasn't being enforced in the StopInfoWatchpoint::PerformAction.

There was a test that was expecting the now-incorrect value of the hit count in this case, so I fixed that, and also all the tests that were failing because of concurrent access were turned on again and run successfully for me.

There's a bit of funniness for MIPS in the watchpoint support.  I am pretty sure I got that right, but I don't have anything MIPS to test that on.  The same concurrent watchpoint tests that were marked failing for arm64 Darwin are also marked failed for MIPS, but I am not sure if that's for the same reason.  Anyway, if someone has access to a MIPS system, it would be good to try this out and see if it works there as well.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129814

Files:
  lldb/include/lldb/Breakpoint/Watchpoint.h
  lldb/include/lldb/Target/StopInfo.h
  lldb/source/Target/StopInfo.cpp
  lldb/test/API/commands/watchpoints/watchpoint_commands/condition/TestWatchpointConditionCmd.py
  lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentDelayWatchBreak.py
  lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentManyWatchpoints.py
  lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentNWatchNBreak.py
  lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalNWatchNBreak.py
  lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatch.py
  lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatchBreak.py
  lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointThreads.py
  lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py
  lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py
  lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneSignal.py
  lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentWatchBreak.py

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D129814.444825.patch
Type: text/x-patch
Size: 21659 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20220714/0c43d177/attachment-0001.bin>