[Lldb-commits] [PATCH] D71440: Extending step-over range past calls was causing deadlocks, fix that.

Mon Dec 16 12:08:40 PST 2019

jingham added a comment.

In D71440#1783197 <https://reviews.llvm.org/D71440#1783197>, @labath wrote:

> This looks like a tricky bug. Thanks for tracking it down.
>
> I have two questions though: :D
>
> - could you use c++11 locking primitives in the test? pthreads do not work on windows

I copied the test from somewhere else, so I'll have to change the original too.  Let me give this a go.

> - what exactly will be the effect of this? Will we start all threads immediately, or will we do the usual dance of trying to run one thread for a short period of time, and then resume all of them if that times out? If it's the latter, I'm wondering if we really need this call detection logic -- we could theoretically just use the "run all" method all the time (except if explicitly disabled), with the knowledge that all reasonable call-less sequences will finish before the "run single thread" timeout expires. And it would automatically also handle weird call-less sequences with spinlocks or what not...

Currently, the actually stepping plans don't do "try a bit on one thread, then resume all".  They are conservative, and will let all threads run anytime they run "arbitrary" code.  It's only running expressions that does the one and then many dance.

The original idea was that the "Process::RunThreadPlan" would be used for all the thread plans that might stall if run on only one thread.  But before it got wired up for that it became dedicated to expression evaluation.  It's more important to try to stay on one thread for expressions, since the user stopped the process somehow and has a reasonable expectation that it will stay stopped.  But as a side effect, I never got around to wiring that dance up to the regular stepping.

However, since I've been playing around with this trick for expression evaluation I'm a little less sure I want to extend it to regular operation.  OTOH, that WOULD fix problems with stepping over call-less spinlocks.  More importantly - since I've never had any reports of that theoretical problem being an actual problem - we would get better at keeping stepping operations on one thread.

OTOH, interrupting a process is not cost-free.  We have to send a signal to interrupt it and that can cause EINTR's in places people weren't expecting.  This has caused problems in the past, where the interrupt from running an expression will cause a read call to raise an unhandled EINTR.  Of course you should always check for EINTR etc so these probably are actually bugs in code, and you'd think people would thank me for surfacing the bug.  But when I point that out it doesn't seem to mollify the users who have reported the problem.

This is a common enough "mis-pattern" that I am hesitant to add a stepping method that would really spam the inferior with interrupts.  I think our current method is the best compromise between trying to keep focus on the active thread, and changing normal flow of execution as little as possible.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71440/new/

https://reviews.llvm.org/D71440