[lldb-dev] Question about breakpoint hit counts

[jingham at apple.com]

Apparently I added this 4 years ago as part of adding support for breakpoint conditions.  I must have been evaluating the condition while creating the stop info, so I needed to be able to create it in the right state if the condition failed, but I don't have a clear memory of this.  Anyway, if it were going to be used this way, you would want to make sure that the hit count didn't get incremented - since hits which fail the condition should not increment the hit count.  

We don't do things that way anymore, the stop info creation and the "should we stop" processing are done in separate phases.

Actually, on a side note, I apparently got the hit count dance wrong when I reworked the breakpoint condition handling, since the hit count actually increments when you hit a breakpoint but fail the condition.  It's arguable what you should do here (at least WE had an argument about it locally...), but I think it makes more sense not to call it a breakpoint hit when the condition fails.  After all, if you set a condition on the breakpoint, and then said "stop after 5 hits," the reasonable interpretation is that you meant 5 hits that satisfied the condition.  I'll fix this when I have a spare minute.

Anyway...  The only place the three-argument version is used (other than in code you are currently changing) is in the PosixThreads.cpp, and I think that use is wrong.  I can't think of a good reason for it to be there.

Once we're sure that switching PosixThreads over to the same way everybody else does it works correctly, or fix whatever quirk required this usage, it would be a good idea to remove it.

Jim


> On Jan 14, 2015, at 2:40 PM, Zachary Turner <zturner at google.com> wrote:
> 
> Thanks, your explanation makes perfect sense.  I'm still a little confused about the purpose of the 3 argument constructor though.  Can you come up with a concrete situation where we should use that constructor?  If we use it with should_stop=true, then the initial hit count will be 0, even though we've told it that it should stop.  Is this behavior correct?
> 
> On Wed Jan 14 2015 at 2:25:36 PM <jingham at apple.com> wrote:
> Just to be clear, the execution control machinery's handling of a thread that stops on a breakpoint does not rely on that thread's stop info to figure out how to restart the thread.  It can't do that, since the user could have put a breakpoint on the PC that the thread happened to be stopped at AFTER it stopped.  So the StopInfo's in this case can be used freely to create the illusion we want to give the user.
> 
> Jim
> 
> > On Jan 14, 2015, at 2:14 PM, jingham at apple.com wrote:
> >
> >
> > If a thread hits a thread specific breakpoint that is not for that thread, you want that thread to say it stopped for no reason, not that it stopped for a reason that isn't actually going to cause it to stop.  For instance, suppose I had a thread specific breakpoint set on thread A, and both Thread A and Thread B hit that breakpoint.  It would be confusing if the stop reason for B was that breakpoint, since the breakpoint was not for that thread. The fact that it happens to be sitting on the breakpoint is irrelevant to the user.
> >
> > And it would similarly be confusing to say that because the breakpoint was for that thread, you knew that the breakpoint was actually going to stop.  Many other things could cause it not to stop.
> >
> > The right thing to do is: if the breakpoint is for this thread, create a breakpoint stop info with no opinion about should_stop, and if not, create an empty stop info.
> >
> > Both ProcessGDBRemote.cpp and StopInfoMachException.cpp do it this way.
> >
> > For some reason, PosixThreads.cpp does the right thing if the breakpoint is for that thread - creates one with no opinion about the should_stop if it IS for the thread, but if it isn't for that thread it creates a stop info with should stop set to false.  That latter bit seems wrong to me for the reason given in the first PP.  The commit that added the should_stop = false breakpoint for a not-for-this-breakpoint thread doesn't say why it was done that way, and Matt isn't working on lldb anymore so he probably doesn't remember...
> >
> > Somebody adventurous on the Linux side might want to try getting PosixThreads.cpp to do it the way the other code does, and see if that causes any fallout.
> >
> > On the Windows side, you should do it the way ProcessGDBRemote.cpp does it.
> >
> > Jim
> >
> >
> >> On Jan 14, 2015, at 1:28 PM, Zachary Turner <zturner at google.com> wrote:
> >>
> >> When I hit a breakpoint on Windows, I'm doing something like this:
> >>
> >>        BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc));
> >>        lldb::break_id_t break_id = LLDB_INVALID_BREAK_ID;
> >>        bool should_stop = true;
> >>        if (site)
> >>        {
> >>            should_stop = site->ValidForThisThread(stop_thread.get());
> >>            break_id = site->GetID();
> >>        }
> >>
> >>        stop_info = StopInfo::CreateStopReasonWithBreakpointSiteID(*stop_thread, break_id, should_stop);
> >>        stop_thread->SetStopInfo(stop_info);
> >>
> >> When should_stop is true (which for now is basically always), this results in the breakpoint's hit count not increasing.  It seems this is because specifying a value for should_stop leads to m_should_stop_is_valid being initialized to true.  But in StopInfoBreakpoint::ShouldStopSynchronous(), we only bump the hit count if m_should_stop_is_valid is false.
> >>
> >> I can fix this bug by using
> >>
> >> StopInfo::CreateStopReasonWithBreakpointSiteID(*stop_thread, break_id);
> >>
> >> instead, and since m_stop_info_is_valid is false, and it bumps the hit count later.  But I'm not sure if this is the right thing to do, or if the behavior I'm seeing with specifying a value for should_stop on creation and the hit count not going up is a bug.
> >>
> >> Can anyone explain this?
> >> _______________________________________________
> >> lldb-dev mailing list
> >> lldb-dev at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
> >