Page MenuHomePhabricator

lldb-test symbols: Add ability to do name-based lookup
ClosedPublic

Authored by labath on May 1 2018, 8:41 AM.

Details

Summary

lldb-test already had the ability to dump all symbol information in a
module. This is interesting, but it can be too verbose, and it also does
not use the same APIs that lldb uses to query symbol information. The
last part is interesting to me now, because I am about to add DWARF v5
debug_names support, which needs to implement these APIs.

This patch adds a set of arguments to lldb-test, which modify it's
behavior from dumping all symbols to dumping only the requested
information:

  • --lookup={function,namespace,type,variable} - search for the given kind of objects.
  • --name - the name to search for.
  • --regex - whether to treat the "name" as a regular expression. This is not available for all lookup types (we do not have the required APIs for namespaces and types).
  • --context - specifies the context, which can be used to restrict the search. This argument takes a variable name (which must be defined and be unique), and we then use the context that this variable is defined in as the search context.
  • --function-flags={auto,full,base,method,selector} - a set of flags to further restrict the search for function symbols.

Together, these flags and their combinations cover the main SymbolFile
entry points which I will need to modify for the accelerator table
support, and so I plan to do most of the regression testing this way.
(I've also found this a useful tool for exploration of what the given
APIs are supposed to do.)

I add a couple of tests to demonstrate the usage of the usage of the
various options, and also an xfailed test which demonstrates a bug I
found while playing with this. The only requirement for these tests is
the presence of lld -- the should run on any platform which is able to
build lldb.

These tests use c++ code as input, but this isn't a requirement. It is also
possible to use IR, assembly or json to create the test module.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.May 1 2018, 8:41 AM
zturner added inline comments.May 1 2018, 9:49 AM
lit/CMakeLists.txt
32 ↗(On Diff #144731)

Note that this depends on lld having had its own CMakeLists.txt file processed before LLDB's. I think this happens by accident since we process them in alphabetical order, but it's something to keep in mind.

lit/SymbolFile/DWARF/basic-function-lookup.cpp
23 ↗(On Diff #144731)

I personally find it a little confusing to have the check lines interspersed with the source code here as we do in this file. I think it's easier to read if all check labels are grouped together so that you get a general idea of what the output of the tool looks like. I don't feel too strongly though, so up to you.

A few nits/stylistic comments

tools/lldb-test/lldb-test.cpp
86 ↗(On Diff #144731)

It's a bit unfortunate that these argument names are different from the ones we have in dwarfdump. For instance, we use -lookup for addresses and -find for the accelerator tables. (https://llvm.org/docs/CommandGuide/llvm-dwarfdump.html)

Would you mind using the same names here?

244 ↗(On Diff #144731)

Empty()?

260 ↗(On Diff #144731)

You can use WithColor::error() from Support :-)

303 ↗(On Diff #144731)

This feels a bit C89-like; personally I prefer to initialize my variables as close to their use as possible.

labath updated this revision to Diff 144890.May 2 2018, 8:08 AM
labath marked 3 inline comments as done.

Address review comments.

lit/SymbolFile/DWARF/basic-function-lookup.cpp
23 ↗(On Diff #144731)

I agree it looks better when it's grouped. I did it this way originally, because the first tests I was writing were the variable & type tests, and there I used `@LINE` thingies to match the declarations (as the output stands now, the declaration line is the only sufficiently deterministic property I can use to distinguish two variables with the same name). I have regrouped the checks which don't depend on the line numbers.

tools/lldb-test/lldb-test.cpp
86 ↗(On Diff #144731)

Hmm... I agree we should avoid confusion where possible. So using lookup is out.
How about renaming it to find. It's nice that it matches the underlying lldb functions, but it is also a bit inconsistent because llvm-dwarfdump uses it to search through the accelerator tables. However we don't have the distinction between searching using accelerator tables and not, and I don't think we will need it (the idea here was to make this search the same way that lldb would do).

The alternative is to rename this argument to something completely different (--search, --name-type, ...)

244 ↗(On Diff #144731)

There isn't such a function, but having it sounds like a good idea, so I added one. :)

JDevlieghere added inline comments.May 2 2018, 9:05 AM
tools/lldb-test/lldb-test.cpp
463 ↗(On Diff #144890)

If we can wrap this in a RAII object we could have this tool return a non-zero exit code in case of an error.

86 ↗(On Diff #144731)

Alright, find sounds okay.

260 ↗(On Diff #144731)

I probably should've noticed this earlier, but since we return after all these errors maybe I'd be nice to have the function return an error? Doesn't have to be in this diff though.

labath updated this revision to Diff 144904.May 2 2018, 11:00 AM
labath marked 4 inline comments as done.

More review feedback.

labath added inline comments.May 2 2018, 11:00 AM
tools/lldb-test/lldb-test.cpp
463 ↗(On Diff #144890)

Done. I've made the individual functions return an int, and had main forward that. The reason I am not having the functions return an Error or like is that the functions generally do the same thing over several inputs, and the errors that happen when processing one of the inputs should be printed next to that input and not deferred until the end. So, the functions just accumulate a flag saying whether they encountered any errors and then return that.

260 ↗(On Diff #144731)

I've handled this a bit differently. Since this will be the same regex for each module we dump, I've done the validation up-front (next to the other checks), and changed this into an assertion.

JDevlieghere accepted this revision.May 3 2018, 3:00 AM

A few more nits but otherwise this LGTM.

tools/lldb-test/lldb-test.cpp
463 ↗(On Diff #144890)

Alright, works for me!

481 ↗(On Diff #144904)

formatting

484 ↗(On Diff #144904)

WithColor:error()? :-)

This revision is now accepted and ready to land.May 3 2018, 3:00 AM
This revision was automatically updated to reflect the committed changes.
labath marked 2 inline comments as done.