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

Repository
rL LLVM

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
466 ↗(On Diff #24538)

Inline this?

508 ↗(On Diff #24538)

Combine with TSC::RequestThreadResume and inline this?

529 ↗(On Diff #24538)

Inline this?

548 ↗(On Diff #24538)

Inline this?

source/Plugins/Process/Linux/ThreadStateCoordinator.h
146 ↗(On Diff #24538)

We don't need this for a struct.

242 ↗(On Diff #24538)

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 ↗(On Diff #24538)

Removed.

242 ↗(On Diff #24538)

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
242 ↗(On Diff #24939)

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

This revision was automatically updated to reflect the committed changes.