This is an archive of the discontinued LLVM Phabricator instance.

[NativeProcessLinux] Remove the stop callback
ClosedPublic

Authored by labath on May 6 2015, 8:42 AM.

Details

Summary

The stop callback is a remnant of the ThreadStateCoordinator. We don't need it now that TSC is
gone, as we know exactly which function to call when threads stop. This also removes some
stop-related functions, which were just forwarding calls to one another.

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 25051.May 6 2015, 8:42 AM
labath retitled this revision from to [NativeProcessLinux] Remove the stop callback.
labath updated this object.
labath edited the test plan for this revision. (Show Details)
labath added reviewers: chaoren, ovyalov.
labath added a subscriber: Unknown Object (MLST).
ovyalov edited edge metadata.May 6 2015, 10:35 AM

Please see my comments.

source/Plugins/Process/Linux/NativeProcessLinux.cpp
4540 ↗(On Diff #25051)

Please add a new ThreadContext constructor that takes ThreadState parameter.

4547 ↗(On Diff #25051)

If RequestThreadStop fails is it necessary to call m_pending_notification_up->wait_for_stop_tids.insert(tid); ?

4548 ↗(On Diff #25051)

Do we need to stop main thread of lldb-server here or RequestThreadStop(tid,.. ) ?

labath added inline comments.May 7 2015, 6:45 AM
source/Plugins/Process/Linux/NativeProcessLinux.cpp
4540 ↗(On Diff #25051)

Done.

4547 ↗(On Diff #25051)

I would leave this as-is, for now at least, as now I am trying to do cleanups that don't change functionality.

But let's think what are the possible reasons the tgkill may fail:
a) The thread has exited in the meantime.
b) The process has exited.
c) The process has done execve().
d) ???

If everything works as it should, a)-c) should not actually happen, since the thread should stop with PTRACE_EVENT_EXIT before this happens. At this point the thread still exists, and we can send signals to it. If we miss this event for whatever reason, and the thread is really dead, then it could happen that tgkill fails. However, we should still get WIFEXITED event, at which point we will delete the thread and fire the notification (and it wont matter whether we set the flag or not). If we don't get that event also then we will probably hang forever waiting for the thread the stop, but if we end up here, then we are probably in a deep mess anyway.

What do you think?

4548 ↗(On Diff #25051)

Good catch. It's tid of course.

labath updated this revision to Diff 25177.May 7 2015, 6:50 AM
labath edited edge metadata.

Address review comments

ovyalov accepted this revision.May 7 2015, 8:22 AM
ovyalov edited edge metadata.

Looks good.

source/Plugins/Process/Linux/NativeProcessLinux.cpp
4547 ↗(On Diff #25051)

I was thinking about possible EINTR error that tgkill may raise but it seems "man tgkill" doesn't mention such error as a possible outcome.
Let's leave this flow as-is - since there's logging within RequestThreadStop we'll be able to catch errors if something goes wrong.

This revision is now accepted and ready to land.May 7 2015, 8:22 AM
This revision was automatically updated to reflect the committed changes.