Fix a cornercase in breakpoint reporting

[labath]

Summary

This resolves a similar problem as D16720 (which handled the case when we single-step onto a
breakpoint), but this one deals with involutary stops: when we stop a thread (e.g. because
another thread has hit a breakpont and we are doing a full stop), we can end up stopping it right
before it executes a breakpoint instruction. In this case, the stop reason will be empty, but we
will still step over the breakpoint when do the next resume, thereby missing a breakpoint hit.

I have observed this happening in TestConcurrentEvents, but I have no idea how to reproduce this
behavior more reliably.

[labath]

I've realized that some foreign stubs may not send the "reason" field, so I limit the scope of this change to cases where "signal" is zero as well.

[clayborg]

Looks fine. We should probably put the code that checks for a breakpoint at the thread PC into a function since it is done about 5 times in this function.

[jingham]

The only worry I have about this is

• thread A is stopped at a breakpoint, and then we stop on thread B instead, then we report a breakpoint, great!
• The user steps thread B, which we do by suspending thread B and stepping A.
• Then A stops, and we report ANOTHER hit on thread B.

Your fix looks better than what was there previously. If it is easy to check "last stop reason was this breakpoint hit, and this thread's temporary resume state is suspended, then don't report the hit, that would be more accurate.

[labath]

In D18692#389529, @clayborg wrote:

Looks fine. We should probably put the code that checks for a breakpoint at the thread PC into a function since it is done about 5 times in this function.

I'll prepare a follow-up to deduplicate the code here.

In D18692#389651, @jingham wrote:

The only worry I have about this is

• thread A is stopped at a breakpoint, and then we stop on thread B instead, then we report a breakpoint, great!
• The user steps thread B, which we do by suspending thread B and stepping A.
• Then A stops, and we report ANOTHER hit on thread B.

Your fix looks better than what was there previously. If it is easy to check "last stop reason was this breakpoint hit, and this thread's temporary resume state is suspended, then don't report the hit, that would be more accurate.

I tried to do the following:

• have two threads: A and B
• hit a breakpoint on thread A
• switch to thread B and resume it
• hit a breakpoint on thread B

After this, thread A still reports as being stopped at a breakpoint, but:

• the breakpoint is not counted as hit twice
• the code I am changing is not even executed, as we back out of this function early due to thread_sp->StopInfoIsUpToDate() being true

So, while I could easily add the check for eStateSuspended, it seems that this situation is already handled elsewhere (probably by Thread::IsStillAtLastBreakpointHit, although I'm not that familiar with this code). Which makes sense as this situation could happen even for normal breakpoint hits, not just the ones I "simulate" here.

Does that make sense?

[jingham]

Yes, that sounds right.

lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

if (tid != LLDB_INVALID_THREAD_ID)
                         }
                         else if (reason.compare("exec") == 0)
                         {
                             did_exec = true;
                             thread_sp->SetStopInfo (StopInfo::CreateStopReasonWithExec(*thread_sp));
                             handled = true;
                         }
                     }
+                    else if (!signo)
+                    {
+                        addr_t pc = thread_sp->GetRegisterContext()->GetPC();
+                        lldb::BreakpointSiteSP bp_site_sp =
+                            thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(pc);
+
+                        // If the current pc is a breakpoint site then the StopInfo should be set to Breakpoint
+                        // even though the remote stub did not set it as such. This can happen when
+                        // the thread is involuntarily interrupted (e.g. due to stops on other
+                        // threads) just as it is about to execute the breakpoint instruction.
+                        if (bp_site_sp && bp_site_sp->ValidForThisThread(thread_sp.get()))
+                        {
+                            thread_sp->SetStopInfo(
+                                StopInfo::CreateStopReasonWithBreakpointSiteID(*thread_sp, bp_site_sp->GetID()));
+                            handled = true;
+                        }
+                    }
 
                     if (!handled && signo && did_exec == false)
                     {
                         if (signo == SIGTRAP)
                         {
                             // Currently we are going to assume SIGTRAP means we are either
                             // hitting a breakpoint or hardware single stepping.
                             handled = true;