From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5803 invoked by alias); 1 Feb 2017 18:10:05 -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 5650 invoked by uid 89); 1 Feb 2017 18:10: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:10:00 +0000 Received: by simark.ca (Postfix, from userid 33) id 6EDCE1E166; Wed, 1 Feb 2017 13:09:58 -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:10:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org In-Reply-To: <1485909045-30285-2-git-send-email-palves@redhat.com> References: <1485909045-30285-1-git-send-email-palves@redhat.com> <1485909045-30285-2-git-send-email-palves@redhat.com> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.3 X-IsSubscribed: yes X-SW-Source: 2017-02/txt/msg00035.txt.bz2 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.