Page MenuHomePhabricator

stop-hooks don't fire on "step-out"
ClosedPublic

Authored by jingham on Aug 14 2019, 12:37 PM.

Details

Summary

Step hooks were not firing when you did a "step out".

This was because, when a breakpoint's PerformAction observes that the breakpoint has completed a plan, it resets the thread StopInfo so that the completed plan will be reported as the stop reason, not the trap that is the private stop reason. But for no good reason that I can remember, it doesn't immediately reset the StopInfo to the completed plan one, but lets that happen the next time somebody gets the stop reason. That didn't happen by the time stop-hooks checked if the thread had a stop reason.

We could make the stop-hooks force the StopInfo to get recomputed, but that might happen somewhere else as well. It's better to just do it right away.

I also added a test for this case.

Diff Detail

Repository
rL LLVM

Event Timeline

jingham created this revision.Aug 14 2019, 12:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2019, 12:37 PM
davide added a subscriber: davide.Aug 14 2019, 1:14 PM
davide added inline comments.
packages/Python/lldbsuite/test/functionalities/stop-hooks/Makefile
4 ↗(On Diff #215201)

Forgive my ignorance, but why you need this?

jingham marked an inline comment as done.Aug 14 2019, 2:22 PM
jingham added inline comments.
packages/Python/lldbsuite/test/functionalities/stop-hooks/Makefile
4 ↗(On Diff #215201)

That's how the dotest tests know what to build.

davide added inline comments.Aug 14 2019, 2:31 PM
packages/Python/lldbsuite/test/functionalities/stop-hooks/Makefile
4 ↗(On Diff #215201)

I understand C_SOURCES, but do you need to specify also the standard? That seems redundant, but I might be missing something [mainly asking for my education].

clayborg added inline comments.Aug 14 2019, 3:00 PM
source/Target/StopInfo.cpp
550 ↗(On Diff #215201)

Can you clarify what is going on here? Does this force a recalculation of the stop info? This looks really goofy from a code perspective.

jingham marked an inline comment as done.Aug 14 2019, 3:11 PM

Do you think I should put more verbiage in the comment, or would this have been clear if you were actually working on the code?

source/Target/StopInfo.cpp
550 ↗(On Diff #215201)

This is replacing the raw stop info (a StopInfoBreakpoint) with the public - cooked - stop info (StopInfoThreadPlan).

I think the handling of "public and private" stop info has gotten a bit muddled up. I swear when I first did this there were distinct Public and Private stop infos. But (I think when the StopInfo's acquired the Stop Actions many years back) we went to having just one stop info, and when you wanted to produce a different StopInfo from the one that you got from the Process plugin you overwrite it.

I think it would be worth playing around with keeping these two separate. It would make this sort of code much clearer, since then you wouldn't muck with the truth you got from the Process plugin, but just layer the public facing Info over top of it. But that's a much bigger change.

clayborg added inline comments.Aug 14 2019, 3:50 PM
source/Target/StopInfo.cpp
550 ↗(On Diff #215201)

ok, it does look quite goofy from a API usage scenario. Seems like you are getting and then setting to the same value. If we had an API that returned an integer, it would be like:

foo->SetInt(foo->GetInt());

Can we just do this in ResetStopInfo()? Or rename it to "UpdateStopInfo" or something clearer? I would like to see this:

thread_sp->ResetStopInfo();
thread_sp->SetStopInfo(thread_sp->GetStopInfo());

become:

thread_sp->UpdateStopInfo();
jingham updated this revision to Diff 215280.Aug 14 2019, 4:27 PM

Does this seem clearer? We use the language of Private and Public StopInfo's in a bunch of places, even though at present that's more about "How" you get them than actual separate entities. But still, this seems like it follows the language we are using elsewhere.

clayborg accepted this revision.Aug 15 2019, 7:09 AM

Better. Might be good to put a comment inside CalculatePublicStopInfo() regarding why the "SetStopInfo(GetStopInfo());" is needed. Seems like you could just remove the code from the sight of it. I will leave that up to you though.

This revision is now accepted and ready to land.Aug 15 2019, 7:09 AM

Better. Might be good to put a comment inside CalculatePublicStopInfo() regarding why the "SetStopInfo(GetStopInfo());" is needed. Seems like you could just remove the code from the sight of it. I will leave that up to you though.

The thing you have to understand for this to make sense is that what you get from GetStopInfo depends on when you ask the question. If you have just stopped, but haven't gone through the ShouldStop mechanism, then there are no completed plans, and you get the raw stop info. But if you ask again after we've done "ShouldStop", then we're in a position to attribute the stop to the plan that was managing it. That's why I said explicitly you had to do CalculatePublicStopInfo after ShouldStop.

I can refine the comment a bit to make this clearer.

I think this is going to be a little opaque so long as we aren't making a real distinction between the private (what I used to call the actual stop info - there's even a few comments that still reference this...) and the public stop info.

There's the added complication that sometimes you need to rewind to an old stop reason. For instance, you finish a step out, then run a function. Now the actual stop info is the breakpoint stop at the entry point that caught the end of the expression evaluation. The public stop info is "Completed Function Thread Plan". But showing that would be confusing, users don't care that a function was run, particularly if it was one they didn't directly cause. The stop info you actually want to show users is still "Completed step out plan".

These complexities are overlaid on one stop info, and a bunch of stop_id variables. We also rely on the assumption that you can always re-fetch the private stop info with the plugin-implemented CalculateStopInfo call. But you have to be careful, because though you can rewind to an old Public StopInfo, you can't get back to the actual StopInfo that caused it - since the process has moved on. This requires other bits of fancy footwork.

This all needs to be teased apart and better represented. But that's a task for another day.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2019, 2:37 PM