From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 94089 invoked by alias); 1 Feb 2017 18:26:13 -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 93983 invoked by uid 89); 1 Feb 2017 18:26:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No 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; Wed, 01 Feb 2017 18:26:00 +0000 Received: by simark.ca (Postfix, from userid 33) id 418EE1E7F5; Wed, 1 Feb 2017 13:25:59 -0500 (EST) To: Pedro Alves Subject: Re: [PATCH v4 1/2] Add back gdb_pretty_print_insn 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: Wed, 01 Feb 2017 18:26:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org In-Reply-To: References: <1485909045-30285-1-git-send-email-palves@redhat.com> <1485909045-30285-2-git-send-email-palves@redhat.com> Message-ID: <4930d51bf36a1182279d972150bce661@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.3 X-IsSubscribed: yes X-SW-Source: 2017-02/txt/msg00036.txt.bz2 On 2017-02-01 13:09, Simon Marchi wrote: > On 2017-01-31 19:30, Pedro Alves wrote: >> ui_file_rewind is a ui_file method that only really works with mem >> buffer files, and is a nop on other ui_file types. It'd be desirable >> to eliminate it from the base ui_file interface, and move it to the >> "mem_fileopen" subclass of ui_file instead. A following patch does >> just that. >> >> Unfortunately, there are a couple references to ui_file_rewind inside >> gdb_disassembler::pretty_print_insn that were made harder to eliminate >> with the recent addition of the gdb_disassembler wrapper. >> >> Before the gdb_disassembler wrapper was added, in commit >> e47ad6c0bd7aa3 ("Refactor disassembly code"), gdb_pretty_print_insn >> used to be passed a ui_file pointer as argument, and it was simple to >> adjust that pointer be a "mem_fileopen" ui_file pointer instead, since >> there's only one gdb_pretty_print_insn caller. >> >> That commit made gdb_pretty_print_insn be a method of >> gdb_disassembler, and removed the method's ui_file parameter at the >> same time, replaced by referencing the gdb_disassembler's stream >> instead. The trouble is that a gdb_disassembler can be instantiated >> with a pointer any kind of ui_file. Casting the gdb_disassembler's >> stream to a mem_fileopen ui_file inside >> gdb_disassembler::pretty_print_insn in order to call the reset method >> would be gross hack. >> >> The fix here is to: >> >> - make gdb_disassembler::pretty_print_insn a be free function again >> instead of a method of gdb_disassembler. I.e., bring back >> gdb_pretty_print_insn. >> >> - but, don't add back the ui_file * parameter. We'd always be >> passing in a fresh mem_fileopen anyway, so move the mem_fileopen >> allocation inside. That is a better interface, given that the >> ui_file is only ever used as temporary scratch buffer as an >> implementation detail of gdb_pretty_print_insn. The function's >> real "where to send output" parameter is the ui_out pointer. >> >> - don't add back a disassemble_info pointer either. That used to be >> necessary for this bit: >> >> err = m_di.read_memory_func (pc, &data, 1, &m_di); >> if (err != 0) >> m_di.memory_error_func (err, pc, &m_di); >> >> ... but AFAIK, it's not really necessary. We can replace those >> three lines with a call to read_code. This seems to fix a >> regression even, because before commit d8b49cf0c891d0 ("Don't throw >> exception in dis_asm_memory_error"), that memory_error_func call >> would throw an error/exception, but now it only records the error >> in the gdb_disassembler's m_err_memaddr field. (read_code throws >> on error.) >> >> With all these, gdb_pretty_print_insn is completely layered on top of >> gdb_disassembler only using the latter's public API. > > I don't think I understand the situation fully, but what you suggest > looks good to me. I was confused by the fact that the > gdb_disassembler constructor accepts a stream, but the > pretty_print_insn method takes a uiout. Which one is used for > printing then? I think that your patch clears that up. > > The only possible issue I can see is that in your version, one > gdb_disassembler and one string_file object are constructed for each > disassembled instruction, rather than re-using them for as long as we > need to disassemble. I don't know how much impact it has on the > performance (probably negligible), but something to keep in mind. I'll just mention it so we hopefully don't duplicate work, looking at your patch has prompted me to start a patch adding a "disassembly_flags" enum type. https://github.com/simark/binutils-gdb/tree/disas-flags