[Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Davide Italiano via Phabricator via lldb-commits]

davide abandoned this revision.
davide added a comment.

Cleaning up my queue.


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

https://reviews.llvm.org/D43048



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

Re: [Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Pavel Labath via lldb-commits]

First, I want to apologise for derailing the tab completion review.
However, now that the cat's out of the bag, let me elaborate on what I
meant.

For example, this is how a typical instruction emulation test looks right now:

TEST_F(Testx86AssemblyInspectionEngine, TestSimple64bitFrameFunction) {
  std::unique_ptr engine = Getx86_64Inspector();

  // 'int main() { }' compiled for x86_64-apple-macosx with clang
  uint8_t data[] = {
  0x55, // offset 0 -- pushq %rbp
  0x48, 0x89, 0xe5, // offset 1 -- movq %rsp, %rbp
  0x31, 0xc0,   // offset 4 -- xorl %eax, %eax
  0x5d, // offset 6 -- popq %rbp
  0xc3  // offset 7 -- retq
  };

  AddressRange sample_range(0x1000, sizeof(data));

  UnwindPlan unwind_plan(eRegisterKindLLDB);
  EXPECT_TRUE(engine->GetNonCallSiteUnwindPlanFromAssembly(
  data, sizeof(data), sample_range, unwind_plan));

  // Expect four unwind rows:
  // 0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
  // 1: CFA=rsp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
  // 4: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
  // 7: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]

  EXPECT_TRUE(unwind_plan.GetInitialCFARegister() == k_rsp);
  EXPECT_TRUE(unwind_plan.GetUnwindPlanValidAtAllInstructions() ==
  eLazyBoolYes);
  EXPECT_TRUE(unwind_plan.GetSourcedFromCompiler() == eLazyBoolNo);

  UnwindPlan::Row::RegisterLocation regloc;

  // 0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
  UnwindPlan::RowSP row_sp = unwind_plan.GetRowForFunctionOffset(0);
  EXPECT_EQ(0ull, row_sp->GetOffset());
  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_rsp);
  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
  EXPECT_EQ(8, row_sp->GetCFAValue().GetOffset());

  EXPECT_TRUE(row_sp->GetRegisterInfo(k_rip, regloc));
  EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
  EXPECT_EQ(-8, regloc.GetOffset());

  // 1: CFA=rsp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
  row_sp = unwind_plan.GetRowForFunctionOffset(1);
  EXPECT_EQ(1ull, row_sp->GetOffset());
  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_rsp);
  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
  EXPECT_EQ(16, row_sp->GetCFAValue().GetOffset());

  EXPECT_TRUE(row_sp->GetRegisterInfo(k_rip, regloc));
  EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
  EXPECT_EQ(-8, regloc.GetOffset());

  // 4: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
  row_sp = unwind_plan.GetRowForFunctionOffset(4);
  EXPECT_EQ(4ull, row_sp->GetOffset());
  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_rbp);
  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
  EXPECT_EQ(16, row_sp->GetCFAValue().GetOffset());

  EXPECT_TRUE(row_sp->GetRegisterInfo(k_rip, regloc));
  EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
  EXPECT_EQ(-8, regloc.GetOffset());

  // 7: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
  row_sp = unwind_plan.GetRowForFunctionOffset(7);
  EXPECT_EQ(7ull, row_sp->GetOffset());
  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_rsp);
  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
  EXPECT_EQ(8, row_sp->GetCFAValue().GetOffset());

  EXPECT_TRUE(row_sp->GetRegisterInfo(k_rip, regloc));
  EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
  EXPECT_EQ(-8, regloc.GetOffset());
}

As you see, in order to write a test like this, somebody had to
assemble a function demonstrating the issue, copy
it's bytes into the test, and then write series of C++ checks to make
sure that the result is correct.

 With FileCheck, we could basically remove everything **except** the
comments from this test. So this
would become something like:

# RUN: llvm-mc -target x86_64-apple-macosx %s | lldb-test unwind
--emulate - | FileCheck %s
.text:
  pushq %rbp
# CHECK: 0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
 movq %rsp, %rbp
# CHECK: 1: CFA=rsp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
  xorl %eax, %eax
# CHECK: 4: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
 popq %rbp
# CHECK: 7: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
 retq


I was hoping we could all agree on that the latter test looks much
simpler. And it still is (can be made to) testing the exact same
functionality as the original test.

I think part of the reason that this part of code lacks better
coverage (even though it's very suitable for unit testing) is that its
very tedious to write tests like this. If adding a new test were as
simple as this, we could easily add dozens of tests for each
architecture.

PS: I was deliberately trying to stay clear of the discussion on how
to test the higher level unwinding logic, as I know that's a more
complicated/contentious issue. However, Davide seemed like he was
looking for things to FileCheck-ify, so I wanted to point this out to
him, as I believe this would make testing of this particular component
much easier, with very little upfront investment.


On 12 February 2018 at 22:29, 

Re: [Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Jason Molenda via lldb-commits]

Ah, no.  Pavel suggested that the unwind plan unittests should be turned into 
FileCheck tests, and then Davide suggested that he'd heard unwind testing is 
difficult (he was conflating the unwind sources -> UnwindPlan IR conversions 
and the runtime use of UnwindPlans to walk the stack & find spilled registers), 
so he thought that this FileCheck could help there.  I was clarifying that (1) 
I don't want the existing tests I wrote changed out of unittests, and (2) 
FileCheck does not help the actually difficult part of testing the unwinder at 
all.  I provided an example of why this was difficult, outlining the possible 
approaches, and saying that if you were doing it today, you would need to do it 
with hand written assembly, and you suggested that a process mock style 
approach would be awesome, which was one of the approaches I described in my 
original email.

> On Feb 12, 2018, at 2:18 PM, Zachary Turner  wrote:
> 
> Sure I don’t think anyone disputes that, but I thought we were discussing an 
> ideal end state
> On Mon, Feb 12, 2018 at 1:31 PM Jason Molenda  wrote:
> 
> 
> > On Feb 12, 2018, at 12:59 PM, Zachary Turner  wrote:
> >
> >
> >
> > On Mon, Feb 12, 2018 at 12:52 PM Jason Molenda  wrote:
> >>
> >>
> >> > On Feb 12, 2018, at 12:48 PM, Zachary Turner via Phabricator 
> >> >  wrote:
> >> >
> >> > zturner added a comment.
> >> >
> >> > In https://reviews.llvm.org/D43048#1005513, @jasonmolenda wrote:
> >> >
> >> > I don't think we'd necessarily need a live register context or stack 
> >> > memory.  A mock register context and stack memory should be sufficient, 
> >> > with an emulator that understands only a handful of instructions.
> >>
> >>
> >> That's ... exactly what I said?  That we need to mock up memory and 
> >> registers and symbols, or have a corefile and binary, or have hand written 
> >> assembly routines that set up a particular stack and an SB API test that 
> >> tries to backtrace out of it with a live process.  After the inferior 
> >> process state has been constructed/reconstituted, is the unwinder capable 
> >> of walking the stack or finding spilled registers by following the correct 
> >> UnwindPlans.  This is right in the wheelhouse of SB API testing.
> >
> > I'm saying we shouldn't need a live process (and in fact we can test it 
> > better if we don't rely on a live process, since we can write tests that 
> > run anywhere).
> 
> 
> Yes, as we've all agreed many times for years, a ProcessMock style Process 
> plugin which can fake up state from a yaml file would be a great addition -- 
> and for the unwind tests, we need a way to provide symbolic information and 
> possibly even eh_frame information from the "binaries", and maybe even a way 
> to construct the yaml representation from an actual debug session.  No one is 
> disagreeing with this.
> 
> But the fact that no one has had time to develop this plugin means that if I 
> want to write an unwind test today, I either need to allocate time to write 
> the above infrastructure first, or write the test given the tools we have 
> available to us today.
> 
> An unwind test that only runs on x86_64, or even only runs on x86_64 Darwin, 
> is not ideal, but it is much better than no test at all especially in the 
> world of buildbots that can flag a problem with a change right away.
> 
> J

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

Re: [Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Zachary Turner via lldb-commits]

Sure I don’t think anyone disputes that, but I thought we were discussing
an ideal end state
On Mon, Feb 12, 2018 at 1:31 PM Jason Molenda  wrote:

>
>
> > On Feb 12, 2018, at 12:59 PM, Zachary Turner  wrote:
> >
> >
> >
> > On Mon, Feb 12, 2018 at 12:52 PM Jason Molenda 
> wrote:
> >>
> >>
> >> > On Feb 12, 2018, at 12:48 PM, Zachary Turner via Phabricator <
> revi...@reviews.llvm.org> wrote:
> >> >
> >> > zturner added a comment.
> >> >
> >> > In https://reviews.llvm.org/D43048#1005513, @jasonmolenda wrote:
> >> >
> >> > I don't think we'd necessarily need a live register context or stack
> memory.  A mock register context and stack memory should be sufficient,
> with an emulator that understands only a handful of instructions.
> >>
> >>
> >> That's ... exactly what I said?  That we need to mock up memory and
> registers and symbols, or have a corefile and binary, or have hand written
> assembly routines that set up a particular stack and an SB API test that
> tries to backtrace out of it with a live process.  After the inferior
> process state has been constructed/reconstituted, is the unwinder capable
> of walking the stack or finding spilled registers by following the correct
> UnwindPlans.  This is right in the wheelhouse of SB API testing.
> >
> > I'm saying we shouldn't need a live process (and in fact we can test it
> better if we don't rely on a live process, since we can write tests that
> run anywhere).
>
>
> Yes, as we've all agreed many times for years, a ProcessMock style Process
> plugin which can fake up state from a yaml file would be a great addition
> -- and for the unwind tests, we need a way to provide symbolic information
> and possibly even eh_frame information from the "binaries", and maybe even
> a way to construct the yaml representation from an actual debug session.
> No one is disagreeing with this.
>
> But the fact that no one has had time to develop this plugin means that if
> I want to write an unwind test today, I either need to allocate time to
> write the above infrastructure first, or write the test given the tools we
> have available to us today.
>
> An unwind test that only runs on x86_64, or even only runs on x86_64
> Darwin, is not ideal, but it is much better than no test at all especially
> in the world of buildbots that can flag a problem with a change right away.
>
> J
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Jason Molenda via lldb-commits]

> On Feb 12, 2018, at 12:59 PM, Zachary Turner  wrote:
> 
> 
> 
> On Mon, Feb 12, 2018 at 12:52 PM Jason Molenda  wrote:
>> 
>> 
>> > On Feb 12, 2018, at 12:48 PM, Zachary Turner via Phabricator 
>> >  wrote:
>> >
>> > zturner added a comment.
>> >
>> > In https://reviews.llvm.org/D43048#1005513, @jasonmolenda wrote:
>> >
>> > I don't think we'd necessarily need a live register context or stack 
>> > memory.  A mock register context and stack memory should be sufficient, 
>> > with an emulator that understands only a handful of instructions.
>> 
>> 
>> That's ... exactly what I said?  That we need to mock up memory and 
>> registers and symbols, or have a corefile and binary, or have hand written 
>> assembly routines that set up a particular stack and an SB API test that 
>> tries to backtrace out of it with a live process.  After the inferior 
>> process state has been constructed/reconstituted, is the unwinder capable of 
>> walking the stack or finding spilled registers by following the correct 
>> UnwindPlans.  This is right in the wheelhouse of SB API testing.
> 
> I'm saying we shouldn't need a live process (and in fact we can test it 
> better if we don't rely on a live process, since we can write tests that run 
> anywhere).


Yes, as we've all agreed many times for years, a ProcessMock style Process 
plugin which can fake up state from a yaml file would be a great addition -- 
and for the unwind tests, we need a way to provide symbolic information and 
possibly even eh_frame information from the "binaries", and maybe even a way to 
construct the yaml representation from an actual debug session.  No one is 
disagreeing with this.

But the fact that no one has had time to develop this plugin means that if I 
want to write an unwind test today, I either need to allocate time to write the 
above infrastructure first, or write the test given the tools we have available 
to us today.  

An unwind test that only runs on x86_64, or even only runs on x86_64 Darwin, is 
not ideal, but it is much better than no test at all especially in the world of 
buildbots that can flag a problem with a change right away.

J
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Zachary Turner via lldb-commits]

On Mon, Feb 12, 2018 at 12:52 PM Jason Molenda  wrote:

>
>
> > On Feb 12, 2018, at 12:48 PM, Zachary Turner via Phabricator <
> revi...@reviews.llvm.org> wrote:
> >
> > zturner added a comment.
> >
> > In https://reviews.llvm.org/D43048#1005513, @jasonmolenda wrote:
> >
> > I don't think we'd necessarily need a live register context or stack
> memory.  A mock register context and stack memory should be sufficient,
> with an emulator that understands only a handful of instructions.
>
>
> That's ... exactly what I said?  That we need to mock up memory and
> registers and symbols, or have a corefile and binary, or have hand written
> assembly routines that set up a particular stack and an SB API test that
> tries to backtrace out of it with a live process.  After the inferior
> process state has been constructed/reconstituted, is the unwinder capable
> of walking the stack or finding spilled registers by following the correct
> UnwindPlans.  This is right in the wheelhouse of SB API testing.
>

I'm saying we shouldn't need a live process (and in fact we can test it
better if we don't rely on a live process, since we can write tests that
run anywhere).
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Jason Molenda via lldb-commits]

> On Feb 12, 2018, at 12:48 PM, Zachary Turner via Phabricator 
>  wrote:
> 
> zturner added a comment.
> 
> In https://reviews.llvm.org/D43048#1005513, @jasonmolenda wrote:
> 
>> No, the unwind unittests that exist today should stay written as unit tests. 
>>  These are testing the conversion of native unwind formats -- for instance, 
>> eh_frame, compact unwind, or instruction analysis -- into the intermediate 
>> UnwindPlan representation in lldb.  They are runtime invariant, unit tests 
>> are the best approach to these.  If there were anything to say about these, 
>> it would be that we need more testing here - armv7 (AArch32) into UnwindPlan 
>> is not tested.  eh_frame and compact_unwind into UnwindPlan is not tested.
> 
> 
> That's exactly the type of thing that FileCheck tests work best for.  I'm not 
> sure why you're saying that unittests are better than FileCheck tests for 
> this scenario.


I'm saying that the unittests here are entirely appropriate, and that rewriting 
existing tests in FileCheck style doesn't make any sense to me.  If people want 
to write new tests using that style, that's fine I guess, but I wrote these 
tests for a part of lldb that I maintain and I'd prefer to keep them as-is.


> 
>> The part of unwind that is difficult to test is the runtime unwind behavior, 
>> and FileCheck style tests don't make that easier in any way.  We need a live 
>> register context, stack memory, symbols and UnwindPlans to test this 
>> correctly -- we either need a full ProcessMock with SymbolVendorMock etc, or 
>> we need corefiles, or we need tests with hand-written assembly code.
> 
> I don't think we'd necessarily need a live register context or stack memory.  
> A mock register context and stack memory should be sufficient, with an 
> emulator that understands only a handful of instructions.


That's ... exactly what I said?  That we need to mock up memory and registers 
and symbols, or have a corefile and binary, or have hand written assembly 
routines that set up a particular stack and an SB API test that tries to 
backtrace out of it with a live process.  After the inferior process state has 
been constructed/reconstituted, is the unwinder capable of walking the stack or 
finding spilled registers by following the correct UnwindPlans.  This is right 
in the wheelhouse of SB API testing.

J
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

In https://reviews.llvm.org/D43048#1005513, @jasonmolenda wrote:

> No, the unwind unittests that exist today should stay written as unit tests.  
> These are testing the conversion of native unwind formats -- for instance, 
> eh_frame, compact unwind, or instruction analysis -- into the intermediate 
> UnwindPlan representation in lldb.  They are runtime invariant, unit tests 
> are the best approach to these.  If there were anything to say about these, 
> it would be that we need more testing here - armv7 (AArch32) into UnwindPlan 
> is not tested.  eh_frame and compact_unwind into UnwindPlan is not tested.


That's exactly the type of thing that FileCheck tests work best for.  I'm not 
sure why you're saying that unittests are better than FileCheck tests for this 
scenario.

> The part of unwind that is difficult to test is the runtime unwind behavior, 
> and FileCheck style tests don't make that easier in any way.  We need a live 
> register context, stack memory, symbols and UnwindPlans to test this 
> correctly -- we either need a full ProcessMock with SymbolVendorMock etc, or 
> we need corefiles, or we need tests with hand-written assembly code.

I don't think we'd necessarily need a live register context or stack memory.  A 
mock register context and stack memory should be sufficient, with an emulator 
that understands only a handful of instructions.


https://reviews.llvm.org/D43048



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

[Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

No, the unwind unittests that exist today should stay written as unit tests.  
These are testing the conversion of native unwind formats -- for instance, 
eh_frame, compact unwind, or instruction analysis -- into the intermediate 
UnwindPlan representation in lldb.  They are runtime invariant, unit tests are 
the best approach to these.  If there were anything to say about these, it 
would be that we need more testing here - armv7 (AArch32) into UnwindPlan is 
not tested.  eh_frame and compact_unwind into UnwindPlan is not tested.

The part of unwind that is difficult to test is the runtime unwind behavior, 
and FileCheck style tests don't make that easier in any way.  We need a live 
register context, stack memory, symbols and UnwindPlans to test this correctly 
-- we either need a full ProcessMock with SymbolVendorMock etc, or we need 
corefiles, or we need tests with hand-written assembly code.

I'm leaning towards starting to write C test cases with hand written assembly 
routines that set up their stacks in unusual ways to test the unwinder, I've 
written unwind testsuites in the past in this way and it works well but it does 
mean that you need to be on the arch (and possibly even the OS) that the test 
is written in -- bots will show the failures for everyone after a commit goes 
in, at least.  But these will be written as SB API tests.


https://reviews.llvm.org/D43048



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

[Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

In https://reviews.llvm.org/D43048#1004807, @labath wrote:

> (Btw, if you're looking for things to FileCheck-ify, I think the stuff under 
> `lldb/unittests/UnwindAssembly` is a prime candidate and has a much higher 
> bang/buck ratio.)


If you have ideas on how to FileCheck'ify unwind testing, I'm all for it. This 
recently came up in a discussion where the unwinder has been pointed out as 
hard to test.
Feel free to ping me offline (or just send me a mail) and we can start thinking 
about something [and then propose upstream]


https://reviews.llvm.org/D43048



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

[Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

(Btw, if you're looking for things to FileCheck-ify, I think the stuff under 
`lldb/unittests/UnwindAssembly` is a prime candidate and has a much higher 
bang/buck ratio.)


https://reviews.llvm.org/D43048



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

[Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The change seems fine to me, and I don't really have anything to add to the 
things that were said already.

Testing the completion of things that require a target/module/etc. will be a 
bit tricky, but that's something we should figure out anyway to have more 
targeted completion tests.


https://reviews.llvm.org/D43048



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

Re: [Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Davide Italiano via lldb-commits]

On Wed, Feb 7, 2018 at 8:20 PM, Adrian Prantl  wrote:

>
>
> > On Feb 7, 2018, at 6:40 PM, Zachary Turner  wrote:
>
> >  and the command line in the log file doesn’t ever work for me'
>
> That's a bug. Can you show me an example where this breaks for you? I'd
> like to investigate this.
>
> -- adrian


Adrian,
I think I reported this issue a while ago, but I haven't checked whether it
still reproduces.
You might take a look at the original bug if you get a chance

https://bugs.llvm.org/show_bug.cgi?id=35037

Thanks!

--
Davide
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Adrian Prantl via lldb-commits]

> On Feb 7, 2018, at 6:40 PM, Zachary Turner  wrote:

>  and the command line in the log file doesn’t ever work for me'

That's a bug. Can you show me an example where this breaks for you? I'd like to 
investigate this.

-- adrian
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Zachary Turner via lldb-commits]

Yes but debugging across several api calls is annoying, and the command
line in the log file doesn’t ever work for me
On Wed, Feb 7, 2018 at 6:07 PM Jim Ingham via Phabricator <
revi...@reviews.llvm.org> wrote:

> jingham added a comment.
>
> If a dotest test fails, you go to the Failure-whatever.log, take the last
> line, add a -d to it, and run it.  It suspends itself, then you attach to
> the Python instance with the debugger.
>
>
> https://reviews.llvm.org/D43048
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

If a dotest test fails, you go to the Failure-whatever.log, take the last line, 
add a -d to it, and run it.  It suspends itself, then you attach to the Python 
instance with the debugger.


https://reviews.llvm.org/D43048



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

Re: [Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Zachary Turner via lldb-commits]

Also, failures that are easy to reproduce are easy to debug. When a test
fails this way, you get a command line that can reproduce the problem that
can be debugged directly without having to debug across the python
boundary. I find that very helpful personally
On Wed, Feb 7, 2018 at 5:48 PM Zachary Turner  wrote:

> Same reason that people use perl for heavy text processing, R for
> scientific programming, python for rapid iteration. It’s what they’re built
> for. When something is built for a very focused specific problem domain,
> the problems in that domain can be expressed very concisely and naturally.
>
> In the current python test there’s 4-6 lines of Python boilerplate for
> every 2-3 lines of test “meat”.  And it’s all code, making matters even
> worse.
>
> A FileCheck test will have approximately 0 lines of text that aren’t part
> of the “meat” of the test, and on top of that it can poke at every low
> level detail of a system, not just those that are blessed with an api
>
> On Wed, Feb 7, 2018 at 5:29 PM Jim Ingham via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> jingham requested changes to this revision.
>> jingham added a comment.
>> This revision now requires changes to proceed.
>>
>> You do care about the common match string.  When the lldb driver handles
>> completion, if the common match string is not null, we append that to the
>> line at the cursor position, then present the matches if there is more than
>> one.  So the common match string also has to be tested.
>>
>> The ability to page the completion requests in the API would be useful
>> for instance in symbol completion where you can get lots of matches, but if
>> you only plan to display the first page you'd rather not pay the cost to go
>> find them all.  I put that in the SB API's because I didn't want to have to
>> add another one when I got around to implementing this.  When I get around
>> to this I'll fix the docs...  But you could remove that from the lldb
>> private version if you're so motivated.  I'll still remember I intended to
>> extend it this way, even if nobody else will see that.
>>
>> We can't return a std::pair across the SB API's, but we could make the
>> common match be another parameter.  There was some reason this seemed
>> logical to me at the time, but I must admit I can't remember why now.  It
>> is in practice easy to use, however.  You append element 0 to the cursor
>> position, then print the rest of the completions if num_matches is > 1.
>> Again, feel free to switch the lldb_private API if it bugs you.
>>
>> An additional test in the Python testsuite is:
>>
>>   def test_target_create_dash_co(self):
>>   """Test that 'target create --co' completes to 'target variable
>> --core '."""
>>   self.complete_from_to('target create --co', 'target create --core ')
>>
>> So I still don't see why the file check method is preferable.  But to
>> each his own, I guess.
>>
>>
>> https://reviews.llvm.org/D43048
>>
>>
>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Zachary Turner via lldb-commits]

Same reason that people use perl for heavy text processing, R for
scientific programming, python for rapid iteration. It’s what they’re built
for. When something is built for a very focused specific problem domain,
the problems in that domain can be expressed very concisely and naturally.

In the current python test there’s 4-6 lines of Python boilerplate for
every 2-3 lines of test “meat”.  And it’s all code, making matters even
worse.

A FileCheck test will have approximately 0 lines of text that aren’t part
of the “meat” of the test, and on top of that it can poke at every low
level detail of a system, not just those that are blessed with an api

On Wed, Feb 7, 2018 at 5:29 PM Jim Ingham via Phabricator <
revi...@reviews.llvm.org> wrote:

> jingham requested changes to this revision.
> jingham added a comment.
> This revision now requires changes to proceed.
>
> You do care about the common match string.  When the lldb driver handles
> completion, if the common match string is not null, we append that to the
> line at the cursor position, then present the matches if there is more than
> one.  So the common match string also has to be tested.
>
> The ability to page the completion requests in the API would be useful for
> instance in symbol completion where you can get lots of matches, but if you
> only plan to display the first page you'd rather not pay the cost to go
> find them all.  I put that in the SB API's because I didn't want to have to
> add another one when I got around to implementing this.  When I get around
> to this I'll fix the docs...  But you could remove that from the lldb
> private version if you're so motivated.  I'll still remember I intended to
> extend it this way, even if nobody else will see that.
>
> We can't return a std::pair across the SB API's, but we could make the
> common match be another parameter.  There was some reason this seemed
> logical to me at the time, but I must admit I can't remember why now.  It
> is in practice easy to use, however.  You append element 0 to the cursor
> position, then print the rest of the completions if num_matches is > 1.
> Again, feel free to switch the lldb_private API if it bugs you.
>
> An additional test in the Python testsuite is:
>
>   def test_target_create_dash_co(self):
>   """Test that 'target create --co' completes to 'target variable
> --core '."""
>   self.complete_from_to('target create --co', 'target create --core ')
>
> So I still don't see why the file check method is preferable.  But to each
> his own, I guess.
>
>
> https://reviews.llvm.org/D43048
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Jim Ingham via Phabricator via lldb-commits]

jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

You do care about the common match string.  When the lldb driver handles 
completion, if the common match string is not null, we append that to the line 
at the cursor position, then present the matches if there is more than one.  So 
the common match string also has to be tested.

The ability to page the completion requests in the API would be useful for 
instance in symbol completion where you can get lots of matches, but if you 
only plan to display the first page you'd rather not pay the cost to go find 
them all.  I put that in the SB API's because I didn't want to have to add 
another one when I got around to implementing this.  When I get around to this 
I'll fix the docs...  But you could remove that from the lldb private version 
if you're so motivated.  I'll still remember I intended to extend it this way, 
even if nobody else will see that.

We can't return a std::pair across the SB API's, but we could make the common 
match be another parameter.  There was some reason this seemed logical to me at 
the time, but I must admit I can't remember why now.  It is in practice easy to 
use, however.  You append element 0 to the cursor position, then print the rest 
of the completions if num_matches is > 1.  Again, feel free to switch the 
lldb_private API if it bugs you.

An additional test in the Python testsuite is:

  def test_target_create_dash_co(self):
  """Test that 'target create --co' completes to 'target variable --core 
'."""
  self.complete_from_to('target create --co', 'target create --core ')

So I still don't see why the file check method is preferable.  But to each his 
own, I guess.


https://reviews.llvm.org/D43048



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

[Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

On the issue of keeping the other test, I think when an SB API method is 
basically a pass-through to a private method, then having a test of the SB API 
method that verifies "did the correct native method get called" is useful if 
for no other reason than to verify the correctness of the SWIG binding 
generation.  If you've tested that the API method invokes the correct native 
method, and you've tested the native method directly with a large amount of 
inputs, then that's sufficient overall coverage IMO


https://reviews.llvm.org/D43048



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

[Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

In https://reviews.llvm.org/D43048#1001293, @davide wrote:

> In https://reviews.llvm.org/D43048#1001287, @jingham wrote:
>
> > The current auto-completer tests aren't interactive - they do exactly the 
> > same thing your command does, but from Python.  It's fine if you want to 
> > add tests but please don't remove the current tests since they directly 
> > test what the SB clients use.
>
>
> This is a drop-in replacement for the other test. I'm not sure why we need to 
> keep the other test, but I'll let others comment.
>
> > This will only allow you to test some of the auto-completers, for instance 
> > you don't have symbols so you can't test the symbol completer.  But since 
> > the symbol commands in this lldb-test have some way to feed symbols in 
> > maybe you can crib that.  I think you'll also need to make a target and 
> > some other bits as well.  As you start adding these you might find that 
> > this becomes onerous, but that will be an interesting experiment.
>
> I voluntarily left that as as follow up.
>
> > You didn't get the HandleCompletion API quite right.  That's my fault this 
> > isn't well documented.  The way it works is that if all the potential 
> > matches share a common substring, then the 0th element of the result 
> > contains the common substring.  If there is no common substring, then the 
> > possible matches will be stored from element 1 on in the Results list.  If 
> > you want examples take a closer look at how the Python test does it.
>
> The API is a little confusing.
>  There are multiple issues IMHO:
>
> 1. We shouldn't really discriminate based on the position of the elements of 
> the list, as it's easy to get them wrong, as I did. Instead, the API might 
> return a, let's say, pair, where the first element is the common substring 
> and the second element is a list containing etc..
> 2. We pass strings for the second and third argument (cursor/end), when we 
> should just pass offsets
> 3. The return value is N and the list contains N +1 values. This is very 
> error prone.


Also, there's a bit of overengineering in the API. As far as I can tell, 
max_return_elements only supports `-1`, so that argument is bogus (at least, 
this is what the comment says, but comment and code could go out of sync, so).


https://reviews.llvm.org/D43048



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

[Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

The current auto-completer tests aren't interactive - they do exactly the same 
thing your command does, but from Python.  It's fine if you want to add tests 
but please don't remove the current tests since they directly test what the SB 
clients use.

This will only allow you to test some of the auto-completers, for instance you 
don't have symbols so you can't test the symbol completer.  But since the 
symbol commands in this lldb-test have some way to feed symbols in maybe you 
can crib that.  I think you'll also need to make a target and some other bits 
as well.  As you start adding these you might find that this becomes onerous, 
but that will be an interesting experiment.

You didn't get the HandleCompletion API quite right.  That's my fault this 
isn't well documented.  The way it works is that if all the potential matches 
share a common substring, then the 0th element of the result contains the 
common substring.  If there is no common substring, then the possible matches 
will be stored from element 1 on in the Results list.  If you want examples 
take a closer look at how the Python test does it.


https://reviews.llvm.org/D43048



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

[Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

In the future, we could add options to the `autocomplete` subcommand that would 
allow specification of a target, and things like cursor position to maximize 
testability.

In general though, I like the approach.  It's not hard to imagine 50+ tests 
being written just for autocomplete this way.


https://reviews.llvm.org/D43048



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

[Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

You can take a look at the 
`test/testcases/functionalities/completion/TestCompletion.py` for the python 
equivalent. I find the potential FileCheck'ed version much easier to 
read/write/understand.
I'm possibly biased having worked many years on LLVM, hence I'm asking for 
general feedback.


https://reviews.llvm.org/D43048



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

[Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

The general direction lgtm, I'd be happy if we could replace interactive 
autocomplete tests with this.


https://reviews.llvm.org/D43048



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

[Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Davide Italiano via Phabricator via lldb-commits]

davide created this revision.
davide added reviewers: aprantl, vsk, friss, labath, zturner, jingham, 
jasonmolenda.

This is an experiment to improve out lldb testing capabilities and making them 
more similar to the one used in LLVM.

Example:

  davide@Davidinos-Mac-Pro ~/w/l/b/bin> ./lldb-test autocomplete "target cr"
  create
  davide@Davidinos-Mac-Pro ~/w/l/b/bin> ./lldb-test autocomplete "target "
  create
  delete
  list
  modules
  select
  stop-hook
  symbols
  variable

This allows the output to be FileCheck'ed, and has the advantage that it 
doesn't depend on python to be executed. It also removes a bunch of boilerplate 
the current autocompletion tests have.
Any feedback on this is appreciated, before I write the actual FileCheck tests.


https://reviews.llvm.org/D43048

Files:
  lldb/tools/lldb-test/lldb-test.cpp


Index: lldb/tools/lldb-test/lldb-test.cpp
===
--- lldb/tools/lldb-test/lldb-test.cpp
+++ lldb/tools/lldb-test/lldb-test.cpp
@@ -15,6 +15,7 @@
 #include "lldb/Core/Module.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Initialization/SystemLifetimeManager.h"
+#include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Symbol/ClangASTContext.h"
 #include "lldb/Symbol/ClangASTImporter.h"
 #include "lldb/Utility/DataExtractor.h"
@@ -32,10 +33,16 @@
 using namespace llvm;
 
 namespace opts {
+cl::SubCommand AutoCompleteSubCommand("autocomplete", "Test LLDB 
autocomplete");
 cl::SubCommand ModuleSubcommand("module-sections",
 "Display LLDB Module Information");
 cl::SubCommand SymbolsSubcommand("symbols", "Dump symbols for an object file");
 
+namespace autocomplete {
+cl::list Input(cl::Positional, cl::desc("input patterns"),
+cl::Required, cl::sub(AutoCompleteSubCommand));
+}
+
 namespace module {
 cl::opt SectionContents("contents",
   cl::desc("Dump each section's contents"),
@@ -52,6 +59,22 @@
 
 static llvm::ManagedStatic DebuggerLifetime;
 
+static void autocompleteCommand(Debugger ) {
+  assert(opts::autocomplete::Input.length() == 1 && "Incorret number of args");
+  std::string InputStr = opts::autocomplete::Input[0];
+  lldb_private::StringList Results;
+  CommandInterpreter  = Dbg.GetCommandInterpreter();
+  unsigned Matches = CI.HandleCompletion(
+  InputStr.c_str(), InputStr.c_str() + InputStr.size(),
+  InputStr.c_str() + InputStr.size(), 0 /* match_start_point */,
+  -1 /* max_return_elements */, Results);
+  for (unsigned I = 1; I <= Matches; ++I) {
+const char *Match = Results.GetStringAtIndex(I);
+llvm::outs() << Match << "\n";
+llvm::outs().flush();
+  }
+}
+
 static void dumpSymbols(Debugger ) {
   for (const auto  : opts::symbols::InputFilenames) {
 ModuleSpec Spec{FileSpec(File, false)};
@@ -116,10 +139,13 @@
 
   auto Dbg = lldb_private::Debugger::CreateInstance();
 
-  if (opts::ModuleSubcommand)
+  if (opts::AutoCompleteSubCommand) {
+autocompleteCommand(*Dbg);
+  } else if (opts::ModuleSubcommand) {
 dumpModules(*Dbg);
-  else if (opts::SymbolsSubcommand)
+  } else if (opts::SymbolsSubcommand) {
 dumpSymbols(*Dbg);
+  }
 
   DebuggerLifetime->Terminate();
   return 0;


Index: lldb/tools/lldb-test/lldb-test.cpp
===
--- lldb/tools/lldb-test/lldb-test.cpp
+++ lldb/tools/lldb-test/lldb-test.cpp
@@ -15,6 +15,7 @@
 #include "lldb/Core/Module.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Initialization/SystemLifetimeManager.h"
+#include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Symbol/ClangASTContext.h"
 #include "lldb/Symbol/ClangASTImporter.h"
 #include "lldb/Utility/DataExtractor.h"
@@ -32,10 +33,16 @@
 using namespace llvm;
 
 namespace opts {
+cl::SubCommand AutoCompleteSubCommand("autocomplete", "Test LLDB autocomplete");
 cl::SubCommand ModuleSubcommand("module-sections",
 "Display LLDB Module Information");
 cl::SubCommand SymbolsSubcommand("symbols", "Dump symbols for an object file");
 
+namespace autocomplete {
+cl::list Input(cl::Positional, cl::desc("input patterns"),
+cl::Required, cl::sub(AutoCompleteSubCommand));
+}
+
 namespace module {
 cl::opt SectionContents("contents",
   cl::desc("Dump each section's contents"),
@@ -52,6 +59,22 @@
 
 static llvm::ManagedStatic DebuggerLifetime;
 
+static void autocompleteCommand(Debugger ) {
+  assert(opts::autocomplete::Input.length() == 1 && "Incorret number of args");
+  std::string InputStr = opts::autocomplete::Input[0];
+  lldb_private::StringList Results;
+  CommandInterpreter  = Dbg.GetCommandInterpreter();
+  unsigned Matches = CI.HandleCompletion(
+  InputStr.c_str(), InputStr.c_str() + InputStr.size(),
+  InputStr.c_str() + InputStr.size(), 0 /* match_start_point */,
+  -1 /* max_return_elements */, Results);
+  for (unsigned