[Lldb-commits] [lldb] r284817 - Revert "Fix a race condition between "ephemeral watchpoint disable/enable" and continue in commands."
Jim Ingham via lldb-commits
lldb-commits at lists.llvm.org
Fri Oct 21 16:04:10 PDT 2016
Note that the WatchpointSentry logic predated the support for "!watchpoint_triggers_after", and it's real goal was to have accesses to the watchpoint in commands in the breakpoint action not re-trigger the watchpoint. The step-over-watchpoint logic was accidentally getting the benefit of this, but if that was all it was for, you wouldn't have needed any of the accounting for "real" vrs. "ephemeral" disables.
Anyway, the solution is to manually disable/enable the watchpoint when single stepping, since that is a simple bit of straight-line code, and then set up the watchpoint sentry to do just it's job.
That's:
https://reviews.llvm.org/D25875
Jim
> On Oct 21, 2016, at 3:52 AM, Pavel Labath via lldb-commits <lldb-commits at lists.llvm.org> wrote:
>
> Author: labath
> Date: Fri Oct 21 05:52:11 2016
> New Revision: 284817
>
> URL: http://llvm.org/viewvc/llvm-project?rev=284817&view=rev
> Log:
> Revert "Fix a race condition between "ephemeral watchpoint disable/enable" and continue in commands."
>
> This reverts commit r284795, as it breaks watchpoint handling on arm (and
> presumable all architectures that report watchpoint hits without executing the
> tripping instruction).
>
> There seems to be something fundamentally wrong with this patch: it uses
> process_sp->AddPreResumeAction to re-enable the watchpoint, but the whole point
> of the step-over-watchpoint logic (which AFAIK is the only user of this class) is
> to disable the watchpoint *after* we resume to do the single step.
>
> I have no idea how to fix this except by reverting the offending patch.
>
> Modified:
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandPython.py
> lldb/trunk/source/Breakpoint/Watchpoint.cpp
> lldb/trunk/source/Target/StopInfo.cpp
>
> Modified: lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandPython.py
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandPython.py?rev=284817&r1=284816&r2=284817&view=diff
> ==============================================================================
> --- lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandPython.py (original)
> +++ lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandPython.py Fri Oct 21 05:52:11 2016
> @@ -54,6 +54,9 @@ class WatchpointPythonCommandTestCase(Te
> # Add a breakpoint to set a watchpoint when stopped on the breakpoint.
> lldbutil.run_break_set_by_file_and_line(
> self, None, self.line, num_expected_locations=1)
> +# self.expect("breakpoint set -l %d" % self.line, BREAKPOINT_CREATED,
> +# startstr = "Breakpoint created: 1: file ='%s', line = %d, locations = 1" %
> +# (self.source, self.line))#
>
> # Run the program.
> self.runCmd("run", RUN_SUCCEEDED)
> @@ -128,6 +131,9 @@ class WatchpointPythonCommandTestCase(Te
> # Add a breakpoint to set a watchpoint when stopped on the breakpoint.
> lldbutil.run_break_set_by_file_and_line(
> self, None, self.line, num_expected_locations=1)
> +# self.expect("breakpoint set -l %d" % self.line, BREAKPOINT_CREATED,
> +# startstr = "Breakpoint created: 1: file ='%s', line = %d, locations = 1" %
> +# (self.source, self.line))#
>
> # Run the program.
> self.runCmd("run", RUN_SUCCEEDED)
> @@ -171,4 +177,3 @@ class WatchpointPythonCommandTestCase(Te
> # second hit and set it to 999
> self.expect("frame variable --show-globals cookie",
> substrs=['(int32_t)', 'cookie = 999'])
> -
>
> Modified: lldb/trunk/source/Breakpoint/Watchpoint.cpp
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Breakpoint/Watchpoint.cpp?rev=284817&r1=284816&r2=284817&view=diff
> ==============================================================================
> --- lldb/trunk/source/Breakpoint/Watchpoint.cpp (original)
> +++ lldb/trunk/source/Breakpoint/Watchpoint.cpp Fri Oct 21 05:52:11 2016
> @@ -232,7 +232,7 @@ void Watchpoint::TurnOffEphemeralMode()
> }
>
> bool Watchpoint::IsDisabledDuringEphemeralMode() {
> - return m_disabled_count > 1 && m_is_ephemeral;
> + return m_disabled_count > 1;
> }
>
> void Watchpoint::SetEnabled(bool enabled, bool notify) {
>
> Modified: lldb/trunk/source/Target/StopInfo.cpp
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/StopInfo.cpp?rev=284817&r1=284816&r2=284817&view=diff
> ==============================================================================
> --- lldb/trunk/source/Target/StopInfo.cpp (original)
> +++ lldb/trunk/source/Target/StopInfo.cpp Fri Oct 21 05:52:11 2016
> @@ -557,45 +557,27 @@ public:
> // performing watchpoint actions.
> class WatchpointSentry {
> public:
> - WatchpointSentry(ProcessSP p_sp, WatchpointSP w_sp) : process_sp(p_sp),
> - watchpoint_sp(w_sp) {
> - if (process_sp && watchpoint_sp) {
> + WatchpointSentry(Process *p, Watchpoint *w) : process(p), watchpoint(w) {
> + if (process && watchpoint) {
> const bool notify = false;
> - watchpoint_sp->TurnOnEphemeralMode();
> - process_sp->DisableWatchpoint(watchpoint_sp.get(), notify);
> - process_sp->AddPreResumeAction(SentryPreResumeAction, this);
> + watchpoint->TurnOnEphemeralMode();
> + process->DisableWatchpoint(watchpoint, notify);
> }
> }
> -
> - void DoReenable() {
> - if (process_sp && watchpoint_sp) {
> - bool was_disabled = watchpoint_sp->IsDisabledDuringEphemeralMode();
> - watchpoint_sp->TurnOffEphemeralMode();
> - const bool notify = false;
> - if (was_disabled) {
> - process_sp->DisableWatchpoint(watchpoint_sp.get(), notify);
> - } else {
> - process_sp->EnableWatchpoint(watchpoint_sp.get(), notify);
> +
> + ~WatchpointSentry() {
> + if (process && watchpoint) {
> + if (!watchpoint->IsDisabledDuringEphemeralMode()) {
> + const bool notify = false;
> + process->EnableWatchpoint(watchpoint, notify);
> }
> + watchpoint->TurnOffEphemeralMode();
> }
> }
> -
> - ~WatchpointSentry() {
> - DoReenable();
> - if (process_sp)
> - process_sp->ClearPreResumeAction(SentryPreResumeAction, this);
> - }
> -
> - static bool SentryPreResumeAction(void *sentry_void) {
> - WatchpointSentry *sentry = (WatchpointSentry *) sentry_void;
> - sentry->DoReenable();
> - return true;
> - }
>
> private:
> - ProcessSP process_sp;
> - WatchpointSP watchpoint_sp;
> - bool sentry_triggered = false;
> + Process *process;
> + Watchpoint *watchpoint;
> };
>
> StopInfoWatchpoint(Thread &thread, break_id_t watch_id,
> @@ -677,12 +659,12 @@ protected:
> GetValue()));
> if (wp_sp) {
> ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0));
> - ProcessSP process_sp = exe_ctx.GetProcessSP();
> + Process *process = exe_ctx.GetProcessPtr();
>
> // This sentry object makes sure the current watchpoint is disabled
> // while performing watchpoint actions,
> // and it is then enabled after we are finished.
> - WatchpointSentry sentry(process_sp, wp_sp);
> + WatchpointSentry sentry(process, wp_sp.get());
>
> {
> // check if this process is running on an architecture where
> @@ -690,10 +672,10 @@ protected:
> // before the associated instruction runs. if so, disable the WP,
> // single-step and then
> // re-enable the watchpoint
> - if (process_sp) {
> + if (process) {
> uint32_t num;
> bool wp_triggers_after;
> - if (process_sp->GetWatchpointSupportInfo(num, wp_triggers_after)
> + if (process->GetWatchpointSupportInfo(num, wp_triggers_after)
> .Success()) {
> if (!wp_triggers_after) {
> StopInfoSP stored_stop_info_sp = thread_sp->GetStopInfo();
> @@ -707,10 +689,10 @@ protected:
> new_plan_sp->SetIsMasterPlan(true);
> new_plan_sp->SetOkayToDiscard(false);
> new_plan_sp->SetPrivate(true);
> - process_sp->GetThreadList().SetSelectedThreadByID(
> + process->GetThreadList().SetSelectedThreadByID(
> thread_sp->GetID());
> - process_sp->ResumeSynchronous(nullptr);
> - process_sp->GetThreadList().SetSelectedThreadByID(
> + process->ResumeSynchronous(nullptr);
> + process->GetThreadList().SetSelectedThreadByID(
> thread_sp->GetID());
> thread_sp->SetStopInfo(stored_stop_info_sp);
> }
> @@ -757,8 +739,6 @@ protected:
> if (wp_sp->GetHitCount() <= wp_sp->GetIgnoreCount())
> m_should_stop = false;
>
> - Debugger &debugger = exe_ctx.GetTargetRef().GetDebugger();
> -
> if (m_should_stop && wp_sp->GetConditionText() != nullptr) {
> // We need to make sure the user sees any parse errors in their
> // condition, so we'll hook the
> @@ -798,6 +778,7 @@ protected:
> }
> }
> } else {
> + Debugger &debugger = exe_ctx.GetTargetRef().GetDebugger();
> StreamSP error_sp = debugger.GetAsyncErrorStream();
> error_sp->Printf(
> "Stopped due to an error evaluating condition of watchpoint ");
> @@ -819,20 +800,8 @@ protected:
> // If the condition says to stop, we run the callback to further decide
> // whether to stop.
> if (m_should_stop) {
> - // FIXME: For now the callbacks have to run in async mode - the
> - // first time we restart we need
> - // to get out of there. So set it here.
> - // When we figure out how to nest watchpoint hits then this will
> - // change.
> -
> - bool old_async = debugger.GetAsyncExecution();
> - debugger.SetAsyncExecution(true);
> -
> StoppointCallbackContext context(event_ptr, exe_ctx, false);
> bool stop_requested = wp_sp->InvokeCallback(&context);
> -
> - debugger.SetAsyncExecution(old_async);
> -
> // Also make sure that the callback hasn't continued the target.
> // If it did, when we'll set m_should_stop to false and get out of
> // here.
>
>
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
More information about the lldb-commits
mailing list