From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id DCC213948A6B for ; Tue, 3 May 2022 16:13:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DCC213948A6B Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-538-dr7fLO4kM4u8jNAwEfsytQ-1; Tue, 03 May 2022 12:13:07 -0400 X-MC-Unique: dr7fLO4kM4u8jNAwEfsytQ-1 Received: by mail-wr1-f69.google.com with SMTP id g7-20020adfbc87000000b0020ac76d254bso6534678wrh.6 for ; Tue, 03 May 2022 09:13:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:in-reply-to:references:date :message-id:mime-version; bh=vHUnhh08mbGAeMfkp7NTz9/HWod5/Gi/eDQqZaNKzsU=; b=WzCwV+gVBl3lbOV3CgL7LyTm9+5WcJo1cfHIRs1dxomsZ/Oq4K80hfvBAIHZjTbf8L Cn+fmObz+9B++rf3KbpSEA3jfqHp2lLegzHZEIv/ymbOOFd7NkfhaaHgV/1FthwlWXNs oRTCvv04PVL2enT/g6/BSg8YwUMN7N/Fir15jG/PGlT10ntqDFT+OgmOgJ7jAqItpV// nD9773w8WD3ZVlrkK4z1rMvDPoQ+BQtmcWV6FSHyjDw89+PxBDkHGp+5bdhfyIFS0g/T wD0lPeecYWrY1C3fouKIk6g4djlY/w+WW/fDaKsS0TTiPM91LKiKOUbDpZNzS9ZCUDpB NhWQ== X-Gm-Message-State: AOAM533AAn4sfTW/nb6q8WUpszqW29qek00k813qKKC30iIx7NNJ1p+R ZUgGWLvJo9gKgR0TE/vB1Gr/XQbACzEI4ejtKYlVNQ7gMjPOWqFPE9qnWB15R72zOlHHCE4MFvz 1QcfRwbugOnMaaj0e4rICaQ== X-Received: by 2002:a05:6000:18a2:b0:20c:6d0d:10b0 with SMTP id b2-20020a05600018a200b0020c6d0d10b0mr5665480wri.345.1651594386404; Tue, 03 May 2022 09:13:06 -0700 (PDT) X-Google-Smtp-Source: ABdhPJy4qDjzWQDzeEfWZo04WHQ1sMRp/hxtBk4mJDOV/WZmGWpC92ItCRpRxvE9kn8rIRBY9SsEsg== X-Received: by 2002:a05:6000:18a2:b0:20c:6d0d:10b0 with SMTP id b2-20020a05600018a200b0020c6d0d10b0mr5665461wri.345.1651594386121; Tue, 03 May 2022 09:13:06 -0700 (PDT) Received: from localhost (host81-136-113-48.range81-136.btcentralplus.com. [81.136.113.48]) by smtp.gmail.com with ESMTPSA id n32-20020a05600c3ba000b003942a244f46sm2021697wms.31.2022.05.03.09.13.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 03 May 2022 09:13:05 -0700 (PDT) From: Andrew Burgess To: Simon Marchi , gdb-patches@sourceware.org Cc: Andrew Burgess Subject: Re: [PATCHv4 1/5] gdb: add new base class to gdb_disassembler In-Reply-To: <0a8f29f9-a8f9-e178-6c89-90ff38be7596@simark.ca> References: <603bc5660c36f0c8c8aaf9248d5f652428b78832.1650878049.git.aburgess@redhat.com> <0a8f29f9-a8f9-e178-6c89-90ff38be7596@simark.ca> Date: Tue, 03 May 2022 17:13:04 +0100 Message-ID: <87o80emw6n.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-6.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, 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 16:13:10 -0000 Simon Marchi via Gdb-patches writes: >> 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). I understand exactly where you're unease comes from, and I had the same feeling. The problem I think is the name of these things. So we used to have two classes 'gdb_disassembler' which is really a wrapper around the libopcodes 'disassemble_info'. Then we had 'gdb_pretty_print_disassembler' which had-a gdb_disassembler. In one of my original versions of this patch I added just gdb_disassemble_info as a base class, then gdb_disassembler inherited from that. At some point the patch expanded, and the new classes took the *_disassembler suffix rather than the *_disassemble_info suffix, which was maybe a mistake. So the classes you listed above, gdb_printing_disassembler and gdb_disassembler, really are specialisations of gdb_disassemble_info, but maybe this would all be better if I renamed all these things to use a _disassemble_info suffix? Thanks, Andrew > > 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