From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id g2zZMcSpVWhpSxYAWB0awg (envelope-from ) for ; Fri, 20 Jun 2025 14:34:44 -0400 Received: by simark.ca (Postfix, from userid 112) id BB66D1E11C; Fri, 20 Jun 2025 14:34:44 -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.0 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, 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 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 E8F451E089 for ; Fri, 20 Jun 2025 14:34:43 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 8C53F3A13460 for ; Fri, 20 Jun 2025 18:33:36 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 8C53F3A13460 Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com [209.85.221.44]) by sourceware.org (Postfix) with ESMTPS id 6F4C338CBF7A for ; Fri, 20 Jun 2025 18:20:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6F4C338CBF7A Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 6F4C338CBF7A Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=209.85.221.44 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1750443610; cv=none; b=H/oR8lM9dUzcSi1A7nmsFRdun1TS4hX0GznEa/1h1C5BOMVp+plHanP3p4UPp3vu3glsBOSI7tDh9ucR1wEPXXy2pe9/+i3Y4J89qm3VXVoVJH+JeHXrA13q32lVELGX10y0q62W+l1G6E+a6D77Z1p9YPkL3iWdnga3g7N1Pqc= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1750443610; c=relaxed/simple; bh=pqkaR7wlh97Rk42BthGo7VYjx1B/ezD0pUqNNA+q+Lg=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=nfHMCRU6m6xn3BazpBbDQaLVQ8HtckO/sjK8ADJSJjmXbdhQMSYG1Y61F8yo6DyihmniGBZbmdqcnUL430HouPZw4t8w6xDK+peTT5WzLG961LnfsfxnHGkWz3Evqm+G+lpv/fpJCIwl+pSN/E5aLD1VlT+EAcaQ/QM/Dbq0ZNY= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 6F4C338CBF7A Received: by mail-wr1-f44.google.com with SMTP id ffacd0b85a97d-3a6d1369d4eso592365f8f.2 for ; Fri, 20 Jun 2025 11:20:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1750443609; x=1751048409; h=content-transfer-encoding:in-reply-to:content-language:from :references:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=M54cBwTRxCDKprW7bLAxwkysvpLfVfMb2lYbKKdan64=; b=FgCZjbYag/pWe/6URjQ7E+vhvA25YnpB72t6lrtPJAeUiNLJFSLeze+nC7Pyjk2hhB tTR/a7LcwdvF+FHBPnefhcanDLrANDggSdmtPApzeTpJeApZJtmqF7601YuJ033zn1aA OXHd57hj1ETmdvjYqZ/NBzI/A/+dq9a1iExe+NXSRQeUIgbrfS+W+Jgbkya8NXVGVQVv xb98FkXB/XlAc68xMw75TADiKdcCzyGuvy7oSOQQuGkf5KP4lbO9wGZOJOd71WKss+Iw 2xs7SetIABm/kj/p6yEat6r8x68EF++s+1ejuKAs7C/zFqNfyoqoXQBfLrVv/KdXcwPa v9ZQ== X-Forwarded-Encrypted: i=1; AJvYcCVNIKHVrSuu5MLze/3U4NT57ooOjLmjaz82sfKlIJZg0PPldLomBNK+QdwZKxYVvxXUI4Jgsa2zFroiPA==@sourceware.org X-Gm-Message-State: AOJu0YyN1X8t3VJZ/xYVGAtfL+h/zhEbPx28ZvQwO9Jq3qhB03d0s8+X We2YF3sildM8XOWTEqiUS+w4nJyKEIfCokqHGRwcs5WVx1bU6OZnoy68iIXXq6Dq X-Gm-Gg: ASbGnctdPRtNIj7N64wRwDZmzncfvO3v/zZ69jI+lBgXCA8UgJJOB44r2oVR7IFt3fW H4cvf/5uXvFDP57VTZKUgAmK5ENCkCEXZee4xDdNIvc2PgBaCVakCmee4KUzJCahumWEd8oVGq7 afB1cE1WAJAidNKh9UoQVcZflfOU+B7uCV11Jv/quYttjKhVRfoPpxGWvlNgcWAa0bIPYgd5Aiy EkpHC2BQX1MCVDxznlvQG9mvyPHdLdbPXU8MCfYRVjAOqWjRE8oka5xl1QQFJjPvC3/FZKNiRPG vq/q16TTNsAIIW8EBJdeuqUzKIppXtbR7tDqKmHKN118e/LqhkpTMRC8vHrrQF5E+n871Y6LRN+ ySaj1BICHdLE1ETiVtB6x25KvhPluYQ== X-Google-Smtp-Source: AGHT+IGbCOZNm9EZSxRz4gnMwHx0L1mTH5di3sZUAL9qqp8K9riX30bn8K1pZBKc95vtzrpQM02JXQ== X-Received: by 2002:a05:6000:3c7:b0:3a3:7ba5:9618 with SMTP id ffacd0b85a97d-3a6d12da112mr3340739f8f.29.1750443609140; Fri, 20 Jun 2025 11:20:09 -0700 (PDT) Received: from ?IPV6:2001:8a0:fac3:6d00:afcc:85d3:71a5:7570? ([2001:8a0:fac3:6d00:afcc:85d3:71a5:7570]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-453646fd642sm31511225e9.21.2025.06.20.11.20.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 20 Jun 2025 11:20:08 -0700 (PDT) Message-ID: Date: Fri, 20 Jun 2025 19:20:07 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 6/6] gdb/solib: C++ify solib_ops To: Simon Marchi , gdb-patches@sourceware.org References: <20250616193443.16703-1-simon.marchi@efficios.com> <20250616193443.16703-6-simon.marchi@efficios.com> From: Pedro Alves Content-Language: en-US In-Reply-To: <20250616193443.16703-6-simon.marchi@efficios.com> 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 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. > 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. We should be able to simplify such code a bit with C++23, with the new explicit object parameter syntax. > > 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". > > 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 > --- 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?