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 0D09A3858D28 for ; Wed, 3 May 2023 17:14:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0D09A3858D28 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) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id BB5FE1E0D6; Wed, 3 May 2023 13:14:54 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1683134094; bh=FwVoza37G/uxhYhJttgtzADyH+RLcL2jwN1JYJg6F2s=; h=Date:Subject:To:References:From:In-Reply-To:From; b=cKWWmserXKGGQnYYZ9i3X0T70iDN4S258cl9tBzXsjCC/URZFBgkVwee1/D8x3TEZ SPB2c72Wz2X+mr+Ma3nn7HJE37heMXU3x4R3JM3hm/F7w43pPoHNKQJ3g+RSA88vKy RXl+mH0JqqYxOGgXNatxO2bpMNPqFR2XXU8iLiB0= Message-ID: Date: Wed, 3 May 2023 13:14:54 -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: <20230427210113.45380-10-jhb@FreeBSD.org> 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: > @@ -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? Also, what does "validate" mean, what happens if the thing we validate is not valid? Otherwise: Approved-By: Simon Marchi Simon