[lldb] Move "SelectMostRelevantFrame" from Thread::WillStop
authorJim Ingham <jingham@apple.com>
Fri, 7 Apr 2023 19:17:57 +0000 (12:17 -0700)
committerJonas Devlieghere <jonas@devlieghere.com>
Fri, 7 Apr 2023 19:21:00 +0000 (12:21 -0700)
commit730c8e160c9dead94ba534d5ad7cc8e47ce892db
tree6359ee1b2e9c71f6ae05bb96782de40adca3b383
parent6ff177d928d809dec637d7d5e32aad416b8d7778
[lldb] Move "SelectMostRelevantFrame" from Thread::WillStop

SelectMostRelevantFrame triggers the StackFrameRecognizer construction,
which can run arbitrary Python code, call expressions etc. WillStop gets
called on every private stop while the recognizers are a user-facing
feature, so first off doing this work on every stop is inefficient. But
more importantly, you can get in to situations where the recognizer
causes an expression to get run, then when we fetch the stop event at
the end of the expression evaluation, we call WillStop again on the
expression handling thread, which will do the same StackFrameRecognizer
work again. If anyone is locking along that path, you will end up with a
deadlock between the two threads.

The example that brought this to my attention was the
objc_exception_throw recognizer which can cause the objc runtime
introspection functions to get run, and those take a lock in
AppleObjCRuntimeV2::DynamicClassInfoExtractor::UpdateISAToDescriptorMap
along this path, so the second thread servicing the expression deadlocks
against the first thread waiting for the expression to complete.

It makes more sense to have the frame recognizers run on demand, either
when someone asks for the variables for the frame, or when someone does
GetSelectedFrame. The former already worked that way, the only reason
this was being done in WillStop was because the StackFrameRecognizers
can change the SelectedFrame, so you needed to run them before the
anyone requested the SelectedFrame.

This patch moves SelectMostRelevantFrame to StackFrameList, and runs it
when GetSelectedFrame is called for the first time on a given stop. If
you call SetSelectedFrame before GetSelectedFrame, then you should NOT
run the recognizer & change the frame out from under you. This patch
also makes that work. There were already tests for this behavior, and
for the feature that caused the hang, but the hang is racy, and it
doesn't trigger all the time, so I don't have a way to test that
explicitly.

One more detail: it's actually pretty easy to end up calling
GetSelectedFrame, for instance if you ask for the best ExecutionContext
from an ExecutionContextRef it will fill the StackFrame with the result
of GetSelectedFrame and that would still have the same problems if this
happens on the Private State Thread. So this patch also short-circuits
SelectMostRelevantFrame if run on the that thread. I can't think of any
reason the computations that go on on the Private State Thread would
actually want the SelectedFrame - that's a user-facing concept, so
avoiding that complication is the best way to go.

rdar://107643231

Differential revision: https://reviews.llvm.org/D147753
lldb/include/lldb/Target/StackFrameList.h
lldb/include/lldb/Target/Thread.h
lldb/source/Target/StackFrameList.cpp
lldb/source/Target/Thread.cpp