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 0BE8E3858D28 for ; Thu, 6 Apr 2023 20:18:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0BE8E3858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [172.16.0.192] (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id C00C71E110; Thu, 6 Apr 2023 16:18:08 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1680812288; bh=Y6BJLf5/1QgVjU4bEy2/mtxqErOaH0cFrmKwa6U73ow=; h=Date:Subject:To:References:From:In-Reply-To:From; b=aHBDGapEztf23TwL9ptHB3MZyvM3Dl2KHBLDunf6avUFpKaIG6hWseN5TNQaEgSaU NuC2LdV+QIrKiuMAEymy6jkmp7Dow0BJFy4fgYpuCcogLFczCwf3WPhgTtCpnwdH34 7rmR3EJmAxhoEdx71mOodmWgR0wcPbe8w2hCLZTI= Message-ID: <448537fa-fff2-4bf9-e01b-1c01e5cb48dd@simark.ca> Date: Thu, 6 Apr 2023 16:18:08 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.1 Subject: Re: [PATCH v4 06/13] gdb: Support XSAVE layouts for the current host in the FreeBSD x86 targets. Content-Language: fr To: John Baldwin , gdb-patches@sourceware.org References: <20230318010905.14294-1-jhb@FreeBSD.org> <20230318010905.14294-7-jhb@FreeBSD.org> From: Simon Marchi In-Reply-To: <20230318010905.14294-7-jhb@FreeBSD.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: > @@ -43,3 +46,51 @@ x86_fbsd_nat_target::low_new_fork (ptid_t parent, pid_t child) > child_state = x86_debug_reg_state (child); > *child_state = *parent_state; > } > + > +#ifdef PT_GETXSTATE_INFO > +/* Implement the "xfer_partial" target_ops method. */ > + > +enum target_xfer_status > +x86_fbsd_nat_target::xfer_partial (enum target_object object, > + const char *annex, gdb_byte *readbuf, > + const gdb_byte *writebuf, > + ULONGEST offset, ULONGEST len, > + ULONGEST *xfered_len) > +{ > + switch (object) > + { > + case TARGET_OBJECT_X86_XSAVE_LAYOUT: > + if (xsave_layout.sizeof_xsave == 0) > + return TARGET_XFER_E_IO; I'm wondering why you don't call probe_xsave_layout before using xsave_layout here. I guess because you assume that the child class will have called probe_xsave_layout at some point before? To make that more robust, could we access xsave_layout through a method like: const x86_xsave_layout &xsave_layout (); which would take care of the caching? Callers wouldn't have to worry about calling probe_xsave_layout before. There could be another method to access xsave_info. > + > + if (offset > sizeof (xsave_layout)) > + return TARGET_XFER_E_IO; > + > + if (offset + len > sizeof (xsave_layout)) > + len = sizeof (xsave_layout) - offset; > + > + memcpy (readbuf, ((gdb_byte *) &xsave_layout) + offset, len); > + *xfered_len = len; > + return len == 0 ? TARGET_XFER_EOF : TARGET_XFER_OK; > + default: > + return fbsd_nat_target::xfer_partial (object, annex, readbuf, writebuf, > + offset, len, xfered_len); Since this class inherits from x86bsd_nat_target, should this technically call x86bsd_nat_target::xfer_partial? I don't really know, I've never used this funky inheritance pattern myself. And it won't change anything in practice right now, because not target between this one and fbsd_nat_target implements xfer_partial. > + } > +} > + > +void > +x86_fbsd_nat_target::probe_xsave_layout (pid_t pid) > +{ > + if (xsave_probed) > + return; > + > + xsave_probed = true; > + > + if (ptrace (PT_GETXSTATE_INFO, pid, (PTRACE_TYPE_ARG3) &xsave_info, > + sizeof (xsave_info)) != 0) > + return; > + if (xsave_info.xsave_len != 0) > + x86_fetch_xsave_layout (xsave_info.xsave_mask, xsave_info.xsave_len, > + xsave_layout); > +} > +#endif > diff --git a/gdb/x86-fbsd-nat.h b/gdb/x86-fbsd-nat.h > index 0a1308f3584..1b5cdfc1a9b 100644 > --- a/gdb/x86-fbsd-nat.h > +++ b/gdb/x86-fbsd-nat.h > @@ -20,6 +20,11 @@ > #ifndef X86_FBSD_NAT_H > #define X86_FBSD_NAT_H > > +#include > + > +#ifdef PT_GETXSTATE_INFO > +#include "gdbsupport/x86-xstate.h" > +#endif > #include "fbsd-nat.h" > #include "x86-bsd-nat.h" > > @@ -27,10 +32,32 @@ > > class x86_fbsd_nat_target : public x86bsd_nat_target > { > + void low_new_fork (ptid_t parent, pid_t child) override; > + > +public: > bool supports_stopped_by_hw_breakpoint () override > { return true; } Is is on purpose that supports_stopped_by_hw_breakpoint is moved to the public section? I guess so, because it is public in reality (inherited from target_ops). > > - void low_new_fork (ptid_t parent, pid_t child) override; > +#ifdef PT_GETXSTATE_INFO > + enum target_xfer_status xfer_partial (enum target_object object, > + const char *annex, > + gdb_byte *readbuf, > + const gdb_byte *writebuf, > + ULONGEST offset, ULONGEST len, > + ULONGEST *xfered_len) override; > + > +protected: > + void probe_xsave_layout (pid_t pid); > + > + struct ptrace_xstate_info xsave_info; > + x86_xsave_layout xsave_layout; > + > +private: > + bool xsave_probed; Private and protected members should be prefixed with m_. > +#endif > }; > > +#ifdef PT_GETXSTATE_INFO > +#endif I guess these last two lines are not needed? Simon