[review v8] gdb/mi: Add -max-results parameter to some -symbol-info-* commands

[Andrew Burgess (Code Review)]

Andrew Burgess has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/269
......................................................................


Patch Set 8:

> Patch Set 7:
> 
> (8 comments)
> 
> I think the way you refactored it is very nice, it breaks the thing up in smaller operations, easier to understand.  But it's difficult to figure out what's old stuff and what's new features.  Do you think you could split this in separate patches, one that does the refactoring (split in multiple helper methods), that would have no user visible changes, and then the one that adds --max-results?
> 
> On the general design of the patch:
> 
> If I understand correctly, the --max-results value doesn't bound the expansion of symtabs without one objfile.  When we have started to expand the matching symtabs of an objfile, there's no way to stop it unless it has gone through the whole objfile, right?  So let's say you do:
> 
>  -symbol-info-functions --max-results 2 a
> 
> we'll still proceed to expand all symtabs of the objfile that have a symbol containing `a`, which may take a very long time.  And once that's done, we'll pick the first 2 of all these symbols.
> 
> I think it's a reasonable behavior for this patch, it's better to get something working first and incrementally improve.
> 
> If we think about how to optimize later: the expand_symtabs_matching method of quick_symbol_functions already takes an "expansion_notify" callback, that is called after each individual symtab expansion.  I could see that callback returning a bool, to indicate whether to continue or stop expanding symtabs.  We would add the matching symbols from the symtab to our result set in that callback, so we could stop the expansions once we have reached our limit. WDYT?

The problem that worried me most here was the difference between when something needs expanding, and when something is already expanded.

So right now we expand matching symtabs within an objfile, which might do nothing is the symtabs are already expanded, then we rewalk the symtabs and add matching symbols.

If we use the current "just expanded symtab" callback then this only triggers if something gets expanded, if it was already expanded then I don't think we see the callback, so we'd then need to walk all the symtabs again to catch any already expanded symtabs.

This will create strange ordering artifacts where results will appear in a different order the first call vs later calls.

What we really need is a "visit matching symtabs" type function, that applies the same logic as expand, but triggers the callback even for symtabs that are already expanded.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I90a28feb55b388fb46461a096c5db08b6b0bd427
Gerrit-Change-Number: 269
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Mon, 02 Dec 2019 17:57:39 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment