[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

> On Sep 20, 2017, at 4:16 PM, Leonard Mosescu <mosescu at google.com> wrote:
> 
> I don't quite understand the comment about signals adding indeterminacy. No signal delivery is required to test this part.  The lldb driver has a sigint handler that calls SBDebugger::DispatchInputInterrupt.  But since you aren't testing whether SIGINT actually calls DispatchInputInterrupt, you can just call it directly.  
> 
> Agreed.
> 
> I was suggesting executing a command (with SBCommandInterpreter::ExecuteCommand) for a Python command that cycles calling WasInterrupted.  Then you would make another thread in Python and have that thread wait a bit then call DispatchInputInterrupt,  If "wait a bit" bothers you then you could have the python based command you are interrupting signal the interrupting thread before going into its loop.  I don't see how this would result in indeterminacy,  And it would be testing exactly what you want to  test: does calling DispatchInputInterrupt cause a command in flight to be interrupted.
> 
> Once you have a second thread you end up with the non-determinism I hinted to (this is true regardless if you hardcode a wait, use an event or keep interrupting at a fixed rate). Now this is not a deal breaker in itself, after all if you go after testing async behavior it's part of the deal.
> 
> But in this case it gets a bit more complicated as far as I can tell: first, DispatchInputInterrupt() is only passed on the top IO Handler, if any. So DispatchInputInterrupt() through SBDebugger is not exactly the same as a real input interrupt.
> 
> Second, if we want to allow the interruption of commands that are executed through SBDebugger::HandleCommand() the command state machine is not a simple idle -> executing (->interrupted) -> idle since you get reentrancy (the command can invoke other commands, etc...). Note that in the current version, the states are only tracking in CommandInterpreter::IOHandlerInputComplete() which should not lead to reentrancy (I did manual testing for this - if anything I'd love a way to automate testing _this_ part btw)
> 
> I got pretty far in dancing around all this, but it become clear that I was not really testing the real path and I was just introducing more artificial complexity. If I'm missing anything I'd be happy to be revisit this and to be proven wrong.

It been a couple of years since I dug into this code - Greg’s mostly been maintaining it.  So I’m entirely willing to believe it’s changed in a way that makes my memory of how it works unhelpful, but I’ll have to do some reading up to see.  I’ll do that when I get a chance.

Jim

> 
> On Wed, Sep 20, 2017 at 3:51 PM, Jim Ingham via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
> jingham accepted this revision.
> jingham added a comment.
> 
> I don't quite understand the comment about signals adding indeterminacy. No signal delivery is required to test this part.  The lldb driver has a sigint handler that calls SBDebugger::DispatchInputInterrupt.  But since you aren't testing whether SIGINT actually calls DispatchInputInterrupt, you can just call it directly.  I was suggesting executing a command (with SBCommandInterpreter::ExecuteCommand) for a Python command that cycles calling WasInterrupted.  Then you would make another thread in Python and have that thread wait a bit then call DispatchInputInterrupt,  If "wait a bit" bothers you then you could have the python based command you are interrupting signal the interrupting thread before going into its loop.  I don't see how this would result in indeterminacy,  And it would be testing exactly what you want to  test: does calling DispatchInputInterrupt cause a command in flight to be interrupted.
> 
> But this change is fine.  Check it in and I'll add a test when I get a chance.
> 
> 
> https://reviews.llvm.org/D37923 <https://reviews.llvm.org/D37923>
> 
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20170920/d3c3a484/attachment.html>