From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 57413 invoked by alias); 1 Aug 2017 16:31:46 -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 57390 invoked by uid 89); 1 Aug 2017 16:31:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=concluding, guarding, uneasy, meet X-Spam-User: qpsmtpd, 2 recipients X-HELO: mailapp01.imgtec.com Received: from mailapp01.imgtec.com (HELO mailapp01.imgtec.com) (195.59.15.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 01 Aug 2017 16:31:44 +0000 Received: from HHMAIL01.hh.imgtec.org (unknown [10.100.10.19]) by Forcepoint Email with ESMTPS id E4E60C32C0C8B; Tue, 1 Aug 2017 17:31:36 +0100 (IST) Received: from [10.20.78.24] (10.20.78.24) by HHMAIL01.hh.imgtec.org (10.100.10.21) with Microsoft SMTP Server id 14.3.294.0; Tue, 1 Aug 2017 17:31:40 +0100 Date: Tue, 01 Aug 2017 16:31:00 -0000 From: "Maciej W. Rozycki" To: Yao Qi CC: , Subject: Re: [PATCH 2/6] Delegate opcodes to select disassembler in GDB In-Reply-To: <867ezk5hea.fsf@gmail.com> Message-ID: 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> <867ezk5hea.fsf@gmail.com> User-Agent: Alpine 2.00 (DEB 1167 2008-08-23) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-SW-Source: 2017-08/txt/msg00010.txt.bz2 On Fri, 7 Jul 2017, Yao Qi wrote: > > 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. What is there in opcodes that needs guarding with the assertion though, i.e. will anything break if the `disassemble_info' passed is not exactly one obtained from the executable? I'd at least imagine a need for the user to be able to explicitly override the disassembler's machine via `set architecture ...', e.g. to get a meaningful disassembly of code that has been built with a temporary instruction set override for encodings that are not included in the ISA recorded in ELF file structures. > > 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. We also need symbols for other purposes, specifically telling code and data apart in PLT entries, jump tables and constant pools, so a fake symbol won't address all cases. Ideally we'd present output corresponding to that produced with `objdump -d', with a way for the user to request `objdump -D' equivalent if disassembly of what is supposed to be data is required. > 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. There is no bug, as it's `info->num_symbols' that tells the disassembler the number of symbols starting from `info->symtab_pos' in the symbol table affecting the location requested. Any further symbols are irrelevant (the symbol table is sorted by the increasing address, so those will be beyond the location disassembled). You also need to set `info->num_symbols' to 1 if you're only passing a single symbol in the first place; it's a part of this internal API. The setting of BSF_SYNTHETIC might be dangerous as code elsewhere assumes it's only set for PLT symbols. Right now it affects `is_mips16_plt_tail', causing it to crash, because with your change `asym.section' remains null and that function effectively accesses `asym.section->vma'. I feel uneasy about papering it over by adding a check for the section being non-null there, I think we need to look for a better solution as this is getting too fragile IMO. So given that this is a grave regression I think short-term we just need to remove the offending assertion checks. NB for the purpose of validation I have now bisected the actual regression to commit 6394c606997f ("Don't use print_insn_XXX in GDB"), rather than commit 39503f82427e ("Delegate opcodes to select disassembler in GDB") where the assertion check has been introduced. There is also commit 003ca0fd2286 ("Refactor disassembler selection") on the opcodes' side, causing assertion failures triggering once the checks in GDB have been removed. I'll be following up with a fix right away. Also in the course of this investigation I have discovered a grave regression in compressed breakpoint handling. I'll be posting a fix separately. Maciej