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 8B6053858D28 for ; Tue, 11 Apr 2023 01:49:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8B6053858D28 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 [10.0.0.11] (unknown [167.248.160.41]) (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 1785E1E0D2; Mon, 10 Apr 2023 21:49:03 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1681177743; bh=Bfyyvy3stcgzwjXLvotf2ugLV+hOk4MUGsqjpOQMtmc=; h=Date:Subject:To:References:From:In-Reply-To:From; b=cf8uw53r6vFvFy0pU5GqeqvPBVKDcFm3mz2r/AauYYpE71e3RgVfE7HSNBm+Jgxyj a3lAzIAUISVGrEqT3H5E/2rGRBXMNm2Yc3MUTo2JMQXJ+6KOBab6qeUVSCRIW3kPN8 pYK9kb1k19JULu+JbTZu4cJKNWz6W8RYhsKdoDVA= Message-ID: <0a1d7dee-3d2f-787a-133b-64fc3e7e2032@simark.ca> Date: Mon, 10 Apr 2023 21:49:01 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: [PATCH v4 02/13] core: Support fetching TARGET_OBJECT_X86_XSAVE_LAYOUT from architectures. Content-Language: en-US To: John Baldwin , gdb-patches@sourceware.org References: <20230318010905.14294-1-jhb@FreeBSD.org> <20230318010905.14294-3-jhb@FreeBSD.org> <775a2e6b-93fe-507a-b78a-991c7683c77a@simark.ca> <7678c952-45bc-2085-ff12-1c36622839fa@FreeBSD.org> From: Simon Marchi In-Reply-To: <7678c952-45bc-2085-ff12-1c36622839fa@FreeBSD.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-6.0 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/10/23 16:42, John Baldwin wrote: > On 4/6/23 12:28 PM, Simon Marchi wrote: >> On 3/17/23 21:08, John Baldwin wrote: >>> Add gdbarch_core_xfer_x86_xsave_layout to fetch the x86 XSAVE layout >>> structure from a core file. >>> >>> Current OS's do not export the offsets of XSAVE state components in >>> core dumps, so provide an i387_set_xsave_layout helper function to set >>> offsets based on known combinations of XCR0 masks and total state >>> sizes. Eventually when core dumps do contain this information this >>> function should only be used as a fall back for older core dumps. >> >> Not a big deal, but it's a bit odd to add the i387_set_xsave_layout >> function here, but not use it. I could see it being added with the >> first patch that uses and, and the current patch just adding the >> gdbarch_core_xfer_x86_xsave_layout gdbarch method + the call in >> corelow.c. > > Humm, the reason I added it here is that it is part of OS-independent > infrastructure that will later be used by OS-specific ABIs. Adding > this function (or other infrastructure) as part of the commit adding > FreeBSD amd64/i386 support seemed a bit odd to me. I wanted it to be > clear which parts are OS-specific vs not in the history in part so it > is easier for other OS's to add XSAVE support in the future. Perfect, makes sense. >> You can remove `invalid=True`, following 564cddf8edc7 ("gdbarch: make >> invalid=True the default for all Components"). >> >> I have yet to see how this is going to be implemented (in subsequent >> patches), but I wonder if we really need to pass use readbuf / offset / >> len in this API. Wouldn't it be possible to pass a pointer or reference >> to an x86_xsave_layout object, and have the arch fill it? I understand >> that this matches the taret_read interface (we have to shoehorn the >> x86_xsave_layout through that interface), but I don't think it needs to >> propagate to the gdbarch method. > > Hmm, passing down the readbuf, offset, len fields is consistent with other > gdbarch methods called from core_target::xfer_partial such as > gdbarch_core_xfer_shared_libraries and gdbarch_core_xfer_siginfo. > > That said, it would simplify the implementation in the architectures if > core_target::xfer_partial handled the partial reads and always read the > full thing from the gdbarch. In particular unlike the other above methods, > the thing being read is a fixed size. I'll make that change as I think > it will be nicer, thanks. I looked at these other methods and came to the same conclusion as you. For gdbarch_core_xfer_siginfo it doesn't really introduce any extra complexity, because we just pass down the offset/len parameters to bfd_get_section_contents. For gdbarch_core_xfer_shared_libraries, seeing that the sole implementation (windows_core_xfer_shared_libraries) just re-generates the whole XML string every time, I think we could simplify its interface as well, making it return just an std::string, and handle the partial read case in the target. Not important for what you're doing, I'm just thinking out loud. But now it makes me wonder what really belongs as an object in the xfer_partial interface, versus what belongs as just a regular target method. Clearly, the xfer partial interface is good to transfer some raw data where the transferred size can be smaller than expected. Like when you make a read/write syscall. But if the goal is to transfer a structure from one side of GDB to the other (like we do with x86_xsave_layout), wouldn't it be simpler to have a normal target method that returns that struct? Simon