[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<x86AssemblyInspectionEngine> 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, Jason Molenda <jmolenda at apple.com> wrote:
> 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 <zturner at google.com> 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 <jmolenda at apple.com> wrote:
>>
>>
>> > On Feb 12, 2018, at 12:59 PM, Zachary Turner <zturner at google.com> wrote:
>> >
>> >
>> >
>> > On Mon, Feb 12, 2018 at 12:52 PM Jason Molenda <jmolenda at apple.com> wrote:
>> >>
>> >>
>> >> > On Feb 12, 2018, at 12:48 PM, Zachary Turner via Phabricator <reviews at 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
>