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

["Andrew Burgess (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>]

Andrew Burgess has posted comments on this change.

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


Patch Set 13:

(2 comments)

> Patch Set 11:
> 
> (2 comments)
> 
> I'm getting a bunch of compilation errors that I am surprised you don't get:
> 
>  /home/simark/src/binutils-gdb/gdb/mi/mi-symbol-cmds.c: In function ‘size_t parse_max_results_option(const char*)’:
>  /home/simark/src/binutils-gdb/gdb/mi/mi-symbol-cmds.c:177:15: error: invalid conversion from ‘const char*’ to ‘char*’ [-fpermissive]
>    177 |   char *ptr = arg;
>        |               ^~~
>        |               |
>        |               const char*
>  /home/simark/src/binutils-gdb/gdb/mi/mi-symbol-cmds.c: In function ‘void mi_info_functions_or_variables(search_domain, char**, int)’:
>  /home/simark/src/binutils-gdb/gdb/mi/mi-symbol-cmds.c:233:29: error: assignment of function ‘size_t parse_max_results_option(const char*)’
>    233 |    parse_max_results_option = parse_max_results_option (oarg);
>        |    ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  /home/simark/src/binutils-gdb/gdb/mi/mi-symbol-cmds.c: In function ‘void mi_cmd_symbol_info_modules(const char*, char**, int)’:
>  /home/simark/src/binutils-gdb/gdb/mi/mi-symbol-cmds.c:436:29: error: assignment of function ‘size_t parse_max_results_option(const char*)’
>    436 |    parse_max_results_option = parse_max_results_option (oarg);
>        |    ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  /home/simark/src/binutils-gdb/gdb/mi/mi-symbol-cmds.c: In function ‘void mi_cmd_symbol_info_types(const char*, char**, int)’:
>  /home/simark/src/binutils-gdb/gdb/mi/mi-symbol-cmds.c:478:29: error: assignment of function ‘size_t parse_max_results_option(const char*)’
>    478 |    parse_max_results_option = parse_max_results_option (oarg);
>        |    ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This is really embarrassing :(  At some point last night I got mixed up between two development versions of GDB I was working on, and started building / testing a completely different version of GDB thinking it was this one.  I'm pretty sure it can only have been for the last ~1 hour of work, but still, I have no excuses and I'm sorry.

I've updated the patch, addressed the compilation issue and the other items you pointed out.  Sorry for wasting your time with the previously bad commits.  I'm sure I tested the right thing this time :)

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +17,19 @@ these commands is slightly harder.
| +There's currently no mechanism for the user of these commands to know
| +if the result list has been truncated if you get back the maximum
| +number of results, so if there are exactly 10 functions and you call
| +'-symbol-info-functions --max-results 10' the reply would appear no
| +different than if you had 20 functions and called with a max of 10.
| +Right now, if you get back the maximum then you should assume that
| +there might be more results available.
| +
| +One other thing to note is that the global_symbol_searcher::search by
| +default returns UINT_MAX results, there's no longer a mechanism to

PS11, Line 26:

Done.

| +return an unlimited number of results, though hopefully this will not
| +be a huge issue.
| +
| +gdb/ChangeLog:
| +
| +	* mi/mi-symbol-cmds.c (mi_symbol_info): Take extra parameter, and
| +	add it into the search spec.
| +	(parse_max_results_option): New function.
| +	(mi_info_functions_or_variables): Parse -max-results flag and pass
| --- gdb/symtab.c
| +++ gdb/symtab.c
| @@ -4737,9 +4735,19 @@ global_symbol_searcher::search () const
| +	 RESULT_SET set.  Use a set here so that we can easily detect
| +	 duplicates as we go, and can therefore track how many unique
| +	 matches we have found so far.  */
| +      if (!add_matching_symbols (objfile, preg, treg, &result_set))
| +	break;
| +    }
| +
| +  /* Convert the result set into a sorted result list.  */
| +  std::vector<symbol_search> result (result_set.begin (), result_set.end ());
| +  std::sort (result.begin (), result.end ());

PS11, Line 4744:

You are correct that the standard defines a set to be sorted, but my
understanding was that we did provide a mechanism for sorting the
objects in the set by providing `operator<` (and also `operator==`) on
`struct symbol_search`.  The results certainly always appear to be
sorted as I'd expect - am I misunderstanding something here?

|  
|    /* If there are no debug symbols, then add matching minsyms.  But if the
|       user wants to see symbols matching a type regexp, then never give a
|       minimal symbol, as we assume that a minimal symbol does not have a
|       type.  */
|    if ((found_msymbol || (filenames.empty () && m_kind == VARIABLES_DOMAIN))
|        && !m_exclude_minsyms
|        && !treg.has_value ())
|      {

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I90a28feb55b388fb46461a096c5db08b6b0bd427
Gerrit-Change-Number: 269
Gerrit-PatchSet: 13
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: Tue, 03 Dec 2019 10:59:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment