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 5FE33385841E for ; Tue, 12 Mar 2024 16:24:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5FE33385841E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 5FE33385841E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1710260657; cv=none; b=NwW39SSsZ7O+yG2Hn7NpvGsMctB2vG1Cvd56m3Cojlb2Bk8bMdPBe64A2GxTS+JYsg12euPpB6192MITHJwc5L7t465OBKvw87GS0HycJQ9rQPr5RPLJQu6ALh2nPUQPzuTW2lYEaXyQ6QKX2tDjZQ8FdxdHgDgkxVuKjBXUDeA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1710260657; c=relaxed/simple; bh=YwVAmRh8EUGs+RtR87/HabvaQqw4qXSNZ3xJaJxU6YE=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=LwwD296txq3ibKQUwREB4m3ukf5wDpwJZX/l5vPTvCIt6gruoR0jLRLoKgtm28FOMf/Y7KYe8AUpojj7yXEA3e/45Tb80aROegRVY/EgmtRsKm46R1eZ5tqwZ8ffTcVoksGMNvyZFySxw9XNM+xTQgeN8Y5CNvF3E3EhZOAC5Do= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1710260655; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=/V1I2vihjB6VKFndTNRh2sYq4cpGfvpzHzXt4BwPwN8=; b=AoNcoGSYrVQik+bytbK0mY/xAQ8QjNCq4RyG1Yf1vYPiB/YRgckbmMftV5Dv5i51BJp/sE 2ZcpBcbTsTMTrXhH5ygKPv/TonOj7X0lqzMJjc87Li6iwUo6k2AX8V43c0pRDrNmUt6Xv+ N4LhmQqhGyZ9PvZxbvE69s3LUR22KM4= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-232-tp6u6Lz1NUe14ngRE9TPAg-1; Tue, 12 Mar 2024 12:24:13 -0400 X-MC-Unique: tp6u6Lz1NUe14ngRE9TPAg-1 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-412dcaf0bd6so24449725e9.0 for ; Tue, 12 Mar 2024 09:24:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1710260652; x=1710865452; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=/V1I2vihjB6VKFndTNRh2sYq4cpGfvpzHzXt4BwPwN8=; b=wMhyxankPr05PYuCPVmZuiwukyEeH/mPAve5RtoGOrJWYrIV3FplALBxZt2sIPguYR 7+LeSEJzrtY/ELXjGnQepMgAodPwi7VK/1SgDTEVYltz31BlNmoi537EM1GwEzEewdTe Ny28ZLytuPdAnb/sI2ZDzw/I5FKRnFkzNxW3Z0fJwC4VEaLmPrzMFkXOSYedkXrJj4xi vt3IfD9j1B23EdZtUfwdz4vZJ/Ssri/3nBPcyRahPagtX4N4ZnNXl5Y7j3169KBpv3oa kIvN0PnyzeN8ehrYUmJjQR1PPmDl1Wdy7ZdGVhoUHpFHCKhsuu9tkDGZhqZH7ggiwwR9 eTFw== X-Gm-Message-State: AOJu0YzmTIGQ5eh1a2UKlNK9MWNzQ5S8Tdib6HNMMy6cq7mLqDgiXatw w2IfaqDIzrMHaNunMAJ5jsjobJ7vjn3sOQVQbEvaqv6MeYv6maPQ44qfipcswCrmJ8TTGvtB8dR Ba7X4aE/1s42D9FiY4G/gV+4fFl7CLTVrbuHBBMBPXZq+tctS6+grvGfXPKp0CjMtC/M= X-Received: by 2002:a05:600c:1d12:b0:413:2ea4:1731 with SMTP id l18-20020a05600c1d1200b004132ea41731mr3701108wms.15.1710260651883; Tue, 12 Mar 2024 09:24:11 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGXUVEsBiDN7H6/QSTCjXvynC5ddnBAiWJTUEXARiecgR2bdvs4wKaf3vZx9UG/twrY81dVoA== X-Received: by 2002:a05:600c:1d12:b0:413:2ea4:1731 with SMTP id l18-20020a05600c1d1200b004132ea41731mr3701094wms.15.1710260651468; Tue, 12 Mar 2024 09:24:11 -0700 (PDT) Received: from [192.168.0.129] (ip-94-112-227-180.bb.vodafone.cz. [94.112.227.180]) by smtp.gmail.com with ESMTPSA id h15-20020a05600c350f00b004131d2307e7sm12892870wmq.12.2024.03.12.09.24.10 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 12 Mar 2024 09:24:11 -0700 (PDT) Message-ID: Date: Tue, 12 Mar 2024 17:24:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 3/4] gdb: Migrate frame unwinders to use C++ classes To: Tom Tromey Cc: gdb-patches@sourceware.org References: <20240306125135.766567-1-blarsen@redhat.com> <20240306125135.766567-4-blarsen@redhat.com> <87plw4r6ti.fsf@tromey.com> From: Guinevere Larsen In-Reply-To: <87plw4r6ti.fsf@tromey.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 08/03/2024 18:07, Tom Tromey wrote: >>>>>> Guinevere Larsen writes: >> Frame unwinders have historically been a structure populated with >> callback pointers, so that architectures (or other specific unwinders) >> could install their own way to handle the inferior. However, since >> moving to c++, we could use polymorphism to get the same functionality >> in a more readable way. Polymorphism also makes it simpler to add new >> functionality to all frame unwinders, since all that's required is >> adding it to the base class. >> As part of the changes to add support to disabling frame unwinders, >> this commit makes the first baby step in using polymorphism for the >> frame unwinders, by making frame_unwind a virtual class, and adds a >> couple of new classes. The main class added is frame_unwind_legacy, >> which works the same as the previous structs, using function pointers >> as callbacks. This class was added to allow the transition to happen >> piecemeal. New unwinders should instead follow the lead of the other >> classes implemented. >> 2 of the others, frame_unwind_python and frame_unwind_trampoline, were added >> because it seemed simpler at the moment to do that instead of reworking >> the dynamic allocation to work with the legacy class, and can be used as >> an example to future implementations. >> /* AArch64 prologue unwinder. */ >> -static frame_unwind aarch64_prologue_unwind = >> -{ >> +static frame_unwind_legacy aarch64_prologue_unwind ( > Some of these should probably be 'const'... not really your problem but > the patch points it out. Sure, I can do this. > >> +/* See frame-unwind.h. */ >> + >> +enum unwind_stop_reason >> +frame_unwind::stop_reason (const frame_info_ptr &this_frame, >> + void **this_prologue_cache) const >> +{ >> + return default_frame_unwind_stop_reason (this_frame, this_prologue_cache); >> +} >> + > It looks like all of these methods here could just be inline in the > header. > >> -struct frame_unwind >> +class frame_unwind >> { >> - const char *name; >> +private: > class defaults to private. > >> + frame_unwind (const char *n, frame_type t, frame_unwind_class c, >> + const struct frame_data *d) >> + : m_name (n), m_type (t), m_unwinder_class (c), m_unwind_data (d) { } > This looks weird. Any specific reason why? If the problem is the single-letter parameters, I can change that (it was from an earlier iteration), but other than that, I'm not sure what you find weird in this. > >> + /* Calculate the ID of the given frame using this unwinder. */ >> + virtual void this_id (const frame_info_ptr &this_frame, >> + void **this_prologue_cache, >> + struct frame_id *id) const >> + { >> + error (_("No method this_id implemented for unwinder %s"), m_name); >> + } > Why not pure virtual? not all classes have these methods defined, and if we make it pure virtual, all classes will need to implement them (or we get linker errors), so it seems easier to implement a base error in here. > >> + frame_unwind_legacy (const char *n, frame_type t, frame_unwind_class c, >> + frame_unwind_stop_reason_ftype *sr, >> + frame_this_id_ftype *ti, >> + frame_prev_register_ftype *pr, >> + const struct frame_data *ud, >> + frame_sniffer_ftype *s, >> + frame_dealloc_cache_ftype *dc = nullptr, >> + frame_prev_arch_ftype *pa = nullptr) >> + : frame_unwind (n, t, c, ud), stop_reason_p (sr), >> + this_id_p (ti), prev_register_p (pr), sniffer_p (s), >> + dealloc_cache_p (dc), prev_arch_p (pa) { } > I wonder if making this constexpr would help avoid running a ton of > constructors at startup time. according to cppreference, constexpr constructors must not have virtual base classes, so unfortunately it isn't possible =/ > > Alternatively maybe an approach would be to overload > frame_unwind_append_unwinder to automatically instantiate the frame_unwind_legacy > object given the C-style struct. I think this would delay > running the constructor until the gdbarch is initialized, which > in many cases would be never. I tried compiling without this patch to see what's GDB startup time, then checked the startup time with this patch. In my system, I can't see a difference in startup time with the "time" command (so 0.01s precision). I could try to find a way to delay instantiation to when the unwinder is installed, so we avoid unnecessary constructor calls. >> + = obstack_new >> + (gdbarch_obstack(newarch), (const struct frame_data *) newarch); >> + > I think it's better to derive from allocate_on_obstack instead. This > helps prevent errors. I am only allocating things on the obstack because I couldn't figure out a better solution to dynamically allocate the unwinder in a way that wouldn't leak the memory if gdbarch was de-allocated. If you are dead set on delaying constructing the objects at startup, I would probably make gdbarch store unique_ptrs instead of raw pointers, or some other similar thing. > > Also a missing space. > > Tom > -- Cheers, Guinevere Larsen She/Her/Hers