[Lldb-commits] [PATCH] D86652: [lldb] Fix ProcessEventData::DoOnRemoval to support multiple listeners

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This still seems to me a pretty fragile way to program lldb.  For instance, if the controlling thread gets the stop event first, and decides it wants to continue, it will set the process running again and by the time the memory reading thread gets the event, the process will have already proceeded and the memory read will fail.  But it it happens the other way around, it will succeed.  That will make for subtle bugs in your implementation.

Having a bunch of event listeners hanging out doing ancillary stop tasks also makes it hard to do cancellation.  For instance, if the user presses "step" again while you are still doing the work to present the current stop, you want to discard all the work you were doing for the first stop and just step again.  People like to drive from an interesting place to another interesting place by whaling on the "next" button and want that to be very responsive and don't care if all the data isn't updated in getting there.  As long as the UI knows what work it cancelled and doesn't mark data as updated if it wasn't, this does seem to better model how people told us they want GUI debuggers to behave.

You could also implement this sort of thing with stop hooks that get run before any of the other actions are taken by then controlling thread (or by implementing this yourself.)  There isn't currently a way to write stop hooks in Python but it would be simple to add that.  The stop hooks don't solve the cancellation problem, however.

Anyway, "I wouldn't do it that way" isn't a good reason not to make a thing possible.  If you feel like you can have success with this added ability, I can't see how it would do harm.

You still need to add some tests for this new behavior.  We have been telling people not to do it this way pretty much forever, and so there are no tests for multiple process listeners.  It would be good to write a not entirely trivial test that tries to do a little simultaneous work in your different Listeners, and deal with DoOnRemoval restarting the target, as well as one of the Listeners doing so, to ensure we don't fall over for other reasons when we actually try to use the feature.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86652/new/

https://reviews.llvm.org/D86652