[Lldb-commits] [PATCH] D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms

[Jim Ingham via lldb-commits]

> On Jan 7, 2021, at 2:29 PM, David Blaikie via Phabricator via lldb-commits <lldb-commits at lists.llvm.org> wrote:
> 
> dblaikie added a comment.
> 
> In D94063#2485271 <https://reviews.llvm.org/D94063#2485271>, @labath wrote:
> 
>> In D94063#2483546 <https://reviews.llvm.org/D94063#2483546>, @dblaikie wrote:
>> 
>>> If it's better to write it using C++ source and custom clang flags I can do that instead (it'll be an -mllvm flag - looks like there's one other test that does that: `lldb/test/API/lang/objc/forward-decl/TestForwardDecl.py:            dict(CFLAGS_EXTRAS="-dwarf-version=5 -mllvm -accel-tables=Dwarf"))`) - means the test will be a bit more convoluted to tickle the subprogram ranges, but not much - just takes two functions+function-sections.
>> 
>> I certainly wouldn't want to drop the existing test.
> 
> Ah, what's the tradeoff between the different test types here?

This is my take (this has been a contentious issue so I'm sure there are other takes...):

The "Shell" tests use pattern matching against the lldb command line output.  They are useful for testing the details of the command interaction.  You can also do that using pexpect in the API tests, but the Python 2.7 version of pexpect seemed really flakey so we switched to shell tests for this sort of thing.

Because you are matching against text output that isn't API, they are less stable.  For instance if we changed anything in the "break set" output, your test would fail(*).  And because you are picking details out of that text, the tests are less precise.  You either have to match more of the command line than you are actually testing for, which isn't a good practice, or you run the risk of finding the text you were looking for in a directory path or other unrelated part of the output.  Also they are harder to debug if you can't reproduce the failure locally, since it isn't easy to add internal checks/output to the test to try hypotheses.  Whenever I have run into failures of this sort the first thing I do is convert the test to an API test...

But the main benefit of the "Shell" tests is that you can write tests without having to know Python or learn the lldb Python API's.  And if you are coming from clang you already know how FileCheck tests work, so that's a bonus.  I think it's legit to require that folks actually working on lldb learn the SB API's.  But we were persuaded that it wasn't fair to impose that on people not working on lldb, and yet such folks do sometimes need to write tests for lldb...  So for simple tests, the Shell tests are an okay option.  But really, there's nothing you can do in a Shell test that you can't do in an API test.

The "API" tests use the Python SB API's - though they also have the ability to run commands and do expect type checks on the output so for single commands they work much as the shell tests do (there's even a FileCheck style assert IIRC).  They are a little more verbose than shell tests (though we've reduced the boilerplate significantly over the years).  And of course you have to know the SB API's.  But for instance, if you wanted to know that a breakpoint was set on line 5 of foo.c, you can set the breakpoint, then ask the resultant SBBreakpoint object what it's file & line numbers were directly.  So once you've gotten familiar with the setup, IMO you can write much higher quality tests with the API tests.


Jim

(*) I am personally not at all in favor of the Shell tests, but that's in part because back in the day I was asked to make a simple and useful change to the output of the gdb "break" command but then righting the gdb testsuite - which is all based on expecting the results of various gdb commands - was so tedious that we ended up dropping the change instead.  I don't want to get to that place with lldb, but the hope is that as long as we mostly write API tests, we can avoid encumbering the current command outputs too heavily...

Jim

> 
>> However, it could be useful to have c++ test too. This one could feature a more complicated executable, and be more open-ended/exploratory and test end-to-end functionality (including compiler integration), instead of a targeted "did we parse DW_AT_ranges correctly" regression test. Then this test could go into the `API` test category, as we have the ability to run those kinds of tests against different compilers.
> 
> Does this include support for custom compiler flags (it'd currently take a non-official/internal-only llvm flag to create the DW_AT_ranges on a subprogram that I have in mind, for instance)?
> 
>> However, all of that is strictly optional.
> 
> I'll consider it for a separate commit.
> 
> 
> Repository:
>  rG LLVM Github Monorepo
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D94063/new/
> 
> https://reviews.llvm.org/D94063
> 
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits