This is an archive of the discontinued LLVM Phabricator instance.

[ThreadStateCoordinator] Remove Event classes
ClosedPublic

Authored by labath on Apr 24 2015, 6:46 AM.

Details

Summary

This is a cleanup patch for thread state coordinator. After making processing of all events
synchronous, there is no need to have a a separate class for each event. I have moved back
processing of all events back into the TSC class. No functional change.

Diff Detail

Event Timeline

labath updated this revision to Diff 24386.Apr 24 2015, 6:46 AM
labath retitled this revision from to [ThreadStateCoordinator] Remove Event classes.
labath updated this object.
labath edited the test plan for this revision. (Show Details)
labath added reviewers: chaoren, vharron.
labath added a subscriber: Unknown Object (MLST).
labath updated this revision to Diff 24464.Apr 27 2015, 3:50 AM

Sending of SIGSTOP to newly created threads got somehow lost..

(unit tests actually managed to find something :)

labath updated this revision to Diff 24467.Apr 27 2015, 5:22 AM

Remove empty method.

labath updated this revision to Diff 24538.Apr 28 2015, 4:13 AM

Fixup log messages

labath updated this revision to Diff 24626.Apr 29 2015, 6:18 AM

Rebase on top of D9227

chaoren added inline comments.May 4 2015, 12:26 PM
source/Plugins/Process/Linux/ThreadStateCoordinator.cpp
471

Inline this?

513

Combine with TSC::RequestThreadResume and inline this?

534

Inline this?

553

Inline this?

source/Plugins/Process/Linux/ThreadStateCoordinator.h
146

We don't need this for a struct.

245

Define a PendingNotificationUP? So we don't need to have std::unique_ptr<PendingNotification> everywhere.

labath added a comment.May 5 2015, 2:33 AM

I have addressed two of the issues you mentioned. As for the inlining, I have that in my sights, but I would prefer to do it in a separate change, after this code get's moved into NPL. Then I can do more function removal as NPL has a lot of similarly named functions, which are just proxies to the corresponding TSC method. And actually, now i quite like the fact that logging and locking is slightly separated from the code doing the actual work. What do you think? Can I do this as a separate change (along with a lot of other cleanups)?

source/Plugins/Process/Linux/ThreadStateCoordinator.h
146

Removed.

245

Done.

labath updated this revision to Diff 24939.May 5 2015, 2:36 AM

Address review comments

chaoren edited edge metadata.May 5 2015, 10:21 AM

Can I do this as a separate change (along with a lot of other cleanups)?

Okay, LGTM for now.

source/Plugins/Process/Linux/ThreadStateCoordinator.h
245

s/std::unique_ptr<PendingNotification>/PendingNotificationUP/

This revision was automatically updated to reflect the committed changes.