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 8A3F23858C78 for ; Wed, 3 May 2023 17:20:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8A3F23858C78 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.170] (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 06DD21E0D6; Wed, 3 May 2023 13:20:38 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1683134438; bh=+U5U4oyYoCW2rS1ipMHuHs8UTsNUzW0r8luZ+uc1Qqc=; h=Date:Subject:To:References:From:In-Reply-To:From; b=ufQmVDWSDvXfopdCxLNw4Lw2AkTSWYVOrKk6z5WHNzTMKWG6hbrC2EbD9ny5GBakp czCpEFh6vptgdaR54C+Tqguuf5H012D7N89nhvf3ZCuYSLjeG413tPE5QatssqXOWk go29+GqJSwZ7EYZVRByV2dGSyIfQOFxWLqkoNvxQ= Message-ID: <70b03e12-9491-33ba-85d3-688ef1f2c350@simark.ca> Date: Wed, 3 May 2023 13:20:37 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.1 Subject: Re: [PATCH v5 09/19] gdb: Update x86 FreeBSD architectures to support XSAVE layouts. Content-Language: fr To: John Baldwin , gdb-patches@sourceware.org References: <20230427210113.45380-1-jhb@FreeBSD.org> <20230427210113.45380-10-jhb@FreeBSD.org> From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.9 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,T_SCC_BODY_TEXT_LINE 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 5/3/23 13:14, Simon Marchi via Gdb-patches wrote: >> @@ -285,7 +291,9 @@ i386fbsd_core_read_description (struct gdbarch *gdbarch, >> struct target_ops *target, >> bfd *abfd) >> { >> - return i386_target_description (i386fbsd_core_read_xcr0 (abfd), true); >> + x86_xsave_layout layout; >> + return i386_target_description (i386_fbsd_core_read_xsave_info (abfd, layout), >> + true); > > Reading this gives me some questions. Just thinking out loud, nothing > necessarily actionable at the moment > > I found it strange that i386_fbsd_core_read_xsave_info fills an > x86_xsave_layout object that we don't use. We get xcr0 to generate an > appropriate target description here, and later we'll call > target_fetch_x86_xsave_layout (when initializing the gdbarch) to get the > x86_xsave_layout and save it in the i386_gdbarch_tdep object. I'm > wondering if, design-wise, this means that the target_desc object should > carry the xsave layout information. It would be saved in the tdesc > here, and i386_gdbarch_init would just get it from the tdesc. > > It's probably not as simple as that, since target descriptions are > transferred as XML from remote targets, and you still have to consider > older target descriptions that wouldn't include that information. But > I'm just trying to think what the ideal design would be. > >> diff --git a/gdb/i386-fbsd-tdep.h b/gdb/i386-fbsd-tdep.h >> index cb991af9e49..f96c00d45eb 100644 >> --- a/gdb/i386-fbsd-tdep.h >> +++ b/gdb/i386-fbsd-tdep.h >> @@ -20,10 +20,16 @@ >> #ifndef I386_FBSD_TDEP_H >> #define I386_FBSD_TDEP_H >> >> +#include "gdbsupport/x86-xstate.h" >> #include "regset.h" >> >> -/* Get XSAVE extended state xcr0 from core dump. */ >> -extern uint64_t i386fbsd_core_read_xcr0 (bfd *abfd); >> +/* Validate and fetch XSAVE extended state xcr0 and extended area >> + layout from core dump. */ >> +uint64_t i386_fbsd_core_read_xsave_info (bfd *abfd, x86_xsave_layout &layout); > > I was a bit confused when I read the comment above for the first time. > Can you rephrase it to make it clear that the function returns the XSAVE > extended state, and fills LAYOUT? Sorry, I meant "returns xcr0" here. Simon