From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 88802 invoked by alias); 3 Dec 2019 10:59:34 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 88774 invoked by uid 89); 3 Dec 2019 10:59:33 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.1 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS autolearn=ham version=3.3.1 spammy= X-HELO: mx1.osci.io Received: from polly.osci.io (HELO mx1.osci.io) (8.43.85.229) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Dec 2019 10:59:32 +0000 Received: by mx1.osci.io (Postfix, from userid 994) id 6428120562; Tue, 3 Dec 2019 05:59:30 -0500 (EST) Received: from gnutoolchain-gerrit.osci.io (gnutoolchain-gerrit.osci.io [IPv6:2620:52:3:1:5054:ff:fe06:16ca]) by mx1.osci.io (Postfix) with ESMTP id A88432032B; Tue, 3 Dec 2019 05:59:22 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by gnutoolchain-gerrit.osci.io (Postfix) with ESMTP id 8F21B2816F; Tue, 3 Dec 2019 05:59:22 -0500 (EST) X-Gerrit-PatchSet: 13 Date: Tue, 03 Dec 2019 10:59:00 -0000 From: "Andrew Burgess (Code Review)" To: gdb-patches@sourceware.org Cc: Simon Marchi , Joel Brobecker Auto-Submitted: auto-generated X-Gerrit-MessageType: comment Subject: [review v13] gdb/mi: Add -max-results parameter to some -symbol-info-* commands X-Gerrit-Change-Id: I90a28feb55b388fb46461a096c5db08b6b0bd427 X-Gerrit-Change-Number: 269 X-Gerrit-ChangeURL: X-Gerrit-Commit: d90e36fb0efd5c74478186643809ce2356cd465c In-Reply-To: References: X-Gerrit-Comment-Date: Tue, 3 Dec 2019 05:59:21 -0500 Reply-To: gnutoolchain-gerrit@osci.io MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Disposition: inline User-Agent: Gerrit/3.0.3-79-g83ff7f88f1 Content-Type: text/plain; charset=UTF-8 Message-Id: <20191203105922.8F21B2816F@gnutoolchain-gerrit.osci.io> X-SW-Source: 2019-12/txt/msg00090.txt.bz2 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 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 Gerrit-Reviewer: Andrew Burgess Gerrit-CC: Joel Brobecker Gerrit-CC: Simon Marchi Gerrit-Comment-Date: Tue, 03 Dec 2019 10:59:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Simon Marchi Gerrit-MessageType: comment