From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 37349 invoked by alias); 21 Nov 2019 05:39:53 -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 37336 invoked by uid 89); 21 Nov 2019 05:39:52 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3 autolearn=ham version=3.3.1 spammy=steal, Sourceware 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; Thu, 21 Nov 2019 05:39:51 +0000 Received: by mx1.osci.io (Postfix, from userid 994) id C6A00201AD; Thu, 21 Nov 2019 00:39:48 -0500 (EST) Received: from gnutoolchain-gerrit.osci.io (gnutoolchain-gerrit.osci.io [8.43.85.239]) by mx1.osci.io (Postfix) with ESMTP id D68C420300; Thu, 21 Nov 2019 00:39:45 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by gnutoolchain-gerrit.osci.io (Postfix) with ESMTP id B20A32816F; Thu, 21 Nov 2019 00:39:45 -0500 (EST) X-Gerrit-PatchSet: 3 Date: Thu, 21 Nov 2019 05:39:00 -0000 From: "Simon Marchi (Code Review)" To: Andrew Burgess , gdb-patches@sourceware.org Cc: Joel Brobecker , Tom Tromey Auto-Submitted: auto-generated X-Gerrit-MessageType: comment Subject: [review v3] gdb/mi: Add new commands -symbol-info-{functions,variables,types} X-Gerrit-Change-Id: Ic2fc6a6750bbce91cdde2344791014e5ef45642d X-Gerrit-Change-Number: 266 X-Gerrit-ChangeURL: X-Gerrit-Commit: cb70e59753d245618bf4f20641b41216e5a8b9d5 In-Reply-To: References: X-Gerrit-Comment-Date: Thu, 21 Nov 2019 00:39:45 -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: <20191121053945.B20A32816F@gnutoolchain-gerrit.osci.io> X-SW-Source: 2019-11/txt/msg00643.txt.bz2 Simon Marchi has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/266 ...................................................................... Patch Set 3: (7 comments) Since you were looking for suggestions for how to make the output code less ugly: I think the best way would be to make the result from the symbol search code more structured from the start, not just return a flat list of symbols. That might not be a realistic goal for the short term though. But what you can do now is iterate on the vector as if it was structured like that: if there are debug symbols: prepare the emitter for debug symbols for each symtab: prepare the emitter for the symtab for each symbol: output the symbol if there are non-debug symbols: prepare the emitter for non-debug symbols for each non-debug symbols: output the symbol This way you don't need any optionals, the code is linear and straighforward to follow, the order of emitter destruction is automatically good because each emitter is in the right scope. I have uploaded the users/simark/mi-symbols-output branch on Sourceware with a commit that does that, feel free to steal from it. But the main part is: static void mi_symbol_info (enum search_domain kind, const char *regexp, const char *t_regexp, bool exclude_minsyms) { /* Must make sure that if we're interrupted, symbols gets freed. */ global_symbol_searcher sym_search (kind, regexp, t_regexp, exclude_minsyms); std::vector symbols = sym_search.search (); ui_out *uiout = current_uiout; int i = 0; ui_out_emit_tuple outer_symbols_emitter (uiout, "symbols"); /* Debug symbols are placed first. */ if (i < symbols.size () && symbols[i].msymbol.minsym == nullptr) { ui_out_emit_list debug_symbols_list_emitter (uiout, "debug"); /* As long as we have debug symbols... */ while (i < symbols.size () && symbols[i].msymbol.minsym == nullptr) { symtab *symtab = symbol_symtab (symbols[i].symbol); ui_out_emit_tuple symtab_tuple_emitter (uiout, nullptr); uiout->field_string ("filename", symtab_to_filename_for_display (symtab)); uiout->field_string ("fullname", symtab_to_fullname (symtab)); ui_out_emit_list symbols_list_emitter (uiout, "symbols"); /* As long as we have debug symbols from this symtab... */ while (i < symbols.size () && symbols[i].msymbol.minsym == nullptr && symbol_symtab (symbols[i].symbol) == symtab) { symbol_search &s = symbols[i]; output_debug_symbol(uiout, kind, s.symbol, s.block); i++; } } } /* Non-debug symbols are placed after. */ if (i < symbols.size ()) { ui_out_emit_list nondebug_symbols_list_emitter (uiout, "nondebug"); /* As long as we have nondebug symbols... */ while (i < symbols.size ()) { gdb_assert (symbols[i].msymbol.minsym != nullptr); output_nondebug_symbol(uiout, symbols[i].msymbol); i++; } } } | --- gdb/mi/mi-symbol-cmds.c | +++ gdb/mi/mi-symbol-cmds.c | @@ -62,0 +190,38 @@ class mi_symbol_info_emitter | + | + /* Record the current symtab. */ | + m_last_symtab = s; | + } | + } | + | +public: | + /* Constructor. */ | + mi_symbol_info_emitter (struct ui_out *uiout) | + : m_last_symtab (nullptr), PS3, Line 199: I don't think we have a rule about which style to use, but this one could be initialized directly where the field is declared, I find it nice: const symtab *m_last_symtab = nullptr; | + m_uiout (uiout), | + m_outer_symbols (uiout, "symbols") | + { /* Nothing. */ } | + | + /* Output P a symbol found by searching for symbols of type KIND. */ | + void output (const symbol_search &p, enum search_domain kind) | + { | + if (p.msymbol.minsym != NULL) | + { | + /* If this is the first nondebug symbol, and we have previous PS3, Line 209: previously | + outputted a debug symbol then we need to close down all of the | + emitters related to printing debug symbols. */ | + maybe_finish_debug_output (); | + | + /* If this is the first nondebug symbol then we need to create the | + emitters related to printing nondebug symbols. */ | + maybe_start_nondebug_symbol_output (); | + | + /* We are no safe to emit the nondebug symbol. */ PS3, Line 218: now | + output_nondebug_symbol (p.msymbol); | + } | + else | + { | + /* All debug symbols should appear in the list before all | + non-debug symbols. */ | + gdb_assert (!have_started_nondebug_symbol_output ()); | + | + /* If this is the first debug symbol then we need to create the ... | @@ -62,0 +234,48 @@ public: | + | + /* Emit information for this debug symbol. */ | + mi_info_one_symbol_details (kind, p.symbol, p.block); | + } | + } | +}; | + | +/* This is the guts of the commands '-symbol-info-functions', | + '-symbol-info-variables', and '-symbol-info-types'. It calls | + search_symbols to find all matches and then prints the matching PS3, Line 243: This should be changed. | + [m]symbols in an MI structured format. This is similar to | + symtab_symbol_info in symtab.c. All the arguments are used to | + initialise a SEARCH_SYMBOLS_SPEC, see symtab.h for a description of PS3, Line 246: This too. | + their meaning. */ | + | +static void | +mi_symbol_info (enum search_domain kind, const char *regexp, | + const char *t_regexp, bool exclude_minsyms) | +{ | + /* Must make sure that if we're interrupted, symbols gets freed. */ | + global_symbol_searcher sym_search (kind, regexp, t_regexp, exclude_minsyms); | + std::vector symbols = sym_search.search (); | + | + mi_symbol_info_emitter emitter (current_uiout); | + for (const symbol_search &p : symbols) | + { | + QUIT; | + emitter.output (p, kind); PS3, Line 261: Since `kind` never changes, couldn't it be passed just once to the constructor? | + } | +} | + | +/* Helper for mi_cmd_symbol_info_{functions,variables} - depending on KIND. | + Processes command line options from ARGV and ARGC. */ | + | +static void | +mi_info_functions_or_variables (enum search_domain kind, char **argv, int argc) | +{ | + const char *regexp = nullptr; | + const char *t_regexp = nullptr; PS3, Line 272: I know these variable names are used elsewhere for the same purpose, but I think that "name_regexp" and "type_regexp" would make it them much clearer. | + bool exclude_minsyms = true; | + | + enum opt | + { | + INCLUDE_NONDEBUG_OPT, TYPE_REGEXP_OPT, NAME_REGEXP_OPT | + }; | + static const struct mi_opt opts[] = | + { | + {"-include-nondebug" , INCLUDE_NONDEBUG_OPT, 0}, -- Gerrit-Project: binutils-gdb Gerrit-Branch: master Gerrit-Change-Id: Ic2fc6a6750bbce91cdde2344791014e5ef45642d Gerrit-Change-Number: 266 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Burgess Gerrit-Reviewer: Andrew Burgess Gerrit-Reviewer: Tom Tromey Gerrit-CC: Joel Brobecker Gerrit-CC: Simon Marchi Gerrit-Comment-Date: Thu, 21 Nov 2019 05:39:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment