From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x32a.google.com (mail-wm1-x32a.google.com [IPv6:2a00:1450:4864:20::32a]) by sourceware.org (Postfix) with ESMTPS id 6C0243858D33 for ; Mon, 27 Nov 2023 04:36:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6C0243858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=adacore.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 6C0243858D33 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::32a ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701059795; cv=none; b=wX2X8zWE16VJQVPz39Npa2pXkSIWVrL7KHEh1Wmc8MreUuLmNukPuVd0+UMtD+Gjd614YEc5LX2ZSdOO/l8qimcyUkygu12zFJDGGum6uknYqr2jABPSrT3Ma9/JEy4kOEyr85Rdo2HVMe5yw5ld+OmB+APd2JFeQJMpgb4NXFs= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701059795; c=relaxed/simple; bh=qMW7WGIAyzQ5MCKy+WjEgjEfHV/aTJY0Fg7UuST3itM=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=x6p6ynEi0COpqX5CEFeAinIrr4ntXi0eBL8X15uHEBFjltRYvhyimslsugNpaoV97TavvEeydTpou/5cB/qMiQrIHMOEgq6nIXXR9jjULX3sx4qfoacl+KS8Or7b6KrRlf+3+ZwWEE+kkf6dXNSxaQaqdUQQXIZDquOdmDHGX4w= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wm1-x32a.google.com with SMTP id 5b1f17b1804b1-40b34563987so20691445e9.1 for ; Sun, 26 Nov 2023 20:36:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=adacore.com; s=google; t=1701059792; x=1701664592; darn=sourceware.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=y4STl4OZ43VOWkDnjLXcDfEWyOh5/6LYIm3VAKM4cVY=; b=lBHpSSTCudDvXyK/0bhSX/RDbjdK28ksYwwfY3UcwpJTKwGlYU23puzTG787YL3+rD Wx7bZ77p/5FGCj3uJcRmugLSX+XphWThHF0gR+OXk9Uvz2exvJYQdGTlYVbQKhzJOV+z PEiDtsrKxwMmchsjHHWthLV+w8HL5hDMSS8y7J3R+0Ip2+WhdmPhY0vZ68PwnhkdXusC 4YzeT3vlojD6BTcgfR2+sw72T4NXNqfXL8KOj/VqxA7miFZ2lhs5iMGqcgoLrWnI7diS UIYH1CacRsA5LS2M6fVw2J6mYP7o47IgQAsjJ5VGB016zqLZ70Sbe8uRFtRXLu//OjNV LOLQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701059792; x=1701664592; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=y4STl4OZ43VOWkDnjLXcDfEWyOh5/6LYIm3VAKM4cVY=; b=FZNWoSBfJVf8jNUwH3ElkEfE/v6fwNlDaovsw4ZoEZ1qSLouUpVbBe7cmxM4ws8gav 1wiiC9O9AGyrjg2byp7HMUXbmWKv+1nm/BUVvWLO+HmOCRe92VIOPKiDvQLt1gE9/o4v wt7zJRCV2bnR6glKQLX7A3U33M3/zRvtAzbgQERpQ92Lzx0k/Wr2bxA9ZlIR9cEyTNUA 1ndukpWhC0yWVHyAJPkP4wvlwGG0kZVbBjW8LQtneZVUTtNj16S4MgyldCytcfqiGwVY gLFNP30ePqldLVcm+1jO/tg87MLHN0qr/Vmo3ZUGUIMP7g/geJqMfBNVWBKdcWlecvQm FVOw== X-Gm-Message-State: AOJu0Ywq4L1Ncm0Pat+Q8t3NySMUc3iXtVYg3qylMZhObVZwt4un+hmO EQ+L2UiIV09hpgXfiQUc2Ch8 X-Google-Smtp-Source: AGHT+IFnbapAEDy+Yn6xswEJ7+tjURyxKrAL6XmK8ku8E6q9zB19WQqr8rRW0CbBB6PyHx4FD6sF1w== X-Received: by 2002:a05:600c:4507:b0:406:44fc:65c9 with SMTP id t7-20020a05600c450700b0040644fc65c9mr8679559wmo.8.1701059791863; Sun, 26 Nov 2023 20:36:31 -0800 (PST) Received: from takamaka.gnat.com (lfbn-reu-1-488-54.w92-130.abo.wanadoo.fr. [92.130.77.54]) by smtp.gmail.com with ESMTPSA id fj13-20020a05600c0c8d00b00407b93d8085sm12984200wmb.27.2023.11.26.20.36.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 26 Nov 2023 20:36:31 -0800 (PST) Received: by takamaka.gnat.com (Postfix, from userid 1000) id 3B57D81CA9; Mon, 27 Nov 2023 08:36:29 +0400 (+04) Date: Mon, 27 Nov 2023 08:36:29 +0400 From: Joel Brobecker To: John Baldwin Cc: gdb-patches@sourceware.org, Joel Brobecker Subject: Re: [PATCH] i386: Use a fallback XSAVE layout for remote targets Message-ID: References: <20231121220931.48497-1-jhb@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20231121220931.48497-1-jhb@FreeBSD.org> X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,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: Hi everyone, Just a quick note that the 14.1 release is currently being put on hold pending this initial resolution (thanks John for the patch!). On Tue, Nov 21, 2023 at 02:09:29PM -0800, John Baldwin wrote: > If a target provides a target description including registers from the > XSAVE extended region, but does not provide an XSAVE layout, use a > fallback XSAVE layout based on the included registers. This fallback > layout matches GDB's behavior in earlier releases which assumes the > layout from Intel CPUs. > > This fallback layout is currently only used for remote targets since > native targets which support XSAVE provide an explicit layout derived > from CPUID. > > PR gdb/30912 > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30912 I don't know about the XSAVE area, but still still skimmed the patch, and it looked reasonable to me - in particular, I don't think it can make things worse, since all of it is conditioned on cases where the target does not provide the XSAVE layout. One minor formatting nit I found... > --- > gdb/i386-tdep.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++ > gdb/i387-tdep.c | 48 +++++++++++++++++++++++++++++++ > gdb/i387-tdep.h | 5 ++++ > 3 files changed, 128 insertions(+) > > diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c > index f5ff55de47a..8efd8584fcd 100644 > --- a/gdb/i386-tdep.c > +++ b/gdb/i386-tdep.c > @@ -8297,6 +8297,72 @@ i386_floatformat_for_type (struct gdbarch *gdbarch, > return default_floatformat_for_type (gdbarch, name, len); > } > > +/* Compute an XCR0 mask based on a target description. */ > + > +static uint64_t > +i386_xcr0_from_tdesc (const struct target_desc *tdesc) > +{ > + if (! tdesc_has_registers (tdesc)) > + return 0; > + > + const struct tdesc_feature *feature_core; > + > + const struct tdesc_feature *feature_sse, *feature_avx, *feature_mpx, > + *feature_avx512, *feature_pkeys; ... there. Should the second line be indented 2 spaces relative to the start of the first line? Nothing else past this. > + /* Get core registers. */ > + feature_core = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.core"); > + if (feature_core == NULL) > + return 0; > + > + /* Get SSE registers. */ > + feature_sse = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.sse"); > + > + /* Try AVX registers. */ > + feature_avx = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.avx"); > + > + /* Try MPX registers. */ > + feature_mpx = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.mpx"); > + > + /* Try AVX512 registers. */ > + feature_avx512 = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.avx512"); > + > + /* Try PKEYS */ > + feature_pkeys = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.pkeys"); > + > + /* The XCR0 bits. */ > + uint64_t xcr0 = X86_XSTATE_X87; > + > + if (feature_sse) > + xcr0 |= X86_XSTATE_SSE; > + > + if (feature_avx) > + { > + /* AVX register description requires SSE register description. */ > + if (!feature_sse) > + return 0; > + > + xcr0 |= X86_XSTATE_AVX; > + } > + > + if (feature_mpx) > + xcr0 |= X86_XSTATE_MPX_MASK; > + > + if (feature_avx512) > + { > + /* AVX512 register description requires AVX register description. */ > + if (!feature_avx) > + return 0; > + > + xcr0 |= X86_XSTATE_AVX512; > + } > + > + if (feature_pkeys) > + xcr0 |= X86_XSTATE_PKRU; > + > + return xcr0; > +} > + > static int > i386_validate_tdesc_p (i386_gdbarch_tdep *tdep, > struct tdesc_arch_data *tdesc_data) > @@ -8508,6 +8574,15 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > > x86_xsave_layout xsave_layout = target_fetch_x86_xsave_layout (); > > + /* If the target did not provide an XSAVE layout but the target > + description includes registers from the XSAVE extended region, > + use a fallback XSAVE layout. Specifically, this fallback layout > + is used when writing out a local core dump for a remote > + target. */ > + if (xsave_layout.sizeof_xsave == 0) > + xsave_layout > + = i387_fallback_xsave_layout (i386_xcr0_from_tdesc (info.target_desc)); > + > /* If there is already a candidate, use it. */ > for (arches = gdbarch_list_lookup_by_info (arches, &info); > arches != NULL; > diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c > index 47667da21c7..b9c9e476e93 100644 > --- a/gdb/i387-tdep.c > +++ b/gdb/i387-tdep.c > @@ -987,6 +987,54 @@ i387_guess_xsave_layout (uint64_t xcr0, size_t xsave_size, > return true; > } > > +/* See i387-tdep.h. */ > + > +x86_xsave_layout > +i387_fallback_xsave_layout (uint64_t xcr0) > +{ > + x86_xsave_layout layout; > + > + if (HAS_PKRU (xcr0)) > + { > + /* Intel CPUs supporting PKRU. */ > + layout.avx_offset = 576; > + layout.bndregs_offset = 960; > + layout.bndcfg_offset = 1024; > + layout.k_offset = 1088; > + layout.zmm_h_offset = 1152; > + layout.zmm_offset = 1664; > + layout.pkru_offset = 2688; > + layout.sizeof_xsave = 2696; > + } > + else if (HAS_AVX512 (xcr0)) > + { > + /* Intel CPUs supporting AVX512. */ > + layout.avx_offset = 576; > + layout.bndregs_offset = 960; > + layout.bndcfg_offset = 1024; > + layout.k_offset = 1088; > + layout.zmm_h_offset = 1152; > + layout.zmm_offset = 1664; > + layout.sizeof_xsave = 2688; > + } > + else if (HAS_MPX (xcr0)) > + { > + /* Intel CPUs supporting MPX. */ > + layout.avx_offset = 576; > + layout.bndregs_offset = 960; > + layout.bndcfg_offset = 1024; > + layout.sizeof_xsave = 1088; > + } > + else if (HAS_AVX (xcr0)) > + { > + /* Intel and AMD CPUs supporting AVX. */ > + layout.avx_offset = 576; > + layout.sizeof_xsave = 832; > + } > + > + return layout; > +} > + > /* Extract from XSAVE a bitset of the features that are available on the > target, but which have not yet been enabled. */ > > diff --git a/gdb/i387-tdep.h b/gdb/i387-tdep.h > index e149e30e52e..f939fb9e393 100644 > --- a/gdb/i387-tdep.h > +++ b/gdb/i387-tdep.h > @@ -147,6 +147,11 @@ extern void i387_supply_fxsave (struct regcache *regcache, int regnum, > extern bool i387_guess_xsave_layout (uint64_t xcr0, size_t xsave_size, > x86_xsave_layout &layout); > > +/* Compute an XSAVE layout based on the XCR0 bitmask. This is used > + as a fallback if a target does not provide an XSAVE layout. */ > + > +extern x86_xsave_layout i387_fallback_xsave_layout (uint64_t xcr0); > + > /* Similar to i387_supply_fxsave, but use XSAVE extended state. */ > > extern void i387_supply_xsave (struct regcache *regcache, int regnum, > -- > 2.42.0 > -- Joel