From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 58121 invoked by alias); 3 Dec 2019 16:50:19 -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 58112 invoked by uid 89); 3 Dec 2019 16:50:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3 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 16:50:16 +0000 Received: by mx1.osci.io (Postfix, from userid 994) id B8544203AF; Tue, 3 Dec 2019 11:50:14 -0500 (EST) Received: from gnutoolchain-gerrit.osci.io (gnutoolchain-gerrit.osci.io [8.43.85.239]) by mx1.osci.io (Postfix) with ESMTP id 24C8C201E0; Tue, 3 Dec 2019 11:50:12 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by gnutoolchain-gerrit.osci.io (Postfix) with ESMTP id 0D2242816F; Tue, 3 Dec 2019 11:50:12 -0500 (EST) X-Gerrit-PatchSet: 13 Date: Tue, 03 Dec 2019 16:50:00 -0000 From: "Simon Marchi (Code Review)" To: Andrew Burgess , gdb-patches@sourceware.org Cc: 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 11:50:11 -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: <20191203165012.0D2242816F@gnutoolchain-gerrit.osci.io> X-SW-Source: 2019-12/txt/msg00098.txt.bz2 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 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 Gerrit-Reviewer: Andrew Burgess Gerrit-CC: Joel Brobecker Gerrit-CC: Simon Marchi Gerrit-Comment-Date: Tue, 03 Dec 2019 16:50:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Andrew Burgess Comment-In-Reply-To: Simon Marchi Gerrit-MessageType: comment