Page MenuHomePhabricator

Handle the case where a thread exits while we were running a function on it
AcceptedPublic

Authored by jingham on May 20 2020, 6:10 PM.

Details

Reviewers
aprantl
Summary

It is possible that the thread we are running a function on could exit while we are waiting for the function call to return.

We sort of handled that case before, but it really only worked by accident because the ThreadPlan still had a pointer to the Thread, and it hadn't actually gone away when we touched it after stopping and finding that it had exited. Now that ThreadPlans re-look up the thread after each stop, we were handing out a null Thread pointer and crashing.

I moved the checking for vanished threads to the helper routine that handles the stop event, added an expression result of eExpressionThreadVanished and handle it properly in RunThreadPlan. I also added a test using a function that just called pthread_exit. This crashed before these changes, and works correctly after.

Diff Detail

Event Timeline

jingham created this revision.May 20 2020, 6:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2020, 6:10 PM
jingham updated this revision to Diff 265608.May 21 2020, 2:37 PM

clang-format

I skimmed it, I guess the one question that comes to mind is how this will behave if we an operating system thread provider plugin active, where it may not actually list all of the threads that exist -- where we try to be tolerant of threads disappearing and then re-appearing later -- and the thread running an inferior function call got scheduled out while the function call was still happening. Pretty unlikely scenario, but I thought I'd throw it out there.

lldb/source/Expression/LLVMUserExpression.cpp
235

I think you meant to include the tid in the Printf. Did this produce a warning? We should have decorated the Printf() method in this class with the thing that tells clang this is a printf functoin.

This looks reasonable based on my limited knowledge.

lldb/source/Target/Process.cpp
4671

Is it useful to both log the message and have an almost similarly-worded diagnostic produced in LLVMUserExpression.cpp?

("Yes" is an acceptable answer)

5534

If you remove the default case and initialize the result_name to "
"<unknown>" at the top, you get a free compiler warning every time when someone adds a new enumerator.

jingham marked an inline comment as done.May 21 2020, 3:36 PM
jingham added inline comments.
lldb/source/Target/Process.cpp
4671

The diagnostic will go the the user, and end up in the Error in the expression result ValueObject. So that part we have to do. Putting it in the log will make the message show up where it happens in the flow of events we're logging, which is very handy. So IMO it is worthwhile doing the latter as well.

jingham updated this revision to Diff 265632.May 21 2020, 3:44 PM

Address review comments

jingham marked 2 inline comments as done.May 21 2020, 3:45 PM
aprantl accepted this revision.May 22 2020, 2:36 PM
This revision is now accepted and ready to land.May 22 2020, 2:36 PM