From: Simon Marchi <simark@simark.ca>
To: John Baldwin <jhb@FreeBSD.org>, gdb-patches@sourceware.org
Subject: Re: [PATCH v5 09/19] gdb: Update x86 FreeBSD architectures to support XSAVE layouts.
Date: Wed, 3 May 2023 13:14:54 -0400 [thread overview]
Message-ID: <a61ad136-7d69-ee84-caa4-bc4c7eabce30@simark.ca> (raw)
In-Reply-To: <20230427210113.45380-10-jhb@FreeBSD.org>
> @@ -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.marchi@efficios.com>
Simon
next prev parent reply other threads:[~2023-05-03 17:14 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-27 21:00 [PATCH v5 00/19] Handle variable " John Baldwin
2023-04-27 21:00 ` [PATCH v5 01/19] x86: Add an x86_xsave_layout structure to handle " John Baldwin
2023-04-27 21:00 ` [PATCH v5 02/19] gdb: Store an x86_xsave_layout in i386_gdbarch_tdep John Baldwin
2023-05-03 16:22 ` Simon Marchi
2023-05-08 16:51 ` John Baldwin
2023-04-27 21:00 ` [PATCH v5 03/19] core: Support fetching x86 XSAVE layout from architectures John Baldwin
2023-04-27 21:00 ` [PATCH v5 04/19] nat/x86-cpuid.h: Add x86_cpuid_count wrapper around __get_cpuid_count John Baldwin
2023-04-27 21:00 ` [PATCH v5 05/19] x86 nat: Add helper functions to save the XSAVE layout for the host John Baldwin
2023-04-27 21:01 ` [PATCH v5 06/19] x86-fbsd-nat: Add missing public label John Baldwin
2023-04-27 21:01 ` [PATCH v5 07/19] *-fbsd-nat: Handle null inferior in read_description John Baldwin
2023-04-27 21:01 ` [PATCH v5 08/19] *-linux-nat: " John Baldwin
2023-05-03 16:38 ` Simon Marchi
2023-05-08 17:24 ` John Baldwin
2023-04-27 21:01 ` [PATCH v5 09/19] gdb: Update x86 FreeBSD architectures to support XSAVE layouts John Baldwin
2023-05-03 17:14 ` Simon Marchi [this message]
2023-05-03 17:20 ` Simon Marchi
2023-05-03 23:45 ` John Baldwin
2023-05-04 17:20 ` Simon Marchi
2023-05-08 17:33 ` John Baldwin
2023-04-27 21:01 ` [PATCH v5 10/19] gdb: Support XSAVE layouts for the current host in the FreeBSD x86 targets John Baldwin
2023-04-27 21:01 ` [PATCH v5 11/19] gdb: Update x86 Linux architectures to support XSAVE layouts John Baldwin
2023-04-27 21:01 ` [PATCH v5 12/19] gdb: Support XSAVE layouts for the current host in the Linux x86 targets John Baldwin
2023-04-27 21:01 ` [PATCH v5 13/19] gdb: Use x86_xstate_layout to parse the XSAVE extended state area John Baldwin
2023-04-27 21:01 ` [PATCH v5 14/19] gdbserver: Add a function to set the XSAVE mask and size John Baldwin
2023-04-27 21:01 ` [PATCH v5 15/19] gdbserver: Refactor the legacy region within the xsave struct John Baldwin
2023-04-27 21:01 ` [PATCH v5 16/19] gdbserver: Clear upper ZMM registers in the right location John Baldwin
2023-05-03 17:49 ` Simon Marchi
2023-05-03 23:47 ` John Baldwin
2023-04-27 21:01 ` [PATCH v5 17/19] gdbserver: Use x86_xstate_layout to parse the XSAVE extended state area John Baldwin
2023-04-27 21:01 ` [PATCH v5 18/19] x86: Remove X86_XSTATE_SIZE and related constants John Baldwin
2023-04-27 21:01 ` [PATCH v5 19/19] gdbserver: Simplify handling of ZMM registers John Baldwin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a61ad136-7d69-ee84-caa4-bc4c7eabce30@simark.ca \
--to=simark@simark.ca \
--cc=gdb-patches@sourceware.org \
--cc=jhb@FreeBSD.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).