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

Tue Dec 3 16:50:00 GMT 2019

Simon Marchi has posted comments on this change.

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

Patch Set 13:

(3 comments)

> Patch Set 13:
> 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 :)

No worries, it's just a simple mistake of pushing the wrong commit, it happens.

The only remaining bit is about the code to parse the parameter value.

| --- /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?

Yes, since we don't pass an explicit comparator in the set template,
it uses std::less, which uses the operator< of the element type
(symbol_search).  But then std::sort also sorts elements using
operator<, by default.  So the order of the set will already be the
same as the order of the vector after passing it through std::sort.

|  
|    /* 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 ())
|      {
| --- gdb/mi/mi-symbol-cmds.c
| +++ gdb/mi/mi-symbol-cmds.c
| @@ -168,4 +170,19 @@ mi_symbol_info (enum search_domain kind, const char *name_regexp,
|  
| +/* Helper to parse the option text from an -max-results argument and return
| +   the parsed value.  If the text can't be parsed then an error is thrown.  */
| +
| +static size_t
| +parse_max_results_option (char *arg)
| +{
| +  char *ptr = arg;
| +  long long val = strtoll (arg, &ptr, 10);
| +  if (arg == ptr || val > SIZE_MAX || val < 0)

PS13, Line 179:

This lets us pass:

 --max-results 1a

which is interpreted 1.

Did you check elsewhere in the MI code to see if there was already
some helper to do this?  It's not the first time we need to parse an
argument as an integer, so I'm surprised you have to write this from
scratch.

| +    error (_("invalid value for --max-results argument"));
| +  size_t max_results = (size_t) val;
| +
| +  return max_results;
| +}
| +
|  /* Helper for mi_cmd_symbol_info_{functions,variables} - depending on KIND.
|     Processes command line options from ARGV and ARGC.  */
|  

-- 
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 16:50:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Andrew Burgess <andrew.burgess@embecosm.com>
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment