This is an archive of the discontinued LLVM Phabricator instance.

NativeProcessLinux: Merge operation and monitor threads
ClosedPublic

Authored by labath on Apr 17 2015, 8:19 AM.

Details

Summary

This commit moves the functionality of the operation thread into the new monitor thread. This is
required to avoid a kernel race between the two threads and I believe it actually makes the code
cleaner.

Diff Detail

Event Timeline

labath updated this revision to Diff 23935.Apr 17 2015, 8:19 AM
labath retitled this revision from to NativeProcessLinux: Merge operation and monitor threads.
labath updated this object.
labath edited the test plan for this revision. (Show Details)
labath added reviewers: ovyalov, tberghammer, vharron.
labath added a subscriber: Unknown Object (MLST).
tberghammer edited edge metadata.Apr 17 2015, 8:37 AM

I think it made the code much cleaner

source/Plugins/Process/Linux/NativeProcessLinux.cpp
1215

Please move it to a static const char to the top of the file with a name indicating its meaning and use the same variable in the switch

1423

Can we get rid of the goto?

labath updated this revision to Diff 23936.Apr 17 2015, 9:05 AM
labath edited edge metadata.

responding to review comments

Thanks. Please take another look.

source/Plugins/Process/Linux/NativeProcessLinux.cpp
1215

I've made it a private member of the Monitor class.

1423

Yep.

ovyalov edited edge metadata.Apr 17 2015, 10:41 AM

Please see my comments.

Overall looks great!

source/Plugins/Process/Linux/NativeProcessLinux.cpp
1077

Could you return Error instead of bool?

1080

Please combine definition of status variable with its initialization

1122

Nit: please move up m_operation_mutex so mutex is initialized before the data it protects.

1197–1218

is it waiting for initial command to complete? If so - please add the comment.

1215

s/1/sizeof(operation_command)

labath updated this revision to Diff 24001.Apr 20 2015, 12:50 AM
labath edited edge metadata.

Respond to review comments.

source/Plugins/Process/Linux/NativeProcessLinux.cpp
1077

done.

1080

done.

1122

done.

1197–1218

Affirmative. Added comment.

1215

done.

tberghammer accepted this revision.Apr 20 2015, 3:41 AM
tberghammer edited edge metadata.

Looks good

This revision is now accepted and ready to land.Apr 20 2015, 3:41 AM
This revision was automatically updated to reflect the committed changes.