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

[Jim Ingham via lldb-commits]

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 crucial step you've internalized so that it isn't even conscious anymore...

Jim


> On Sep 19, 2017, at 1:04 PM, Leonard Mosescu <mosescu at google.com> wrote:
> 
> 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 <jingham at apple.com> wrote:
> 
> > On Sep 19, 2017, at 11:25 AM, Leonard Mosescu <mosescu at google.com> 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 chime in. I was thinking just a pexpect based test.
> > I love tests but what exactly do we want to test here? Sorry if I'm missing something obvious - are you talking about splitting a string into chunks? The actual interruption? ...
> >
> > Also, I like the idea of exposing this though the SB APIs, but that's a significant expansion of the original goal which is to improve the interactive LLDB debugger.
> 
> This is sort of a side point, but one of the goals of lldb is to make the command set extensible by adding commands implemented with the SB API's either from Python or from C++ (Python is more common but both are possible.)  And there are a bunch of groups here at Apple that have all sorts of special purpose LLDB commands implemented this way.  The xnu.py and Co. that gets distributed with the kernel development kit has a wide array of examples of this that are publicly available if you want to have a look.
> 
> So the SB API's are in fact an intrinsic part of the interactive LLDB debugger.  We aren't there yet (there isn't an SB API to define command options & arguments so SB added commands don't parse like lldb commands do) but the end goal is that it you can't tell whether commands are from SB API's or are builtin.
> 
> BTW, I think the fact that we use lldb_private API's is an unfortunate artifact of how lldb was implemented.  Having to use our vended API's to implement the commands would make them better implemented - since we would really be on the hook for them, and would reduce a lot of code duplication where we do things one way in the commands and slightly differently in the equivalent SB API's, and reduce the testing surface considerably.  I'm not sure this is a project we will ever actually get to, but it is a good aspirational goal to keep in mind.
> 
> We didn't do it that way originally because we needed something we could poke at early on in the development of lldb, at a point where it would have been premature to design the SB API's.  Then lldb went from skunkworks project to mission critical debugger too quickly for us to be able to revise the decision.
> 
> Jim
> 
> 
> 
> > It may be nice looking at SB APIs later on, but I'm afraid that trying to pile up everything in one change is just going to spiral the complexity out of control.
> >
> > On Tue, Sep 19, 2017 at 11:15 AM, Zachary Turner <zturner at google.com> wrote:
> > 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 passes through to is a strictly better test, since it's focusing the test on the underlying piece of functionality and removing additional sources of failure and instability from the test.
> >
> > On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham <jingham at apple.com> wrote:
> > 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 you should ask yourself is whether this is useful at the SB API layer.  In this case it obviously is (actually I'm a little embarrassed I didn't think of this requirement).  If so, then it's simplest to test it at that layer.  If the SB API is anything more than a pass-through, then you might also want to write a unit test for the underlying C++ API's.  But in this case the SB function will just call the underlying CommandInterpreter one, so testing at the SB layer is sufficient.
> >
> > Jim
> >
> > > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator <reviews at reviews.llvm.org> wrote:
> > >
> > > zturner added a comment.
> > >
> > > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> > >
> > >> 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.
> > >
> > >
> > > This kind of thing is perfect for a unit test, but I'm not sure how easy that would be with the current design.  You'd basically do something like:
> > >
> > >  struct MockStream {
> > >    explicit MockStream(CommandInterpreter &Interpreter, int InterruptAfter)
> > >      : CommandInterpreter(Interpreter), InterruptAfter(InterruptAfter) {}
> > >
> > >    CommandInterpreter &Interpreter;
> > >    const int InterruptAfter;
> > >    int Lines = 0;
> > >    std::string Buffer;
> > >
> > >    void Write(StringRef S) {
> > >      ++Lines;
> > >      if (Lines >= InterruptAfter) {
> > >        Interpreter.Interrupt();
> > >        return;
> > >      }
> > >      Buffer += S;
> > >    }
> > >  };
> > >
> > >  TEST_F(CommandInterruption) {
> > >    CommandInterpreter Interpreter;
> > >    MockStream Stream(Interpreter, 3);
> > >    Interpreter.PrintCommandOutput(Stream, "a\nb\nc\nd\ne\nf\n");
> > >    EXPECT_EQ(Stream.Lines == 3);
> > >    EXPECT_EQ(Stream.Buffer == "a\nb\nc\n");
> > >  }
> > >
> > >
> > > https://reviews.llvm.org/D37923
> > >
> > >
> > >
> >
> >
> 
>