[Lldb-commits] [PATCH] D147753: Move "SelectMostRelevantFrame" from Thread::WillStop

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added reviewers: JDevlieghere, mib.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

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 lldb_private::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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147753

Files:
  lldb/include/lldb/Target/StackFrameList.h
  lldb/include/lldb/Target/Thread.h
  lldb/source/Target/StackFrameList.cpp
  lldb/source/Target/Thread.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D147753.511579.patch
Type: text/x-patch
Size: 5796 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20230407/aef65955/attachment.bin>