From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 61871 invoked by alias); 13 Jun 2017 22:16:35 -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 59552 invoked by uid 89); 13 Jun 2017 22:16:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy=HTo:D*synopsys.com X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 13 Jun 2017 22:16:28 +0000 Received: by simark.ca (Postfix, from userid 33) id 573E01E4E8; Tue, 13 Jun 2017 18:16:31 -0400 (EDT) To: Anton Kolesov Subject: Re: [PATCH v2] arc: Select CPU model properly before disassembling X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Tue, 13 Jun 2017 22:16:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org, Francois Bedard In-Reply-To: <20170613134951.29359-1-Anton.Kolesov@synopsys.com> References: <39A54937CC95F24AA2F794E2D2B66B135874F238@DE02WEMBXB.internal.synopsys.com> <20170613134951.29359-1-Anton.Kolesov@synopsys.com> Message-ID: <887411aaab641fb92f809eadce4d9011@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.5 X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg00410.txt.bz2 On 2017-06-13 15:49, Anton Kolesov wrote: > Changes in V2: > > * Use xstrprintf instead of string_printf > * Reinstate arc_delayed_print_insn as a print instruction function for > ARC. > * Change arc_delayed_print_insn to use default_print_insn. > * Change commit description accordingly. > > --- > > Enforce CPU model for disassembler via its options, if it was specified > in XML > target description, otherwise use default method of determining CPU > implemented > in disassembler - scanning ELF private header. The latter requires > disassemble_info->section to be properly initialized. To make sure > that > info->section is set in all cases this patch partially reverts [1] for > ARC: it > reinstates arc_delayed_print_insn as a "print_insn" function for ARC, > but > now this function only sets disassemble_info->section and then calls > default_print_insn to do the rest of the job. > > Support for CPU in disassembler options for ARC has been added in [2]. The patch looks good to me, there's just one formatting nit below, and some random thoughts (which doesn't affect the fact that the patch LGTM). > [1] > https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=39503f82427e22ed8e04d986ccdc8562091ec62e > [2] > https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=10045478d984f9924cb945423388ba25b7dd3ffe > > gdb/ChangeLog: > > yyyy-mm-dd Anton Kolesov > > * arc-tdep.c (arc_disassembler_options): New variable. > (arc_gdbarch_init): Set and use it. Use arc_delayed_print_insn instead > of default_print_insn. > (arc_delayed_print_insn): Set info->section when needed, > use default_print_insn to retrieve a disassembler. > --- > gdb/arc-tdep.c | 51 > +++++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 45 insertions(+), 6 deletions(-) > > diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c > index d9ee5c6..9a5efc1 100644 > --- a/gdb/arc-tdep.c > +++ b/gdb/arc-tdep.c > @@ -145,6 +145,8 @@ static const char *const > core_arcompact_register_names[] = { > "lp_count", "reserved", "limm", "pcl", > }; > > +static char *arc_disassembler_options = NULL; > + > /* Functions are sorted in the order as they are used in the > _initialize_arc_tdep (), which uses the same order as gdbarch.h. > Static > functions are defined before the first invocation. */ > @@ -1405,12 +1407,31 @@ arc_skip_prologue (struct gdbarch *gdbarch, > CORE_ADDR pc) > int > arc_delayed_print_insn (bfd_vma addr, struct disassemble_info *info) > { > - int (*print_insn) (bfd_vma, struct disassemble_info *); > - /* exec_bfd may be null, if GDB is run without a target BFD file. > Opcodes > - will handle NULL value gracefully. */ > - print_insn = arc_get_disassembler (exec_bfd); > - gdb_assert (print_insn != NULL); > - return print_insn (addr, info); > + /* Standard BFD "machine number" field allows libocodes disassembler > to > + distinguish ARC 600, 700 and v2 cores, however v2 encompasses > both ARC EM > + and HS, which have some difference between. There are two ways > to specify > + what is the target core: > + 1) via the disassemble_info->disassembler_options; > + 2) otherwise libopcodes will use private (architecture-specific) > ELF > + header. > + > + Using disassembler_options is preferable, because it comes > directly from > + GDBserver which scanned an actual ARC core identification info. > However, > + not all GDBservers report core architecture, so as a fallback GDB > still > + should support analysis of ELF header. The libopcodes > disassembly code > + uses the section to find the BFD and the BFD to find the ELF > header, > + therefore this function should set disassemble_info->section > properly. > + > + disassembler_options was already set by non-target specific code > with > + proper options obtained via gdbarch_disassembler_options (). */ > + if (info->disassembler_options == NULL) > + { > + struct obj_section * s = find_pc_section (addr); Extra space after the *. > + if (s != NULL) > + info->section = s->the_bfd_section; > + } When printing multiple instructions in a row, is this function called multiple times with the same disassemble_info? If so, are we going to make a find_pc_section call for each instruction? Is this needed, given that the options won't change for a given ELF (on ARC at least)? > + > + return default_print_insn (addr, info); > } > > /* Baremetal breakpoint instructions. > @@ -2013,6 +2034,8 @@ arc_gdbarch_init (struct gdbarch_info info, > struct gdbarch_list *arches) > > set_gdbarch_frame_align (gdbarch, arc_frame_align); > > + set_gdbarch_print_insn (gdbarch, arc_delayed_print_insn); > + > set_gdbarch_cannot_step_breakpoint (gdbarch, 1); > > /* "nonsteppable" watchpoint means that watchpoint triggers before > @@ -2041,6 +2064,22 @@ arc_gdbarch_init (struct gdbarch_info info, > struct gdbarch_list *arches) > if (tdep->jb_pc >= 0) > set_gdbarch_get_longjmp_target (gdbarch, arc_get_longjmp_target); > > + /* Disassembler options. Enforce CPU if it was specified in XML > target > + description, otherwise use default method of determining CPU (ELF > private > + header). */ > + if (info.target_desc != NULL) > + { > + const struct bfd_arch_info *tdesc_arch > + = tdesc_architecture (info.target_desc); > + if (tdesc_arch != NULL) > + { > + arc_disassembler_options = xstrprintf ("cpu=%s", > + tdesc_arch->printable_name); > + set_gdbarch_disassembler_options (gdbarch, > + &arc_disassembler_options); > + } > + } > + I was a bit concerned with the fact that multiple gdbarch objects (that represent different architectures) can end up sharing the same pointer to arc_disassembler_options, so necessarily some of them will have the wrong value at a certain point. But that doesn't look like an immediate problem, since you don't seem to recycle previously created gdbarch objects, like some other architectures do (does that mean that the gdbarch list keeps growing indefinitely if you connect multiple times, load new exec files?). The gdbarch in use should always be the last that was created, so arc_disassembler_options should contain the good value for that gdbarch. I was thinking that a situation like this could be problematic, if you were re-using gdbarch objects: 1. You connect to a gdbserver that reports arch A. A gdbarch is created and arc_disassembler_options is set for arch A. 2. You disconnect, and connect to a gdbserver that reports arch B. A gdbarch is created and arc_disassembler_options is set for arch B. 3. You disconnect, and connect again to the first gdbserver. This time, you would return the existing gdbarch for arch A, so the current gdbarch (arch A) would still use the disassembler options of arch B. But like I said, that doesn't seem to be the case right now. That makes me wonder why we pass a char ** and not simply an allocated char *, of which the gdbarch would take ownership, which would avoid this problem entirely. Simon