From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id Dv6vJNuLXWjZkCAAWB0awg (envelope-from ) for ; Thu, 26 Jun 2025 14:05:15 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1750961115; bh=meNRS3iG57ZZ+bo5SuSlQjSUEz2AwZHJrQ7cdPcNAA4=; h=Date:Subject:To:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=ufjEjL8zl+5pcZ+c6RQiaUfVi/r656EEJK7IhtemrW7eWdNPP+yFZrT7X4y8QXo+0 mOJ3ZAAD0LnYCegBKY3Dt5QqlyakpnK00pxGEKOGTeuw2KxUfr+w8jyvF+FxqYFiMk 8qgUn4isrx2Vn4tUQliiMt/l3jogz11hHnrfKZ4w= Received: by simark.ca (Postfix, from userid 112) id 84E721E11E; Thu, 26 Jun 2025 14:05:15 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-9.1 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED,RCVD_IN_VALIDITY_RPBL, RCVD_IN_VALIDITY_SAFE autolearn=ham autolearn_force=no version=4.0.1 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=Be4j7b8n; dkim=pass (1024-bit key) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=vyv5wDav; dkim-atps=neutral Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id D710C1E0C2 for ; Thu, 26 Jun 2025 14:05:14 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 55FD93854A83 for ; Thu, 26 Jun 2025 18:05:14 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 55FD93854A83 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=Be4j7b8n; dkim=pass (1024-bit key) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=vyv5wDav Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 41E03385B511 for ; Thu, 26 Jun 2025 18:04:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 41E03385B511 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 41E03385B511 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1750961073; cv=none; b=G1V3aU2KxuLtB48vFTyjgC01f2HV4MPpBp7oPeijJGYbG8EPDmlkWVSpsQTJflHIcJNL8sYQppaTka73Zwyal3Xy7Q1dnzFQbr3pQhlZf2B3QFcKv+1t+P7jqxFaHiZKDkqa8OzGC0zQSf0uJ7OH5Q3ZY8j+GJI1rPQIRFNmwQQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1750961073; c=relaxed/simple; bh=meNRS3iG57ZZ+bo5SuSlQjSUEz2AwZHJrQ7cdPcNAA4=; h=DKIM-Signature:DKIM-Signature:Message-ID:Date:MIME-Version: Subject:To:From; b=mxlyHtRKOK1jsfQ7zh1PE2VpwWCqPybeuk/XtYpL9CZU9HIs1XhmqpkIkvyhsSs+x703DF3P1BPJg0JnpdoKxQIeueOYohwz7fv63rCvkqIJovWSQAp+EmKQRX+9HkqujZ8c/U4ioBcPIJKuf/cLKQMejaOmB0XnPfRFuODpC6w= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 41E03385B511 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1750961072; bh=meNRS3iG57ZZ+bo5SuSlQjSUEz2AwZHJrQ7cdPcNAA4=; h=Date:Subject:To:References:From:In-Reply-To:From; b=Be4j7b8n6dgLxHg0zqp+jnrNse8NHovw+VuMRZ6Akml7MLGLUPVM0nP/jvd9Z61xU 1l3Ug0WK16tc4RiPIiiHTc8ir70GpHlIdFI+UUkuxM9kbyt4iWffCaDxhZjkd0Yjkn oJ/6b5ScYXYJa2jXH7ZHgNOsEMIK2m+KL4MZF+3E= Received: by simark.ca (Postfix, from userid 112) id DE8F41E126; Thu, 26 Jun 2025 14:04:32 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1750961071; bh=meNRS3iG57ZZ+bo5SuSlQjSUEz2AwZHJrQ7cdPcNAA4=; h=Date:Subject:To:References:From:In-Reply-To:From; b=vyv5wDavzZZzhZAGNhw9MXnPKHJkpzn/hjhIkqSCM48nFeqz7Aa+EgXYZGc16h4sR MIuOgKjSfyzNWVGNtAyXuSIni71U3nMNdYcO0WeFv9fBB73qgx7nOdzpPI9xL3fhyu w2EJwsdrmY7q2DhCOhebzs8tbhBWWOyFhrqZXOWo= Received: from [10.0.0.11] (modemcable238.237-201-24.mc.videotron.ca [24.201.237.238]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id F1A411E0C2; Thu, 26 Jun 2025 14:04:30 -0400 (EDT) Message-ID: <06dd119d-2789-4928-a651-9a8ca0567d84@simark.ca> Date: Thu, 26 Jun 2025 14:04:30 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 6/6] gdb/solib: C++ify solib_ops To: Pedro Alves , Simon Marchi , gdb-patches@sourceware.org References: <20250616193443.16703-1-simon.marchi@efficios.com> <20250616193443.16703-6-simon.marchi@efficios.com> Content-Language: en-US From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org On 2025-06-20 14:20, Pedro Alves wrote: > Hi! > > On 2025-06-16 20:33, Simon Marchi wrote: >> Convert solib_ops into an abstract base class (with abstract methods, >> some of them with default implementations) and convert all the existing >> solib_ops instances to solib_ops derived classes / implementations. > > Thanks a lot for doing this. > > Some small nits below. Other than that: > > Approved-By: Pedro Alves > >> >> Prior to this patch, solib_ops is a structure holding function pointers, >> of which there are only a handful of global instances (in the >> `solib-*.c` files). When passing an `solib_ops *` around, it's a >> pointer to one of these instances. After this patch, there are no more >> global solib_ops instances. Instances are created as needed and stored >> in struct program_space. These instances could eventually be made to >> contain the program space-specific data, which is currently kept in >> per-program space registries (I have some pending patches for that). >> >> Prior to this patch, `gdbarch_so_ops` is a gdbarch method that returns a >> pointer to the appropriate solib_ops implementation for the gdbarch. >> This is replaced with the `gdbarch_new_solib_ops` method, which returns >> a new instance of the appropriate solib_ops implementation for this >> gdbarch. This requires introducing some factory functions for the >> various solib_ops implementation, to be used as `gdbarch_new_solib_ops` >> callbacks. For instance: >> >> solib_ops_up >> new_linux_ilp32_svr4_solib_ops () >> { >> return std::make_unique (); >> } >> > > (Annoying) nit: I'd say that factory functions that return a non-raw pointer are more > typically named make_foo, like std::make_unique. new_foo suggests that they would return a > raw pointer, like operator new. Np, it's a quick change. Done. >> There is a little "base class template" hack in mips-linux-tdep.c, >> because both mips_linux_ilp32_svr4_solib_ops and >> mips_linux_lp64_svr4_solib_ops need to derive from different SVR4 base >> classes (linux_ilp32_svr4_solib_ops and linux_lp64_svr4_solib_ops), but >> they both want to override the in_dynsym_resolve_code method with the >> same implementation. Let me know if there's a more straightforward way >> to do this. > > I don't know why would call this a hack. This is a CRTP mixin, a standard > technique, that we use in other places in GDB, for the same reason. I think > you should just say that instead. Ack, done. > We should be able to simplify such code a bit with C++23, with the new > explicit object parameter syntax. I have seen this, but haven't internalized yet how it can be useful. >> The solib_ops::supports_namespaces method is new: the support for >> namespaces was previously predicated by the presence or absence of a >> find_solib_ns method. It now needs to be explicit. >> >> There is a new progspace::release_solib_ops method, which is only needed >> for rocm_solib_ops. For the moment, rocm_solib_ops replaces and wraps >> the existing svr4_solib_ops instance, in order to combine the results of >> the two. The plan is to have a subsequent patch to allow program spaces to have >> multiple solib_ops, removing the need that release_solib_ops. > > > Some word missing in "need that release_solib_ops". I guess "for". Replacing "that" with "for", actually. Fixed. >> Speaking of rocm_solib_ops: it previously overrode only a few methods by >> copying svr4_solib_ops and overwriting some function pointers. Now, it >> needs to implement all the methods that svr4_solib_ops implements, in >> order to forward the call. Otherwise, the default solib_ops method woul > > woul -> would Fixed. >> --- a/gdb/progspace.h >> +++ b/gdb/progspace.h >> @@ -21,6 +21,7 @@ >> #ifndef GDB_PROGSPACE_H >> #define GDB_PROGSPACE_H >> >> +#include "solib.h" >> #include "target.h" >> #include "gdb_bfd.h" >> #include "registry.h" >> @@ -234,19 +235,23 @@ struct program_space >> /* Set this program space's solib provider. >> >> The solib provider must be unset prior to call this method. */ >> - void set_solib_ops (const struct solib_ops &ops) >> + void set_solib_ops (solib_ops_up ops) >> { >> gdb_assert (m_solib_ops == nullptr); >> - m_solib_ops = &ops; >> + m_solib_ops = std::move (ops); >> }; >> >> - /* Unset this program space's solib provider. */ >> + /* Unset and free this program space's solib provider. */ >> void unset_solib_ops () >> { m_solib_ops = nullptr; } >> >> + /* Unset and return this program space's solib provider. */ >> + solib_ops_up release_solib_ops () >> + { return std::move (m_solib_ops); } >> + >> /* Get this program space's solib provider. */ >> - const struct solib_ops *solib_ops () const >> - { return m_solib_ops; } >> + struct solib_ops *solib_ops () const >> + { return m_solib_ops.get (); } > > Curious that const dropped here and other places. Was const previously > just wrong? Ah, it's because I have followup patches (not sure if/when I'll post them) that change solib implementations to move the data from registries to fields of solib_ops_* objects directly. Doing so requires making some methods non-const, and so to call them we need program_space::solib_ops to return a non-const solib_ops. But for the moment, given that solib_ops_* objects don't keep data, it would make more sense to leave everything const. I will make all the methods const again (like I had initially) and make program_space::solib_ops return a const object. We can remove the consts if/when it's justified to do so. Thanks for the review, I will push the series shortly, when I'm done addressing these comments. Simon