From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.freebsd.org (mx2.freebsd.org [IPv6:2610:1c1:1:606c::19:2]) by sourceware.org (Postfix) with ESMTPS id 4BD1D3858C2C for ; Tue, 11 Apr 2023 16:19:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4BD1D3858C2C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=FreeBSD.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits)) (Client CN "mx1.freebsd.org", Issuer "R3" (verified OK)) by mx2.freebsd.org (Postfix) with ESMTPS id 4PwrdQ3z6yz4D4y; Tue, 11 Apr 2023 16:19:22 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4PwrdQ38vdz3MMg; Tue, 11 Apr 2023 16:19:22 +0000 (UTC) (envelope-from jhb@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1681229962; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=h1REDMyuZ0aDy2as3a1YAFs76kfPlKX4/auqXzbqXLY=; b=XfimAFGQ845xIMZcHJDsngMGSbLhownCo68OgFiJDKiRX4yPtcgm5sdi9qrncooUK3/wW2 kJGfr1OXmXN8Q+P/i3ZmFgr/LzSohDrzrAJXgCZ8bQSMHCk5kZDY0pZVeCcHh3uqeIoDFB H0kDsWfD5vHrMWz9Bk2D94S6U9l8RQ4S8qkiSh88fWLHfsv286wf0w1OskF8m01qF/wQ0F Qq3Toytra/8PSHrLo4qultnF9zxWuLyvMkS79FV4c9Yu+7sXOo39CcWLrEW4gU/W08hkNJ Kji8fp2H1M2R4tHMqWtst0itMgcTCO5ZfMnzIl/0EOWZPAhwFSyrusQaACjf8w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1681229962; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=h1REDMyuZ0aDy2as3a1YAFs76kfPlKX4/auqXzbqXLY=; b=k8cV8pfY0sl1xbfxwflEXfIso49zNgQxt8aqZCL5PkTVpf+XPN2+M5NaUoJWHv1g+zz9Ri mwf+N9Nf6YVp/1Mm54u26535l22wsLpf3+TijZkFfKcGhCQWIoltUOvQfn4jiOnzfBCkhk V/gVEd+3jPXUd/RXiaZCNqk3kEaYnDUzGdYz5l2FppS9DyuWewwDA8bNLWErQ7lJ9Tnv+N h5cp5X/NvFeP0MC98asdstsjH7cDY+K6UipFC/Oj3Gy7LY8frspUpltCIA4eSrURPBi9/0 XOz53G2iPY8S24FE/xUaENJAfdWFe3QXR1sKY/fsdXJGr80vSznqtMMsVu+l0A== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1681229962; a=rsa-sha256; cv=none; b=C0JiXnxsSx0/nuXvBatFqdskeSChrlk/DWCcV3QGhdyaMiCkkFKKpEc6GWRp94Qc4eQCGa vIp8zFMfFYx6Fix8sFE+4rKJKV/E8K0PaRagzCAMZRZN5IS2BjTHMJyJqI+R8ZREsGd876 l1TN02x3ZtzHocqdoLan8RpHMBIferiJCNh+XQBT9tTa5EOxcaCt6OYq+9BLRnkjl4CRXp IpUmeUl4fxWz5G86ZiNnQlt5208kidYj+7tFNY2Y3GT0s3J7teew2ScSxswKXtoOScD+2L hr0BPbsYBZ4DEwhEoL2vB4rdFBSiPOKn9VTrNFvDicTUfxQZnVo678cpWwRuag== Received: from [IPV6:2601:648:8680:16b0:14bb:55bb:f654:b139] (unknown [IPv6:2601:648:8680:16b0:14bb:55bb:f654:b139]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 4PwrdQ03GJz13JT; Tue, 11 Apr 2023 16:19:21 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: <02d796f2-de80-b3c4-8279-38c750286446@FreeBSD.org> Date: Tue, 11 Apr 2023 09:19:21 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.9.1 Content-Language: en-US To: Simon Marchi , gdb-patches@sourceware.org References: <20230318010905.14294-1-jhb@FreeBSD.org> <20230318010905.14294-7-jhb@FreeBSD.org> <448537fa-fff2-4bf9-e01b-1c01e5cb48dd@simark.ca> <198191af-c026-9e6a-767c-3418e8174959@simark.ca> From: John Baldwin Subject: Re: [PATCH v4 06/13] gdb: Support XSAVE layouts for the current host in the FreeBSD x86 targets. In-Reply-To: <198191af-c026-9e6a-767c-3418e8174959@simark.ca> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,SPF_HELO_NONE,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: On 4/10/23 7:23 PM, Simon Marchi wrote: > > > On 4/10/23 17:27, John Baldwin wrote: >> On 4/6/23 1:18 PM, Simon Marchi wrote: >>>> @@ -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? >> >> Yes. Note that the default value of xsave_layout (all zeroes) will result >> in this failing to read the object if it hasn't been called (rather than >> undefined behavior). >> >>> 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. >> >> Well, part of the trick here is that probe_xsave_layout takes a pid >> argument that it passes to ptrace () to fetch the layout, but at the >> point of xfer_partial I don't necessarily have a pid. It's true that >> the only callers (the target ::read_description methods) all use >> inferior_ptid's pid, so I could perhaps use inferior_ptid directly, >> but that seems somewhat dubious. > > Using inferior_ptid would probably be fine... I think? It's how targets > know which thread's memory to read from or write to, for the > TARGET_OBJECT_MEMORY object. The question is, will all calls to > xfer_partial with the TARGET_OBJECT_X86_XSAVE_LAYOUT object be done with > inferior_ptid set to a stopped thread that you can used to do the ptrace > query? > > Ok, that made me think about an unlikely (but still possible > scenario) where the read_description target method can be called with > inferior_ptid == null_ptid: > > (gdb) target native # push the native target > (gdb) unset tdesc filename > > I think the code as it is in this patch would try to call ptrace with > pid 0. > > The approach of fetching the xsave layout using ptrace has this > particularity that you need a stopped thread to access it. I'm > wondering, is there an advantage in fetching the xsave layout using > ptrace instead of the cpuid method? The latter has the advantage of > working at any time. Well, you need the value of xcr0 as well as the total size, though it is possible to execute XGETBV in userspace I believe. I had wanted to ensure we were using the same mask ptrace is going to export vs assuming that the mask of the running GDB process will match the mask of all target processes. I guess in practice we are assuming that though via the caching. Note that the read_description methods for both Linux and FreeBSD already use ptrace for other things. Both use ptrace to decide if the inferior is using a 64-bit vs 32-bit tdesc. For 32-bit both use ptrace to determine if there is support for a custom ptrace op to fetch SSE/XMM register state (PTRACE_GETFPXREGS on Linux, PT_GETXMMREGS on FreeBSD), so this case is already quite broken. >>>> + >>>> + 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. >> >> Oh, that is more correct, yes. > > Ok, I think there was the same issue for the Linux class hierarchy too, > I don't remember if I pointed it out. I've fixed that one locally, too. >>>> 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 >>>> @@ -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). >> >> Yes, that was an old bug, I can split that out into a separate patch though. > > Bah... as you wish. You can push an obvious patch if you'd like to keep > this one really clean. Ok. -- John Baldwin