This is an archive of the discontinued LLVM Phabricator instance.

[NativeProcessLinux] Integrate MainLoop
ClosedPublic

Authored by labath on Jul 13 2015, 8:54 AM.

Details

Summary

This commit integrates MainLoop into NativeProcessLinux. By registering a SIGCHLD handler with
the llgs main loop, we can get rid of the special monitor thread in NPL, which saves as a lot of
thread ping-pong when responding to client requests (e.g. qThreadInfo processing time has been
reduced by about 40%). It also makes the code simpler, IMHO.

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 29583.Jul 13 2015, 8:54 AM
labath retitled this revision from to [NativeProcessLinux] Integrate MainLoop.
labath updated this object.
labath added a subscriber: lldb-commits.
tberghammer accepted this revision.Jul 13 2015, 10:35 AM
tberghammer edited edge metadata.

LGTM

source/Plugins/Process/Linux/NativeProcessLinux.cpp
2729 ↗(On Diff #29583)

Please inline this function call

2743 ↗(On Diff #29583)

Please inline this function call

This revision is now accepted and ready to land.Jul 13 2015, 10:35 AM
ovyalov added inline comments.Jul 13 2015, 6:13 PM
include/lldb/Host/common/NativeProcessProtocol.h
19 ↗(On Diff #29583)

Can you use forward declaration here?

source/Plugins/Process/Linux/NativeProcessLinux.cpp
3302 ↗(On Diff #29583)

indentation.

3303 ↗(On Diff #29583)

Please consider storing errno as a local variable before logging - if log->Printf will be failing by any reason we don't want it to overwrite errno.

3310 ↗(On Diff #29583)

s/NULL/nullptr

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
102 ↗(On Diff #29583)

Since you're removing Terminate method from it's seems feasible to remove this destructor impl as well.

labath updated this revision to Diff 29660.Jul 14 2015, 3:16 AM
labath marked 5 inline comments as done.
labath edited edge metadata.
  • Address review comments
include/lldb/Host/common/NativeProcessProtocol.h
19 ↗(On Diff #29583)

The simple forward declaration class MainLoop; will not work because MainLoop is a typedef and you will get redefinition errors. We could try to do something clever here, but I am not sure it is worth it. I could also have the interface take MainLoopBase and forward declare that, but then I would have to upcast the object to MainLoopPosix in NPL, which i also don't like much.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
102 ↗(On Diff #29583)

Note that I haven't removed the base method NativeProcessProtocol::Terminate(). However, I think this is a good idea, so I will interpret this comment as a request to remove that as well. :)

ovyalov accepted this revision.Jul 14 2015, 4:40 PM
ovyalov edited edge metadata.

LGTM

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
98–99 ↗(On Diff #29660)

Thanks)

This revision was automatically updated to reflect the committed changes.