From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 101379 invoked by alias); 5 Jun 2017 20:07:51 -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 101034 invoked by uid 89); 5 Jun 2017 20:07:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.0 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=determining, reserved, scanned 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; Mon, 05 Jun 2017 20:07:47 +0000 Received: by simark.ca (Postfix, from userid 33) id AFF3C1E4E8; Mon, 5 Jun 2017 16:07:49 -0400 (EDT) To: Anton Kolesov Subject: Re: [PATCH] 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: Mon, 05 Jun 2017 20:07:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org, Francois Bedard In-Reply-To: <20170601160213.16092-1-Anton.Kolesov@synopsys.com> References: <20170601160213.16092-1-Anton.Kolesov@synopsys.com> Message-ID: <64a77ae925bca53fed60b579642f07f8@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.5 X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg00136.txt.bz2 On 2017-06-01 18:02, Anton Kolesov wrote: > 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. Unfortunately > the latter > trick will be used only when doing semantic disassembling to analyze > instructions, because arc_delayed_print_insn is not called to print > instructions starting with [1]. Maybe it would be possible to fix that > by > altering opcodes/disassemble.c:disassemble_init_for_target somehow. > > Support for CPU in disassembler options for ARC has been added in [2]. > > [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 Hi Anton, This is not very clear to me (I haven't really touched disassembly yet), so let me train to summarize the situation. Let's say you feed the right executable to GDB, but connect to a gdbserver that doesn't report the cpu arch in the target description. arc_disassembler_options won't be set, so we won't pass an explicit disassembler option. opcodes internals could try to fall back on data from the BFD if disassemble_info::section was set. However, this is only done in arc_delayed_print_insn, which is not used in that case (it was removed as a gdbarch callback in commit [2]). Does that sound right? Could gdb_disassembler set the section field of disassemble_info itself? What you are doing to set the section field here is not ARC-specific, it looks like it could potentially help other architectures to have that field set. Other places that use the disassembler would have to set it too, for example in the object prepared by the arc_disassemble_info function. > gdb/ChangeLog: > > yyyy-mm-dd Anton Kolesov > > * arc-tdep.c (arc_disassembler_options): New variable. > (arc_gdbarch_init): Set it. > (arc_delayed_print_insn): Set info->section when needed. > --- > gdb/arc-tdep.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c > index d9ee5c6..c0b8a14 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. */ > @@ -1410,6 +1412,31 @@ arc_delayed_print_insn (bfd_vma addr, struct > disassemble_info *info) > will handle NULL value gracefully. */ > print_insn = arc_get_disassembler (exec_bfd); > gdb_assert (print_insn != NULL); > + > + /* 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); > + if (s != NULL) > + info->section = s->the_bfd_section; > + } > + > return print_insn (addr, info); > } > > @@ -2041,6 +2068,23 @@ 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) > + { > + std::string disasm_options > + = string_printf ("cpu=%s", tdesc_arch->printable_name); > + arc_disassembler_options = xstrdup (disasm_options.c_str ()); You could shorten this by using xstrprintf directly. > + set_gdbarch_disassembler_options (gdbarch, > + &arc_disassembler_options); > + } > + } > + > tdesc_use_registers (gdbarch, tdesc, tdesc_data); > > return gdbarch; This part, which sets the disassembler options based on the target description, looks good to me. Simon