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

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to