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 264993858D28 for ; Tue, 11 Apr 2023 16:46:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 264993858D28 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.146] (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 739011E0D3; Tue, 11 Apr 2023 12:46:05 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1681231565; bh=wN8wxsooRbSTrjxrTBo5Gx0sQtfctF52c7NgefJZw9E=; h=Date:Subject:To:References:From:In-Reply-To:From; b=rjQBMCuNSrV0MBd/Y/NqyE49fuwh1pI0ACQbZx+9gbydmU/BLoJBH/yyI8Hq/olpC 6Fp+ouP92ku8i01KrbXn38EF4kCk65Jv27yDjhpYl2XHjQnpwMKi8BgjM/mWLqWUfn ja5iO7nvRKjTZakO/SZU7PflyX5mju5aWYfxbP9A= Message-ID: Date: Tue, 11 Apr 2023 12:46:05 -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> <448537fa-fff2-4bf9-e01b-1c01e5cb48dd@simark.ca> <198191af-c026-9e6a-767c-3418e8174959@simark.ca> <02d796f2-de80-b3c4-8279-38c750286446@FreeBSD.org> From: Simon Marchi In-Reply-To: <02d796f2-de80-b3c4-8279-38c750286446@FreeBSD.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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: On 4/11/23 12:19, John Baldwin wrote: > 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. Who sets xcr0, the kernel or the userspace? In theory could we have xcr0 values that differ per process? Could the xcr0 value for a process change over time? To be clear, I'm just asking to understand better, I don't suggest to support these cases if they don't happen in reality. > 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. Heh, you're right, I ran those commands on x86 Linux and didn't even notice the error message! (gdb) target native Done. Use the "run" command to start a process. (gdb) unset tdesc filename Couldn't get CS register: No such process. So yeah, perhaps make sure this corner case is relatively correctly handled. That probably means skipping over the ptrace check if inferior_ptid is null_ptid, and make sure you don't cache that value (so we actually probe it next time around). Simon