From patchwork Fri Jul 7 16:41:17 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yao Qi X-Patchwork-Id: 21472 Received: (qmail 127190 invoked by alias); 7 Jul 2017 16:41:29 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 127166 invoked by uid 89); 7 Jul 2017 16:41:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.4 required=5.0 tests=BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy=confuse, HTo:U*macro, H*r:sk:static. X-Spam-User: qpsmtpd, 2 recipients X-HELO: mail-it0-f67.google.com Received: from mail-it0-f67.google.com (HELO mail-it0-f67.google.com) (209.85.214.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 07 Jul 2017 16:41:26 +0000 Received: by mail-it0-f67.google.com with SMTP id v193so6215235itc.2; Fri, 07 Jul 2017 09:41:26 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-transfer-encoding; bh=EvBz6S+X/lrDydjkmrDMCI9fTX0LwaUQCWfvmEGfyck=; b=UafR9x/epQHUOOskVUGkZ/IgRHwhHFbJmhV4R+oHiC12dwbLbNg3ZQB0wExprS+dTj jb1MUKxC7o5alHLpyPdIdXDWUgJ0JrWs78Dy7r1vOOtHTZ9alXlqKBOpuX4nZ9uAFgXD xIVew8NUN7GySESooQKqNJlkUfFDNf3T9gOM9Jg0mCJI+/573+AHhUcaNkPnSQPdNGnu 4aVNIevfoqlI/jJ4UB1PbiamgN24o4PO8pxUE1NguUz2gTE6LWjcKAk1rfVVIPrlD9qb v05GkWOfKJp9er62+wMkCoIIGWbodkBie9i+sniu48afznTt3zAgQU33kWd0UE6Qok63 u/MA== X-Gm-Message-State: AKS2vOzt+OGfAgnzde4kpXHMHO9NePSbODYABcfV5yu5o89s09w+SxFS 6flNJQEDTeRQEjrF X-Received: by 10.107.9.137 with SMTP id 9mr59251667ioj.131.1499445684552; Fri, 07 Jul 2017 09:41:24 -0700 (PDT) Received: from E107787-LIN (static.42.136.251.148.clients.your-server.de. [148.251.136.42]) by smtp.gmail.com with ESMTPSA id o8sm1974446ioo.56.2017.07.07.09.41.22 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Fri, 07 Jul 2017 09:41:24 -0700 (PDT) From: Yao Qi To: "Maciej W. Rozycki" Cc: , Subject: Re: [PATCH 2/6] Delegate opcodes to select disassembler in GDB References: <1494931698-15309-1-git-send-email-yao.qi@linaro.org> <1494931698-15309-3-git-send-email-yao.qi@linaro.org> <86efu1diwp.fsf@gmail.com> Date: Fri, 07 Jul 2017 17:41:17 +0100 In-Reply-To: (Maciej W. Rozycki's message of "Thu, 6 Jul 2017 00:53:18 +0100") Message-ID: <867ezk5hea.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes "Maciej W. Rozycki" writes: > I can see the assumption from the assertion itself, but what is its > purpose? > The purpose of this assert is to check that the disassemble_info passed to opcodes is correctly got from the executable. > Normally you place an assertion in code to guard against a case that is > not handled (correctly) and if let through it would lead execution to go > astray, e.g. because it is a new complicated feature we have not yet got > to implementing or because it is a case we think that will never happen, > and code that follows has assumptions that will not be met. > > So if you say that you can remove the assertion with no adverse effect on > processing, then I think it should not have been placed there in the first > place. > > Anyway, if you look at code in `gdb_print_insn_mips', then you'll find > this comment: > > /* FIXME: cagney/2003-06-26: Is this even necessary? The > disassembler needs to be able to locally determine the ISA, and > not rely on GDB. Otherwize the stand-alone 'objdump -d' will not > work. */ > Yes, that is the point of my patch series. > and it is indeed the case that `objdump' handles this correctly without > the need to switch away from the BFD selected for the binary being > handled. However in GDB we have this problem that we do not pass the > symbol table down to libopcodes for the disassembler, and in the case of > the MIPS target it is the symbol table that carries information about > which functions contain regular MIPS code, MIPS16 code and microMIPS code > respectively. > > Lacking symbol information we resort to this hack of overriding the BFD > for the purpose of disassembly only, and this has the adverse effect of > getting instruction subsetting wrong: the `bfd_mach_mips16' and > `bfd_mach_mips_micromips' BFDs always choose the maximum instruction set > supported possible whereas the binary handled may only support a subset > (or worse yet an alternative set), as indicated by the original BFD > selected. This in turn may confuse and mislead the person running a debug > session into thinking code disassembled is not the problem they are after. > > Do you think it would be possible as a part of your disassembler rework > to make the symbol table available to libopcodes? Then the hack currently > present in `gdb_print_insn_mips' could go. I remember looking into it a > while ago and concluding it would be somewhat tricky because the symbol > table format expected by libopcodes is not the same that we use in > GDB. We can pass the symbol table to opcodes, so it can choose the right disassembler. However, current GDB uses both symbol and address to determine some address is in a MIPS16 or microMIPS function (done by mips_pc_is_mips16, mips_pc_is_micromips). If we want to use symbol table to tell the address is in microMIPS or MIPS16, and GDB can't find a symbol for a given address, I suppose we need to create a fake symbol, and pass it to disassembler. Why don't we teach GDB unconditionally create a fake symbol with the right bits set in order to meet the check in mips-dis.c:is_compressed_mode_p? arm-tdep.c:gdb_print_insn_arm has already do so to disassemble Thumb instructions. What do you think of the patch below? IMO, change in is_compressed_mode_p fixes a bug on usage of info->num_symbols vs. info->symtab_size. I can't test this patch. diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c index c1800e4..b848015 100644 --- a/gdb/mips-tdep.c +++ b/gdb/mips-tdep.c @@ -7007,14 +7007,31 @@ gdb_print_insn_mips (bfd_vma memaddr, struct disassemble_info *info) disassembler needs to be able to locally determine the ISA, and not rely on GDB. Otherwize the stand-alone 'objdump -d' will not work. */ - if (mips_pc_is_mips16 (gdbarch, memaddr)) - info->mach = bfd_mach_mips16; - else if (mips_pc_is_micromips (gdbarch, memaddr)) - info->mach = bfd_mach_mips_micromips; - - /* Round down the instruction address to the appropriate boundary. */ - memaddr &= (info->mach == bfd_mach_mips16 - || info->mach == bfd_mach_mips_micromips) ? ~1 : ~3; + if (mips_pc_is_mips16 (gdbarch, memaddr) + || mips_pc_is_micromips (gdbarch, memaddr)) + { + static asymbol asym; + static asymbol *asymp = &asym; + + /* Create a fake symbol to tell disassembler in opcodes that + the code is MIPS16 or miscroMIPS. */ + asym.flags = BSF_SYNTHETIC; + info->symtab_pos = 0; + info->symtab_size = 1; + info->symtab = ≈ + info->symbols = ≈ + + if (mips_pc_is_mips16 (gdbarch, memaddr)) + asym.udata.i = ELF_ST_SET_MIPS16 (asym.udata.i); + else + asym.udata.i = ELF_ST_SET_MICROMIPS (asym.udata.i); + + /* Round down the instruction address to the appropriate + boundary. */ + memaddr &= ~1; + } + else + memaddr &= ~3; /* Set the disassembler options. */ if (!info->disassembler_options) diff --git a/opcodes/mips-dis.c b/opcodes/mips-dis.c index 4519500..d413841 100644 --- a/opcodes/mips-dis.c +++ b/opcodes/mips-dis.c @@ -2452,7 +2452,7 @@ is_compressed_mode_p (struct disassemble_info *info, bfd_boolean micromips_p) int i; int l; - for (i = info->symtab_pos, l = i + info->num_symbols; i < l; i++) + for (i = info->symtab_pos, l = info->symtab_size; i < l; i++) if (((info->symtab[i])->flags & BSF_SYNTHETIC) != 0 && ((!micromips_p && ELF_ST_IS_MIPS16 ((*info->symbols)->udata.i))