From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id BD4AE3856DC6 for ; Tue, 3 May 2022 13:34:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org BD4AE3856DC6 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 4373E1E00E; Tue, 3 May 2022 09:34:48 -0400 (EDT) Message-ID: <0a8f29f9-a8f9-e178-6c89-90ff38be7596@simark.ca> Date: Tue, 3 May 2022 09:34:47 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1 Subject: Re: [PATCHv4 1/5] gdb: add new base class to gdb_disassembler Content-Language: en-US To: Andrew Burgess , gdb-patches@sourceware.org Cc: Andrew Burgess References: <603bc5660c36f0c8c8aaf9248d5f652428b78832.1650878049.git.aburgess@redhat.com> From: Simon Marchi In-Reply-To: <603bc5660c36f0c8c8aaf9248d5f652428b78832.1650878049.git.aburgess@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-7.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 May 2022 13:34:50 -0000 > From: Andrew Burgess > > The motivation for this change is an upcoming Python disassembler API > that I would like to add. As part of that change I need to create a > new disassembler like class that contains a disassemble_info and a > gdbarch. The management of these two objects is identical to how we > manage these objects within gdb_disassembler, so it might be tempting > for my new class to inherit from gdb_disassembler. > > The problem however, is that gdb_disassembler has a tight connection > between its constructor, and its print_insn method. In the > constructor the ui_file* that is passed in is replaced with a member > variable string_file*, and then in print_insn, the contents of the > member variable string_file are printed to the original ui_file*. > > What this means is that the gdb_disassembler class has a tight > coupling between its constructor and print_insn; the class just isn't > intended to be used in a situation where print_insn is not going to be > called, which is how my (upcoming) sub-class would need to operate. > > My solution then, is to separate out the management of the > disassemble_info and gdbarch into a new gdb_disassemble_info class, > and make this class a parent of gdb_disassembler. > > In arm-tdep.c and mips-tdep.c, where we used to cast the > disassemble_info->application_data to a gdb_disassembler, we can now > cast to a gdb_disassemble_info as we only need to access the gdbarch > information. > > Now, my new Python disassembler sub-class will still want to print > things to an output stream, and so we will want access to the > dis_asm_fprintf functionality for printing. > > However, rather than move this printing code into the > gdb_disassemble_info base class, I have added yet another level of > hierarchy, a gdb_printing_disassembler, thus the class structure is > now: > > struct gdb_disassemble_info {}; > struct gdb_printing_disassembler : public gdb_disassemble_info {}; > struct gdb_disassembler : public gdb_printing_disassembler {}; I can't explain it very well, but it seems a little strange to make the other classes inherit from gdb_disassemble_info. From the name (and the code), I understand that gdb_disassemble_info purely holds the data necessary for the disassemblers to do their job. So this is not really an IS-A relationship, but more a HAS-A. So I would think composition would be more natural (i.e. gdb_printing_disassembler would have a field of type gdb_disassemble_info). But with the callbacks we pass to struct disassemble_info (such as fprintf_func and fprintf_styled_func), what is data and what is behavior gets a little blurry. It doesn't matter much in the end, I'm fine with what you have. And it can always be refactored later. > +gdb_disassemble_info::gdb_disassemble_info > + (struct gdbarch *gdbarch, struct ui_file *stream, > + read_memory_ftype read_memory_func, memory_error_ftype memory_error_func, > + print_address_ftype print_address_func, fprintf_ftype fprintf_func, > + fprintf_styled_ftype fprintf_styled_func) > + : m_gdbarch (gdbarch) > { > - init_disassemble_info (&m_di, &m_buffer, dis_asm_fprintf, > - dis_asm_styled_fprintf); > + gdb_assert (fprintf_func != nullptr); > + gdb_assert (fprintf_styled_func != nullptr); > + init_disassemble_info (&m_di, stream, fprintf_func, > + fprintf_styled_func); > m_di.flavour = bfd_target_unknown_flavour; > - m_di.memory_error_func = dis_asm_memory_error; > - m_di.print_address_func = dis_asm_print_address; > - /* NOTE: cagney/2003-04-28: The original code, from the old Insight > - disassembler had a local optimization here. By default it would > - access the executable file, instead of the target memory (there > - was a growing list of exceptions though). Unfortunately, the > - heuristic was flawed. Commands like "disassemble &variable" > - didn't work as they relied on the access going to the target. > - Further, it has been superseeded by trust-read-only-sections > - (although that should be superseeded by target_trust..._p()). */ > - m_di.read_memory_func = read_memory_func; > + if (memory_error_func != nullptr) > + m_di.memory_error_func = memory_error_func; > + if (print_address_func != nullptr) > + m_di.print_address_func = print_address_func; > + if (read_memory_func != nullptr) > + m_di.read_memory_func = read_memory_func; Are the nullptr checks needed? Are these fields initially nullptr, or they have some other value? > +struct gdb_disassemble_info > +{ > + DISABLE_COPY_AND_ASSIGN (gdb_disassemble_info); > > - /* Return the gdbarch of gdb_disassembler. */ > + /* Return the gdbarch we are disassembing for. */ disassembing -> disassembling Simon