From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26401 invoked by alias); 2 Dec 2019 17:57:45 -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 26366 invoked by uid 89); 2 Dec 2019 17:57:45 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00 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; Mon, 02 Dec 2019 17:57:44 +0000 Received: by mx1.osci.io (Postfix, from userid 994) id 68DDF20334; Mon, 2 Dec 2019 12:57:42 -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 2209A20362; Mon, 2 Dec 2019 12:57:40 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by gnutoolchain-gerrit.osci.io (Postfix) with ESMTP id 036EA2816F; Mon, 2 Dec 2019 12:57:40 -0500 (EST) X-Gerrit-PatchSet: 8 Date: Mon, 02 Dec 2019 17:57: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 v8] 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: a45290c61f433efb3f64cb883ff2b29e8fe7b5cd In-Reply-To: References: X-Gerrit-Comment-Date: Mon, 2 Dec 2019 12:57:39 -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: <20191202175740.036EA2816F@gnutoolchain-gerrit.osci.io> X-SW-Source: 2019-12/txt/msg00048.txt.bz2 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 Gerrit-Reviewer: Andrew Burgess Gerrit-CC: Joel Brobecker Gerrit-CC: Simon Marchi Gerrit-Comment-Date: Mon, 02 Dec 2019 17:57:39 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment