Page MenuHomePhabricator

Add support for "source info" and use it to fix MI's -symbol-list-lines.
ClosedPublic

Authored by dawn on Dec 16 2015, 3:09 PM.

Details

Summary

This patch adds support the command 'source info' as follows:

(lldb) help source info
     Display source line information (as specified) based on the current executable's
     debug info.

Syntax: source info <cmd-options>

Command Options Usage:
  source info [-c <count>] [-s <shlib-name>] [-f <filename>] [-l <linenum>] [-e <linenum>]
  source info [-c <count>] [-s <shlib-name>] [-n <symbol>]
  source info [-c <count>] [-a <address-expression>]

       -a <address-expression> ( --address <address-expression> )
            Lookup the address and display the source information for the corresponding
            file and line.

       -c <count> ( --count <count> )
            The number of line entries to display.

       -e <linenum> ( --end-line <linenum> )
            The line number at which to stop displaying lines.

       -f <filename> ( --file <filename> )
            The file from which to display source.

       -l <linenum> ( --line <linenum> )
            The line number at which to start the displaying lines.

       -n <symbol> ( --name <symbol> )
            The name of a function whose source to display.

       -s <shlib-name> ( --shlib <shlib-name> )
            Look up the source in the given module or shared library (can be specified
            more than once).

For example:

(lldb) source info --file x.h
Lines for file x.h in compilation unit x.cpp in `x
[0x0000000100000d00-0x0000000100000d10): /Users/dawn/tmp/./x.h:10
[0x0000000100000d10-0x0000000100000d1b): /Users/dawn/tmp/./x.h:10

The new options are used to fix the MI command:

-symbol-list-lines <file>

which didn't work for header files because it called:

target modules dump line-table <file>

which only dumps line tables for a compilation unit.

The patch also fixes a bug in the error reporting when no files were supplied to the command. Previously you'd get:

(lldb) target modules dump line-table
error:
Syntax:
error: no source filenames matched any command arguments

Now you get:

error: file option must be specified.

Diff Detail

Repository
rL LLVM

Event Timeline

dawn updated this revision to Diff 43071.Dec 16 2015, 3:09 PM
dawn retitled this revision from to Add "target modules dump line-entries <file>" command and use it to fix MI's -symbol-list-lines..
dawn updated this object.
dawn added reviewers: clayborg, ki.stfu, abidh.
dawn set the repository for this revision to rL LLVM.
dawn added a subscriber: lldb-commits.
dawn updated this revision to Diff 43076.Dec 16 2015, 3:42 PM

Updated patch to include changes to TestHelp.py.

jingham requested changes to this revision.Dec 16 2015, 4:00 PM
jingham added a reviewer: jingham.
jingham added a subscriber: jingham.

I don't think this is sufficiently different from "target modules dump line-tables" to warrant a separate command. This seems more like a candidate for a flag to "target modules dump line-tables" like --search-for-inlines or something. That would keep the command surface more manageable and also make the option more discoverable, since you can get both variants in one place.

This revision now requires changes to proceed.Dec 16 2015, 4:00 PM
dawn added a comment.EditedDec 16 2015, 4:36 PM

I don't think this is sufficiently different from "target modules dump line-tables" to warrant a separate command. This seems more like a candidate for a flag to "target modules dump line-tables" like --search-for-inlines or something. That would keep the command surface more manageable and also make the option more discoverable, since you can get both variants in one place.

I first thought about adding it as an option to the line-table but decided against it because the information provided by each are just too different.

Dumping the tables gives you raw output from the CU's DWARF .debug_line entries, including end sequences, all files in the compilation unit, etc.:

(lldb) target modules du line-table x.cpp
Line table for /Users/dawn/tmp/x.cpp in `x
0x0000000100000aa0: /Users/dawn/tmp/x.cpp:12
0x0000000100000ab6: /Users/dawn/tmp/x.cpp:14
0x0000000100000af2: /Users/dawn/tmp/x.cpp:16
0x0000000100000afa: /Users/dawn/tmp/x.cpp:17
[...]
0x0000000100000cc0: /Users/dawn/tmp/x.cpp:6
0x0000000100000ccb: /Users/dawn/tmp/x.cpp:8
[...]
0x0000000100000d20: /Users/dawn/tmp/./x.h:10
0x0000000100000d2c: /Users/dawn/tmp/./x.h:10
0x0000000100000d3b: /Users/dawn/tmp/./x.h:10

Whereas dumping the line entries gives you entries after they've been interpreted and sorted by lldb, and will not include any files other than the one(s) asked for:

(lldb) target modules du line-entries x.h
Lines for file x.h in compilation unit x.cpp in `x
[0x0000000100000d00-0x0000000100000d10): /Users/dawn/tmp/./x.h:10
[0x0000000100000d10-0x0000000100000d1b): /Users/dawn/tmp/./x.h:10
[0x0000000100000d20-0x0000000100000d2c): /Users/dawn/tmp/./x.h:10
[0x0000000100000d2c-0x0000000100000d3b): /Users/dawn/tmp/./x.h:10

(lldb) target modules du line-entries x.cpp
Lines for file x.cpp in compilation unit x.cpp in `x
[0x0000000100000cc0-0x0000000100000ccb): /Users/dawn/tmp/x.cpp:6
[0x0000000100000ccb-0x0000000100000cdd): /Users/dawn/tmp/x.cpp:8
[0x0000000100000cdd-0x0000000100000cf1): /Users/dawn/tmp/x.cpp:9
[0x0000000100000aa0-0x0000000100000ab6): /Users/dawn/tmp/x.cpp:12
[0x0000000100000ab6-0x0000000100000af2): /Users/dawn/tmp/x.cpp:14
[...]

I'm fine with combining the two if we can find an interface that makes sense. 'line-table' is not appropriate for the new command, and '--search-for-inlines' doesn't convey the differences. We could change 'line-table' to 'lines', but then what?

Okay. So it seems like there are a couple of orthogonal differences here.

  1. The current behavior requires that the the source file you pass in be a compile unit. We don't search inside the line tables. So for instance

target modules dump line-tables foo.h

would currently not dump anything.

  1. We dump the whole line table for the given file, not just the lines from that file.
  1. We show raw vrs. cooked line entries.

So it seems like your change is adding three flags

--compile-unit-only --only-explicit-matches --show-raw

or something. The current behavior is:

--compile-unit-only true --only-explicit-matches false --show-raw true

and your command does:

--compile-unit-only false --only-explicit-matches true --show-raw false

Does that seem right. If so, then we have to decide whether it is worth the effort to be able to tweak these three independently.

dawn added a comment.Dec 16 2015, 6:02 PM

So it seems like your change is adding three flags

--compile-unit-only --only-explicit-matches --show-raw

or something. The current behavior is:

--compile-unit-only true --only-explicit-matches false --show-raw true

and your command does:

--compile-unit-only false --only-explicit-matches true --show-raw false

Does that seem right.

Yup.

If so, then we have to decide whether it is worth the effort to be able to tweak these three independently.

I think these flags only make sense if you could tweak them independently, but to support something like:

--compile-unit-only false --only-explicit-matches true --show-raw *true*

would be a major pain - it would mean adding a lot of extra code and interfaces for something that would only be used by this one command.

I think it's better to just have both:

target modules line-tables <cu>
target modules line-entries <file>

and if we find a need for any other combinations of the options you proposed, we can merge them at that time.

Thoughts?

Provided we error out appropriately I don't think we need to support all possible combinations of options. I'd happier doing it by options and not supporting the combinations that aren't worth the effort. Then if anybody really needs one of the unsupported ones, the path to adding the support will be straight-forward, even if tedious.

The options seem to me to actually explain what the variants do, whereas the two names are not going to be meaningful enough to most folks to do more than cause confusion (and force me to have to type "line-t" or "line-e".)

dawn added a comment.EditedDec 16 2015, 6:18 PM

I'm only going to support the combinations:

--compile-unit-only true --only-explicit-matches false --show-raw true
--compile-unit-only false --only-explicit-matches true --show-raw false

OK?

(and force me to have to type "line-t" or "line-e".)

or force me to have to type "line-table --compile-unit-only false --only-explicit-matches true --show-raw false"?! (ugh)

That seems fine for now.

Note, if you use the OptionGroupOptions way of defining the new options for your command, then you can call OptionsSeen to tell you which options were actually provided by the user, so for extra credit, if --compile-unit-only is true and no other options were provided, you can set the other options to the correct values, and ditto when --compile-unit-only is false. That way for now you would only have to provide --compile-unit-only, and the others would be set to the supported combo, but if they provided an unsupported combo you could report the error appropriately.

You can also get the same effect in the table driven way of defining options by just manually marking which options have been seen in your command's SetOptionValue (just remember to clear this info in the OptionParsingStarting method.

Jim

dawn updated this revision to Diff 43103.Dec 16 2015, 11:23 PM
dawn retitled this revision from Add "target modules dump line-entries <file>" command and use it to fix MI's -symbol-list-lines. to Enhance "target modules dump line <file>" and use it to fix MI's -symbol-list-lines..
dawn updated this object.
dawn edited edge metadata.
ki.stfu requested changes to this revision.Dec 17 2015, 5:04 AM
ki.stfu edited edge metadata.

clang-format your changes please (there are many deviations from the coding style)

I'll check it on Linux and say if everything is OK.

source/Commands/CommandObjectTarget.cpp
1510

nit: fix indentation

1536–1537

use size_t

1541–1542

Isn't it always false?

1543

You don't need a raw pointer here, just use cu_sp.get() on line #1587

1576

!cu_header_printed

1594–1596

combine it together:

cu->FindLineEntry(start_idx + 1, ...)
2689

Please use the most popular pattern:

error.SetErrorStringWithFormat ("unrecognized option '%c'", short_option);
tools/lldb-mi/MICmdCmdSymbol.cpp
85

Could you use long-options here?

This revision now requires changes to proceed.Dec 17 2015, 5:04 AM
ki.stfu added inline comments.Dec 17 2015, 5:11 AM
packages/Python/lldbsuite/test/tools/lldb-mi/symbol/symbol_list_lines_inline_test.h
20

and could you pick it up at 1 line above?

dawn marked 6 inline comments as done.Dec 17 2015, 10:25 AM

clang-format your changes please (there are many deviations from the coding style)

I've made some fixes even though the code is no longer consistent with the rest of the file. Alas. I would love to run all of lldb through clang-format, but as we've seen, there are several options which still need to be added before we can do that (mostly relating to the formatting function declarations).

source/Commands/CommandObjectTarget.cpp
1510

This is the original code, and is consistent with the coding style of the rest of the file. It bothers me too that lldb is so schizophrenic about its coding style, but unless we fix all of lldb, I think it's best to just try and follow the style of the code around you.

Does anyone else have an opinion about this? I'll go ahead and change it, only because I see I that the new code I added didn't follow this style (oops).

1541–1542

Code elsewhere checks for it, so I assume there are cases when cu_sp can be null. Example in source/Core/SearchFilter.cpp:

for (size_t i = 0; i < num_comp_units; i++)
{
    CompUnitSP cu_sp (module_sp->GetCompileUnitAtIndex (i));
    if (cu_sp)
    {

Better safe than sorry.

1543

There are 5 uses of cu in this code, so I think it's cleaner to have a variable.

1594–1596

I'd love to have a guideline as to when to wrap lines - lldb is all over the place about this. I've tended to try to keep lines to 100.

dawn updated this revision to Diff 43152.Dec 17 2015, 10:27 AM
dawn edited edge metadata.
dawn marked an inline comment as done.

Updated patch with suggestions from Ilia's review (thanks Ilia!).

clayborg requested changes to this revision.Dec 17 2015, 10:37 AM
clayborg edited edge metadata.

Man those options are complex. I really would like to see something more simple:

"--raw" dumps the line entries as you have mentioned
"--no-inlines" would omit any inline entries
"--module <module>" would only find any source file arguments in the module that is specified by full or basename (this option can be specified more than once)

The arguments are still the compile unit names (full or basenames). If there are no arguments, dump all line tables from all modules. If --module is specified, then dump all compile units from any modules that are specified.

Am I missing something?

This revision now requires changes to proceed.Dec 17 2015, 10:37 AM

Ok, so I talked with Jim on this and this is what we came up with. I would like to see no changes made to the "target modules dump line-table" command. It is dumping complete line tables. This command is not for "find any line entries that match this file anywhere including inline references". We should make this new command into "source info". Its syntax will be:

source info --file <file> [--line <line>] [--module <module]

line is optional. This job of this command is to find all line entries that match the file and optional line and optionally look only in the modules that might have been specified (might be more than one). --file and --line can only be specified once, --module can be specified multiple times.

The functionality that you reproduced in DumpFileLines() should be replaced. This is the code that looks through all compile units and finds the line entries that is contained in:

else if (compile_unit_only == false && explicit_matches_only == true && show_raw==false)

should be replaced with a call to:

uint32_t
CompileUnit::ResolveSymbolContext
(
    const FileSpec& file_spec,
    uint32_t line,
    bool check_inlines,
    bool exact,
    uint32_t resolve_scope,
    SymbolContextList &sc_list
)

As this already does exactly what you are doing. We will need to modify CompileUnit::ResolveSymbolContext() to accept the "line" argument being zero. It will return an "sc_list" of all matches, inside each symbol context will be the LineEntry that you need.

I do want to apologize to Dawn and Jim for not having chimed in earlier and causing Dawn some extra work.

ki.stfu added inline comments.Dec 17 2015, 12:28 PM
source/Commands/CommandObjectTarget.cpp
1536–1537

use it on line #1536 too

1543

That's no problem. Just replace s/cu/cu_sp/ on lines #1544, #1565, #1580, #1594, and then use cu_sp.get() on line #1586.

1594–1596

How about that?

start_idx = cu->FindLineEntry(start_idx + 1,
                              line,
                              &cu_file_spec,
                              /*exact=*/true,
                              &line_entry);
2689

nit: you forgot the dot

ki.stfu added inline comments.Dec 17 2015, 1:01 PM
tools/lldb-mi/MICmdCmdSymbol.cpp
226

Is strWantFile needed? Seems it's an auxiliary variable.

Dawn, if you don't understand what I asked you to do, let me know and I can go and do the "source info" command for you, then you can modify this patch to just use the command I added. But it should be simple to just revert and changes to the original line table dumping command, and move the contents over to the new "source info" command and just make it do what you wanted it to do.

Note that you won't need any of the options that you had added to the other command becuase the "source info" command will do what you wanted in the first place: find all line entries where a file and optional line is mentioned in any line tables.

dawn added a comment.Dec 21 2015, 7:07 PM

Dawn, if you don't understand what I asked you to do,

Sorry for late reply - been recovering from minor surgery. I have a new patch, but I've not yet gone through all of Ilia's additional comments - will probably go ahead and upload anyway just to give you all a glimpse, as I fear there will be a few more rounds before this patch is ready.

As this already does exactly what you are doing. We will need to modify CompileUnit::ResolveSymbolContext() to accept the "line" argument being zero. It will return an "sc_list" of all matches, inside each symbol context will be the LineEntry that you need.

I didn't do this because I added support for additional options which were supported by 'source list', and needed access to the options during the loop which collects the lines. I hope you'll agree that this was the right approach after seeing the patch.

dawn marked 4 inline comments as done.Dec 21 2015, 8:03 PM

Replies to Ilia's comments...

source/Commands/CommandObjectTarget.cpp
1543

this no longer applies to the most recent patch.

2689

did I forget a dot?

tools/lldb-mi/MICmdCmdSymbol.cpp
226

It's no longer needed with the new code, because you'll never get a file that isn't what was asked for.

dawn updated this revision to Diff 43429.Dec 21 2015, 8:10 PM
dawn retitled this revision from Enhance "target modules dump line <file>" and use it to fix MI's -symbol-list-lines. to Add support for "source info" and use it to fix MI's -symbol-list-lines..
dawn updated this object.
dawn edited edge metadata.
dawn marked 2 inline comments as done.
dawn added a comment.Jan 4 2016, 7:53 PM

Hopefully folks are back from the holidays by now - can someone give this a look? Thanks in advance...

clayborg accepted this revision.Jan 5 2016, 10:00 AM
clayborg edited edge metadata.

So I am going to let this through because you have done all we asked and I really like the "source info" command!

But in the future when adding things for MI, please add an API in the lldb::SB layer to do what you need. Text scraping is really not the solution we should be adding into the code. Other people using MI are going to see this kind of code and think it is what they should do when implementing/fixing new/existing MI commands.

We have an API for a debugger that we are trying to make with the lldb::SB layer, so we should use it and stop ANY form of text scraping in the future. I think an API'ized version of what you are requesting here -- all line table entries for a given source file -- should be available from SBTarget and a future modification to MICmdCmdSymbol.cpp should switch over to using it.

dawn added a comment.Jan 5 2016, 10:47 AM

... in the future when adding things for MI, please add an API in the lldb::SB layer to do what you need.

Thanks for not asking me to do that as part of this patch.

Text scraping is really not the solution we should be adding into the code. Other people using MI are going to see this kind of code and think it is what they should do when implementing/fixing new/existing MI commands.

I agree that MI should have used the SB layer - did anyone ask the authors about that? Because as you say, the text scraping has now become a precedent for MI commands.

We have an API for a debugger that we are trying to make with the lldb::SB layer, so we should use it and stop ANY form of text scraping in the future. I think an API'ized version of what you are requesting here -- all line table entries for a given source file -- should be available from SBTarget and a future modification to MICmdCmdSymbol.cpp should switch over to using it.

+100. Except that I think it should only apply to new code - bug fixes like this shouldn't be held back because of preexisting code.

Rewriting lldb-mi to use the SB layer would be a great project for an intern or someone who wants to get familiar with lldb. We should keep that in mind if/when folks on lldb-dev ask about how they can help out.

This revision was automatically updated to reflect the committed changes.
dawn added a comment.Jan 5 2016, 1:06 PM

I forgot to commit updated changes to the tests:

MiSymbolTestCase.test_lldbmi_symbol_list_lines_file
HelpCommandTestCase.test_help_image_du_line_should_work

so they are failing now, but svn at llvm.org appears to be down at the moment.
I'll commit those changes as soon as svn is back up...

dawn added a comment.EditedJan 5 2016, 2:28 PM

Amended patch was posted to http://reviews.llvm.org/D15904 - hopefully someone who can get to llvm.org will commit.

dawn added a comment.Jan 5 2016, 4:12 PM

Amended patch was committed in svn r256877 (svn at lldb.org is back up now)

labath added a subscriber: labath.Jan 6 2016, 2:01 AM

Hi, the new check you have added to TestMiSymbol fails on linux: all the source lines are listed twice for some reason. I don't know if this is expected behavior (and the test needs to be fixed) or it's an bug (and the code needs to be fixed), but I have marked the test as XFAIL no pacify the buildbots.