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

This revision was automatically updated to reflect the committed changes. Closed by commit rL315037: Implement interactive command interruption (authored by lemo). Changed prior to commit: https://reviews.llvm.org/D37923?vs=117900=117936#toc Repository: rL LLVM

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

amccarth accepted this revision. amccarth added a comment. LGTM https://reviews.llvm.org/D37923 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. That looks fine. https://reviews.llvm.org/D37923 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

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

lemo updated this revision to Diff 117900. lemo edited the summary of this revision. lemo added a comment. Updating the original changes to handle reentrant calls to CommandInterpreter::IOHandlerInputComplete(). - This fixes bug #34758 - Also enhanced the existing test case for "command source"

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL313904: [LLDB] Implement interactive command interruption (authored by amccarth). Changed prior to commit: https://reviews.llvm.org/D37923?vs=116074=116247#toc Repository: rL LLVM

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

> On Sep 20, 2017, at 4:16 PM, Leonard Mosescu 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.

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

Yes, using line endings as split points is somewhat arbitrary, anything that's a reasonable compromise between interruption responsiveness and low interrupt polling overhead would do. I feel that the lines are somewhat nicer since arbitrary splitting may lead to confusion and/or formatting

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

amccarth accepted this revision. amccarth added a comment. LGTM. But just a thought: Is it worth doing all the work to scan for line endings for the interruption points? What if, instead, it just printed the next _n_ characters on each iteration until the entire buffer has been printed.

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

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

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

zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. seems fine to me, I agree with the point about non-determinism, due to the nature of signals and the fact this is only intended to provide a best-effort interruption.

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

lemo updated this revision to Diff 116074. lemo added a comment. 1. Added SB APIs (SBCommandInterpreter::WasInterrupted()). This allows python commands to query for interrupt requests. 2. I talked offline with Zach and decided that the line_iterator would require more tinkering to get it to

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

> On Sep 20, 2017, at 11:25 AM, Zachary Turner wrote: > > > > On Wed, Sep 20, 2017 at 11:14 AM Jim Ingham > wrote: > > The amount of test coverage lldb has at present has much more to do with the > very aggressive schedules

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

I write lots of tests. I don’t really think that the SB API’s are really a barrier to writing tests. The lldbinline tests are trivial to write and can even be made just as a transcription of an lldb command line session, so the barrier to entry is trivial. That is surely the easiest way to

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

On Wed, Sep 20, 2017 at 10:46 AM Jim Ingham wrote: > Directly WRT testing. I’m not against ALSO adding gtests when you add > some functionality. But when your change is actually adding behaviors to > lldb, one of the things you need to ask yourself is if this is >

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

Directly WRT testing. I’m not against ALSO adding gtests when you add some functionality. But when your change is actually adding behaviors to lldb, one of the things you need to ask yourself is if this is functionality that extension developers for lldb will benefit from. If it is, then

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

On Wed, Sep 20, 2017 at 10:33 AM Jim Ingham wrote: > One of the fundamental goals of lldb is that it be an extensible > debugger. The extension mechanism for command line lldb all runs through > the SB API through either Python or C++ (though most folks choose to use >

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

One of the fundamental goals of lldb is that it be an extensible debugger. The extension mechanism for command line lldb all runs through the SB API through either Python or C++ (though most folks choose to use Python). We know first hand that many people take advantage this mechanism to add

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

On Tue, Sep 19, 2017 at 11:51 AM Zachary Turner wrote: > On Tue, Sep 19, 2017 at 11:40 AM Jim Ingham wrote: > >> I must be missing something. >> >> DisaptchInputInterrupt knows how to interrupt a running process, and >> because of Enrico's Python

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

Here's a short description of the coding rules and the general idea of how to add API's: http://lldb.llvm.org/SB-api-coding-rules.html If you come across something you wished would have been in this document while you are implementing this, please add it. It's sometimes hard to see some

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

Any pointers on the steps required to make changes to the SB APIs? Is it documented anywhere? Thanks! On Tue, Sep 19, 2017 at 11:58 AM, Jim Ingham wrote: > > > On Sep 19, 2017, at 11:25 AM, Leonard Mosescu > wrote: > > > > These are all great suggestions,

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

Thanks for the context. On Tue, Sep 19, 2017 at 11:58 AM, Jim Ingham wrote: > > > On Sep 19, 2017, at 11:25 AM, Leonard Mosescu > wrote: > > > > These are all great suggestions, thanks everyone! > > > > > We should have a test. The test would need to use

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

> On Sep 19, 2017, at 11:25 AM, Leonard Mosescu wrote: > > These are all great suggestions, thanks everyone! > > > We should have a test. The test would need to use pexpect or something > > similar. If anyone has any pointer on how to make a test for this, please > >

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

On Tue, Sep 19, 2017 at 11:40 AM Jim Ingham wrote: > I must be missing something. > > DisaptchInputInterrupt knows how to interrupt a running process, and > because of Enrico's Python interruption stuff it will interrupt script > execution at some point but it wasn't very

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

That seems fine to me. Jim > On Sep 19, 2017, at 11:39 AM, Leonard Mosescu wrote: > > So, how about I look into exposing WasInterrupted() through SB APIs in a > follow up change? > > On Tue, Sep 19, 2017 at 11:36 AM, Jim Ingham wrote: > Xcode does, I

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

I must be missing something. DisaptchInputInterrupt knows how to interrupt a running process, and because of Enrico's Python interruption stuff it will interrupt script execution at some point but it wasn't very reliable last time I tried it. But the whole point of this patch is that in

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

So, how about I look into exposing WasInterrupted() through SB APIs in a follow up change? On Tue, Sep 19, 2017 at 11:36 AM, Jim Ingham wrote: > Xcode does, I don't know about other UI's. > > Jim > > > On Sep 19, 2017, at 11:35 AM, Leonard Mosescu >

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

On Tue, Sep 19, 2017 at 11:34 AM Jim Ingham wrote: > > > On Sep 19, 2017, at 11:30 AM, Zachary Turner wrote: > > > > > > > > On Tue, Sep 19, 2017 at 11:27 AM Jim Ingham wrote: > > We agreed to forwards compatibility because people write

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

Xcode does, I don't know about other UI's. Jim > On Sep 19, 2017, at 11:35 AM, Leonard Mosescu wrote: > > I agree Jim. I'd like like to build thing incrementally - checking in the > current change as is does not preclude adding the SB APIs while it does > provide the

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

IIRC Enrico put in something where we would tell Python to interrupt at points where Python checks for interruptibility, but that is pretty herky-jerky. It would be much better to have the commands control this. Jim > On Sep 19, 2017, at 11:34 AM, Jim Ingham wrote: > >

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

I agree Jim. I'd like like to build thing incrementally - checking in the current change as is does not preclude adding the SB APIs while it does provide the foundation. I think that going after the scenarios you mention is a significant increase in scope. Are people even hooking up

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

> On Sep 19, 2017, at 11:30 AM, Zachary Turner wrote: > > > > On Tue, Sep 19, 2017 at 11:27 AM Jim Ingham wrote: > We agreed to forwards compatibility because people write big scripts that use > the SB API, implement GUI's on top of them (more than

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

On Tue, Sep 19, 2017 at 11:27 AM Jim Ingham wrote: > We agreed to forwards compatibility because people write big scripts that > use the SB API, implement GUI's on top of them (more than just Xcode) etc. > So we try not to jerk those folks around. That adds a little more >

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

Right, I can fix that. Give me a few minutes though. On Tue, Sep 19, 2017 at 11:28 AM Leonard Mosescu wrote: > This looks beautiful indeed. The problem is that it doesn't quite work > with the current MemoryBuffer and the line_iterator : for one thing there's > no way to

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

This looks beautiful indeed. The problem is that it doesn't quite work with the current MemoryBuffer and the line_iterator : for one thing there's no way to construct a MemoryBuffer from a StringRef, or to use the line_iterator directly with a StringRef. On Tue, Sep 19, 2017 at 10:39 AM, Zachary

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

We agreed to forwards compatibility because people write big scripts that use the SB API, implement GUI's on top of them (more than just Xcode) etc. So we try not to jerk those folks around. That adds a little more responsibility on our part to think carefully about what we add, but the

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

These are all great suggestions, thanks everyone! > We should have a test. The test would need to use pexpect or something similar. If anyone has any pointer on how to make a test for this, please chime in. I was thinking just a pexpect based test. I love tests but what exactly do we want to test

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

I disagree. The things we care about are (a) that this works in C++ implemented commands and (b) that it works in Python commands. This doesn't seem to test either of those things. Also, I find writing tests for new functionality to be a great way to inform you about what you need to add to

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

Also, it avoids polluting the SB interface with another function that probably nobody is ever going to use outside of testing. Adding to the SB API should take an act of God, given that once it gets added it has to stay until the end of time. On Tue, Sep 19, 2017 at 11:15 AM Zachary Turner

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

That would work, but I think it's adding many more pieces to the test. Now there's threads, a Python layer, and multiprocess dotest infrastructure in the equation. Each providing an additional source of flakiness and instability. If all it is is a pass-through, then just calling the function it

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

I'd really prefer to do this as/along with an SB API test since we also need commands made through the SB API to be interruptible, and we should test that that also works. So this kills two birds with one stone. In general, when developing any high-level features in lldb one of the questions

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

Python based commands will need to be able to call WasInterrupted if they are going to do their jobs. So it would make sense to add an SBCommandInterpreter::WasInterrupted API and make sure that works. And you can already send the interrupt with DispatchInputInterrupt. So it should be pretty

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

zturner added a comment. Give me a few more hours, if there's a way to make this work with `line_iterator` I'd really prefer that since I think it improves readability. Can you confirm that if you were able to write: auto begin = line_iterator(str, /* skip_empty_lines =*/ false); auto end

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

clayborg added a comment. We should have a test. The test would need to use pexpect or something similar. If anyone has any pointer on how to make a test for this, please chime in. I was thinking just a pexpect based test. https://reviews.llvm.org/D37923

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

lemo added a comment. Ping? Are there any more open questions or unresolved comments or is this good to be checked in? Thanks! https://reviews.llvm.org/D37923 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

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

Hi Greg, are you referring to the manual line splitting vs. using StringRef::split()? This is how it's documented: /// Split into two substrings around the first occurrence of a separator /// character. /// /// If \p Separator is in the string, then the result is a pair (LHS, RHS) /// such that

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

Looking at line_iterator, it seems to be designed to work with MemoryBuffer, which in turn seems specialized for dealing with file content (so while it may be possible to force init a MemoryBuffer from a StringRef it seems a bit awkward to me). Also, line_iterator has extra stuff which we don't

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

clayborg added a comment. ok. Then back to the "can we use StringRef"? We might be able to check if the second.data() is NULL? It might be NULL if the string doesn't end with a newline. It it does, it might be empty but contain a pointer to the '\0' byte? https://reviews.llvm.org/D37923

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

jingham added a comment. And in general, interruption should be as responsive as you can make it. When I tell lldb to "Stop right now" it really should do as close as possible to "stop right now". Not, :-"excuse me I have 10 more pages of output to dump". It's not uncommon to dump some

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

On Mon, Sep 18, 2017 at 3:13 PM Leonard Mosescu wrote: > It's a good question - here's a two part answer: > > 1. The actual printing of the output is the longest blocking in many cases > (as mentioned in the description: the actual data gathering for "target > module dump

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

jingham added a comment. In the original discussion of this feature (it was on the mailing list), it sounded like the specific cast that motivated Leonard in adding this feature was when there's a command that's in the process of generating tons of output, and you want to interrupt the tons

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

It's a good question - here's a two part answer: 1. The actual printing of the output is the longest blocking in many cases (as mentioned in the description: the actual data gathering for "target module dump symtab" can take 1-2sec, but printing it can take 20min. For quick experiment, try dis -p

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

zturner added inline comments. Comment at: source/Interpreter/CommandInterpreter.cpp:2708-2710 +const char *data = str.data(); +size_t size = str.size(); +while (size > 0 && !WasInterrupted()) { lemo wrote: > lemo wrote: > > lemo wrote: > > >

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

lemo added inline comments. Comment at: source/Interpreter/CommandInterpreter.cpp:2708-2710 +const char *data = str.data(); +size_t size = str.size(); +while (size > 0 && !WasInterrupted()) { lemo wrote: > lemo wrote: > > clayborg wrote: > > > Since

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

lemo added inline comments. Comment at: source/Interpreter/CommandInterpreter.cpp:2708-2710 +const char *data = str.data(); +size_t size = str.size(); +while (size > 0 && !WasInterrupted()) { lemo wrote: > clayborg wrote: > > Since we are using

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

lemo updated this revision to Diff 115516. lemo edited the summary of this revision. lemo added a comment. Incorporating CR feedback. https://reviews.llvm.org/D37923 Files: include/lldb/Core/IOHandler.h include/lldb/Interpreter/CommandInterpreter.h source/Commands/CommandObjectTarget.cpp

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

lemo added inline comments. Comment at: source/Interpreter/CommandInterpreter.cpp:2713 + for (; chunk_size < size; ++chunk_size) { +assert(data[chunk_size] != '\0'); +if (data[chunk_size] == '\n') { amccarth wrote: > Should we be that

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

clayborg added inline comments. Comment at: source/Commands/CommandObjectTarget.cpp:2056-2058 +if (m_interpreter.WasInterrupted()) { + break; +} Remove braces Comment at:

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

lemo added inline comments. Comment at: source/Commands/CommandObjectTarget.cpp:2058 + break; +} num_dumped++; amccarth wrote: > Many LLVM developers prefer to omit the braces when the body of the > control-flow statement

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

amccarth added inline comments. Comment at: source/Commands/CommandObjectTarget.cpp:2058 + break; +} num_dumped++; Many LLVM developers prefer to omit the braces when the body of the control-flow statement is a single

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

lemo updated this revision to Diff 115476. lemo added a comment. Split the SIGINT handles fixes into a stanalone patch https://reviews.llvm.org/D37923 Files: include/lldb/Core/IOHandler.h include/lldb/Interpreter/CommandInterpreter.h source/Commands/CommandObjectTarget.cpp

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

amccarth added a comment. I haven't looked at the whole patch yet, but it seems the SIGINT fix is well isolated. That should probably be in a separate patch. https://reviews.llvm.org/D37923 ___ lldb-commits mailing list

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

lemo created this revision. Herald added a subscriber: ki.stfu. The core of this change is the new CommandInterpreter::m_command_state, which models the state transitions for interactive commands, including an "interrupted" state transition. In general, command interruption requires