Page MenuHomePhabricator

[lldb] [MainLoop] Support multiple callbacks per signal
ClosedPublic

Authored by mgorny on Apr 13 2021, 4:00 PM.

Details

Summary

Support registering multiple callbacks for a single signal. This is
necessary to support multiple co-existing native process instances, with
separate SIGCHLD handlers.

The system signal handler is registered on first request, additional
callback are added on subsequent requests. The system signal handler
is removed when last callback is unregistered.

Diff Detail

Event Timeline

mgorny requested review of this revision.Apr 13 2021, 4:00 PM
mgorny created this revision.

I've been thinking about whether this should be done here, or via a separate multiplexer entity. It's not clear to me which one is cleaner so I suppose we can go with what you have done here.

That said, I'm not too happy about this callback_id business. IIUC, it's only there to enable removing a specific callback (because you can't have a set of std::functions). But there are other ways to achieve that, and they don't leak this detail to the users. One option would be to hold the callbacks in a std::list, and use the (stable) iterator as the ID....

It could also use a test... something like, register a callback, fire a signal and check it's called, registers a second callback and check that both are called, unregister the first one and check that only the second one is called, ...

I've been thinking about whether this should be done here, or via a separate multiplexer entity. It's not clear to me which one is cleaner so I suppose we can go with what you have done here.

That said, I'm not too happy about this callback_id business. IIUC, it's only there to enable removing a specific callback (because you can't have a set of std::functions). But there are other ways to achieve that, and they don't leak this detail to the users. One option would be to hold the callbacks in a std::list, and use the (stable) iterator as the ID....

It could also use a test... something like, register a callback, fire a signal and check it's called, registers a second callback and check that both are called, unregister the first one and check that only the second one is called, ...

Hmm, my first though was to use a static int that increases with every call. But the std::list idea is interesting, so I'll explore that.

I've been thinking about whether this should be done here, or via a separate multiplexer entity. It's not clear to me which one is cleaner so I suppose we can go with what you have done here.

That said, I'm not too happy about this callback_id business. IIUC, it's only there to enable removing a specific callback (because you can't have a set of std::functions). But there are other ways to achieve that, and they don't leak this detail to the users. One option would be to hold the callbacks in a std::list, and use the (stable) iterator as the ID....

It could also use a test... something like, register a callback, fire a signal and check it's called, registers a second callback and check that both are called, unregister the first one and check that only the second one is called, ...

Hmm, my first though was to use a static int that increases with every call. But the std::list idea is interesting, so I'll explore that.

That would work too, though it should definitely be a member variable instead of a global (thread safety). The only difference should be in the memory footprint. I suspect the list solution will be slightly smaller, unless you use DenseMap. OTOH, a DenseMap with only a few items tends to waste space...

mgorny updated this revision to Diff 337539.Apr 14 2021, 1:42 PM
mgorny edited the summary of this revision. (Show Details)

Store callbacks in std::list, and use the list iterator to identify the callback to remove.

mgorny updated this revision to Diff 337562.Apr 14 2021, 2:29 PM

Fixed storing incorrect iterator (to temporary info).

Looks good, just add the test case.

lldb/source/Host/common/MainLoop.cpp
413–415

llvm::SmallVector<Callback, 4> callbacks_to_run(it->second.callbacks.begin(), it->second.callbacks.end());

mgorny updated this revision to Diff 338170.Apr 16 2021, 11:09 AM
mgorny marked an inline comment as done.

Fixed the iterator, formatting and added a test for adding, running and removing multiple handlers.

labath accepted this revision.Apr 21 2021, 1:12 AM
This revision is now accepted and ready to land.Apr 21 2021, 1:12 AM
This revision was landed with ongoing or failed builds.Apr 21 2021, 3:18 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2021, 3:18 AM