Re: [Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion
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
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 Turnerwrote: > > 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
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 Molendawrote: > > > > 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
> On Feb 12, 2018, at 12:59 PM, Zachary Turnerwrote: > > > > 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
On Mon, Feb 12, 2018 at 12:52 PM Jason Molendawrote: > > > > 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
> 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
Re: [Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion
On Wed, Feb 7, 2018 at 8:20 PM, Adrian Prantlwrote: > > > > 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
> On Feb 7, 2018, at 6:40 PM, Zachary Turnerwrote: > 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
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
Re: [Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion
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 Turnerwrote: > 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
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