[LLDB][MIPS] Fix TestWatchpointIter failure

[nitesh.jain]

Summary

In case of MIPS, the watchpoint exception occurred before the associated instruction is executed. Whenever watchpoint is hit , we first preserve the watchpoint index before it is disable and then update the index after enabling it. Thus preserving watchpoint index.

[omjavaid]

Although this patch fixes the test case in question but in theory EphemeralMode watchpoint enable/disable cycles should be independent of step-over watchpoint enable disable cycle.

On gdb-remote type targets we only update hardware_watch_id when a watchpoint is hit so when we clear hw_watch_id in Watchpoint::SetEnabled (source/Breakpoint/Watchpoint.cpp line 238) then information about last hardware watchpoint id that was hit also gets removed.

To update hardware watchpoint id on gdb-remote targets we ll have to send/recv an extra query packet asking about hardware watchpoint id corresponding to particular watchpoint we just enabled.

One possible solution could be to preserver the hardware watchpint id like way we are preserving stop info in this case. But there no harm in even not clearing watchpoint hardware id on disable rather leave that job for the target to decide if it can update hardware id upon every enable disable or just when watchpoint is hit.

[labath]

Jim will be a better person to review this. However, this feels like a hack to me.

[jingham]

I agree with Pavel, it looks like you're using a side-effect of the Ephemeral mode to preserve something you would be better off preserving explicitly.

[nitesh.jain]

Updated diff as per suggestion.

[clayborg]

I let Jim OK this patch.

[jingham]

This looks fine. Can you add a comment explaining why this is necessary, it isn't obvious right off the bat?

If this is fixing a test case, then just add a comment and this change is fine. If the fix is test-suite neutral, then please add a test case.

[nitesh.jain]

Update diff as per suggestion

[jingham]

Great, thanks!

lldb/trunk/source/Target/StopInfo.cpp

if (thread_sp) {
           // re-enable the watchpoint
           if (process_sp) {
             uint32_t num;
             bool wp_triggers_after;
 
             if (process_sp->GetWatchpointSupportInfo(num, wp_triggers_after)
                     .Success()) {
               if (!wp_triggers_after) {
-                process_sp->DisableWatchpoint(wp_sp.get(), false);
+                // We need to preserve the watch_index before watchpoint 
+                // is disable. Since Watchpoint::SetEnabled will clear the
+                // watch index.
+                // This will fix TestWatchpointIter failure
+                Watchpoint *wp = wp_sp.get();
+                uint32_t watch_index = wp->GetHardwareIndex();
+                process_sp->DisableWatchpoint(wp, false);
                 StopInfoSP stored_stop_info_sp = thread_sp->GetStopInfo();
                 assert(stored_stop_info_sp.get() == this);
 
                 ThreadPlanSP new_plan_sp(
                     thread_sp->QueueThreadPlanForStepSingleInstruction(
                         false,  // step-over
                         false,  // abort_other_plans
                         true)); // stop_other_threads
                 new_plan_sp->SetIsMasterPlan(true);
                 new_plan_sp->SetOkayToDiscard(false);
                 new_plan_sp->SetPrivate(true);
                 process_sp->GetThreadList().SetSelectedThreadByID(
                     thread_sp->GetID());
                 process_sp->ResumeSynchronous(nullptr);
                 process_sp->GetThreadList().SetSelectedThreadByID(
                     thread_sp->GetID());
                 thread_sp->SetStopInfo(stored_stop_info_sp);
-                process_sp->EnableWatchpoint(wp_sp.get(), false);
+                process_sp->EnableWatchpoint(wp, false);
+                wp->SetHardwareIndex(watch_index);
               }
             }
           }
         }
 
         // This sentry object makes sure the current watchpoint is disabled
         // while performing watchpoint actions,
         // and it is then enabled after we are finished.