i386: Use a fallback XSAVE layout for remote targets

  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
---
 gdb/i386-tdep.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/i387-tdep.c | 48 +++++++++++++++++++++++++++++++
 gdb/i387-tdep.h |  5 ++++
 3 files changed, 128 insertions(+)
  

On 11/21/23 2:09 PM, 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

This fixes the regression for me locally.  It would perhaps be cleaner
to isolate the regression to the remote target by moving the new logic
in i386_gdbarch_init into remote::fetch_x86_xsave_layout.  The one
caveat of that is getting access to the right tdesc.  I could perhaps
change target::fetch_x86_xsave_layout to pass down the tdesc pointer
(or maybe the gdbarch_info?).  Alternatively it might be sufficient to
just use target_current_description() as the tdesc in remote.c?  It
wasn't clear to me if the relevant tdesc in this case was always the
same as target_current_description() (though I suspect it probably is).
  

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
>
  

Hello,

On Tue, Nov 21, 2023 at 02:22:18PM -0800, John Baldwin wrote:
> On 11/21/23 2:09 PM, 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
> 
> This fixes the regression for me locally.  It would perhaps be cleaner
> to isolate the regression to the remote target by moving the new logic
> in i386_gdbarch_init into remote::fetch_x86_xsave_layout.  The one
> caveat of that is getting access to the right tdesc.  I could perhaps
> change target::fetch_x86_xsave_layout to pass down the tdesc pointer
> (or maybe the gdbarch_info?).  Alternatively it might be sufficient to
> just use target_current_description() as the tdesc in remote.c?  It
> wasn't clear to me if the relevant tdesc in this case was always the
> same as target_current_description() (though I suspect it probably is).

I don't know if it would be cleaner, actually. Maybe safer in the sense
that it would further limit the possible impact zone of your patch, but
I thought the way you approached it was nicely general so that, if we
had any other kind of target system that suffered from the same issue,
we'd get the same fix as part of it.

Just my limited 2 cents, though. It could be that I'm seeing this
from the wrong perspective, or misunderstood something...
  

On 11/26/23 8:36 PM, Joel Brobecker wrote:
> 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.

Oh, humm, I just copied that from another function below that validates
the tdesc and adds register numbers.  The GNU style in emacs agrees with
your suggestion.
  

On 11/26/23 8:39 PM, Joel Brobecker wrote:
> Hello,
> 
> On Tue, Nov 21, 2023 at 02:22:18PM -0800, John Baldwin wrote:
>> On 11/21/23 2:09 PM, 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
>>
>> This fixes the regression for me locally.  It would perhaps be cleaner
>> to isolate the regression to the remote target by moving the new logic
>> in i386_gdbarch_init into remote::fetch_x86_xsave_layout.  The one
>> caveat of that is getting access to the right tdesc.  I could perhaps
>> change target::fetch_x86_xsave_layout to pass down the tdesc pointer
>> (or maybe the gdbarch_info?).  Alternatively it might be sufficient to
>> just use target_current_description() as the tdesc in remote.c?  It
>> wasn't clear to me if the relevant tdesc in this case was always the
>> same as target_current_description() (though I suspect it probably is).
> 
> I don't know if it would be cleaner, actually. Maybe safer in the sense
> that it would further limit the possible impact zone of your patch, but
> I thought the way you approached it was nicely general so that, if we
> had any other kind of target system that suffered from the same issue,
> we'd get the same fix as part of it.
> 
> Just my limited 2 cents, though. It could be that I'm seeing this
> from the wrong perspective, or misunderstood something...

Ok, I'm fine with using this approach but will ping Simon to see if he
has any thoughts since he reviewed the original series.
  

On 11/21/23 17:22, John Baldwin wrote:
> On 11/21/23 2:09 PM, 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
> 
> This fixes the regression for me locally.  It would perhaps be cleaner
> to isolate the regression to the remote target by moving the new logic
> in i386_gdbarch_init into remote::fetch_x86_xsave_layout.  The one
> caveat of that is getting access to the right tdesc.  I could perhaps
> change target::fetch_x86_xsave_layout to pass down the tdesc pointer
> (or maybe the gdbarch_info?).  Alternatively it might be sufficient to
> just use target_current_description() as the tdesc in remote.c?  It
> wasn't clear to me if the relevant tdesc in this case was always the
> same as target_current_description() (though I suspect it probably is).

My understanding is that the fallback is used for targets that can debug
x86, where xcr0 may claim that the inferior has extended state
registers, but where the target is unable to provide an xsave layout.
There probably won't ever be another target than the remote target
in this situation, so if you can make the fix local to the remote
target that would be nice.  But really I would also be fine with what
you have.

I think that using target_current_description in
remote_target::fetch_x86_xsave_layout would be fine.
When connecting to a remote target, fetch_x86_xsave_layout would be
called here:

    (top-gdb) bt
    #0  0x0000555562d27c3a in target_ops::fetch_x86_xsave_layout (this=0x61b000044480) at /home/smarchi/src/binutils-gdb/gdb/target-delegates.c:4568
    #1  0x0000555562c86416 in target_fetch_x86_xsave_layout () at /home/smarchi/src/binutils-gdb/gdb/target.c:803
    #2  0x0000555561a672d2 in i386_gdbarch_init (info=..., arches=0x602000019bf0) at /home/smarchi/src/binutils-gdb/gdb/i386-tdep.c:8575
    #3  0x0000555560749019 in gdbarch_find_by_info (info=...) at /home/smarchi/src/binutils-gdb/gdb/arch-utils.c:1417
    #4  0x000055556070de0b in gdbarch_update_p (info=...) at /home/smarchi/src/binutils-gdb/gdb/arch-utils.c:599
    #5  0x0000555562bb7e25 in target_find_description () at /home/smarchi/src/binutils-gdb/gdb/target-descriptions.c:504
    #6  0x00005555626a6d0a in remote_target::start_remote_1 (this=0x61b000044480, from_tty=1, extended_p=0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:5077
    #7  0x00005555626ab0be in remote_target::start_remote (this=0x61b000044480, from_tty=1, extended_p=0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:5316
    #8  0x00005555626b329b in remote_target::open_1 (name=0x60200001a398 ":1234", from_tty=1, extended_p=0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:6175
    #9  0x00005555626ab3f5 in remote_target::open (name=0x60200001a398 ":1234", from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/remote.c:5338
    #10 0x0000555562c86879 in open_target (args=0x60200001a398 ":1234", from_tty=1, command=0x61200004f3c0) at /home/smarchi/src/binutils-gdb/gdb/target.c:824
    #11 0x0000555560d50343 in cmd_func (cmd=0x61200004f3c0, args=0x60200001a398 ":1234", from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2735
    #12 0x0000555562dd9f5c in execute_command (p=0x60200001a39c "4", from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/top.c:575
    #13 0x000055556174be63 in command_handler (command=0x60200001a390 "") at /home/smarchi/src/binutils-gdb/gdb/event-top.c:566
    #14 0x000055556174d212 in command_line_handler (rl=...) at /home/smarchi/src/binutils-gdb/gdb/event-top.c:802
    #15 0x0000555562f3c3e1 in tui_command_line_handler (rl=...) at /home/smarchi/src/binutils-gdb/gdb/tui/tui-interp.c:104
    #16 0x0000555561749de2 in gdb_rl_callback_handler (rl=0x60200001a330 "tar rem :1234") at /home/smarchi/src/binutils-gdb/gdb/event-top.c:259
    #17 0x00007ffff7f7b96f in rl_callback_read_char () from /usr/lib/libreadline.so.8
    #18 0x000055556174978f in gdb_rl_callback_read_char_wrapper_noexcept () at /home/smarchi/src/binutils-gdb/gdb/event-top.c:195
    #19 0x00005555617499e7 in gdb_rl_callback_read_char_wrapper (client_data=0x6110000007c0) at /home/smarchi/src/binutils-gdb/gdb/event-top.c:234
    #20 0x0000555563092d1c in stdin_event_handler (error=0, client_data=0x6110000007c0) at /home/smarchi/src/binutils-gdb/gdb/ui.c:155
    #21 0x000055556378a1e4 in handle_file_event (file_ptr=0x60700000bc40, ready_mask=1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:573
    #22 0x000055556378ab7a in gdb_wait_for_event (block=1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:694
    #23 0x00005555637887f3 in gdb_do_one_event (mstimeout=-1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:264
    #24 0x0000555561eeb6b3 in start_event_loop () at /home/smarchi/src/binutils-gdb/gdb/main.c:407
    #25 0x0000555561eebeba in captured_command_loop () at /home/smarchi/src/binutils-gdb/gdb/main.c:471
    #26 0x0000555561ef1535 in captured_main (data=0x7ffff35135a0) at /home/smarchi/src/binutils-gdb/gdb/main.c:1324
    #27 0x0000555561ef1672 in gdb_main (args=0x7ffff35135a0) at /home/smarchi/src/binutils-gdb/gdb/main.c:1343
    #28 0x00005555603daedf in main (argc=5, argv=0x7fffffffdd18) at /home/smarchi/src/binutils-gdb/gdb/gdb.c:39

The target description (if any) has been saved in inferior::tdesc_info
earlier in target_find_description (frame 5).

As to what to pass down to fetch_x86_xsave_layout (if you wanted to
avoid using target_find_description), I think it depends on what the
semantic of target_ops::fetch_x86_xsave_layout is:

 - Does the core of GDB see the xsave layout as a target-specific thing
   (meaning there is always a single xsave layout for a whole target)?
   If so I wouldn't pass anything down.  The remote target could then
   find an inferior it owns and use this inferior's tdesc to generate a
   fallback xsave layout.

 - Does the core of GDB see the xsave layout as an inferior-specific
   thing (meaning that different inferiors of a given target can have
   different xsave layouts)?  If so I would pass the inferior down.

The current targets that support returning an xsave layout return a
per-target value.  So the question is: when we add support for fetching
the xsave layout from a remote, do we make it global for the whole
remote, or do we make it per-process?

Also, my understanding is that even when we'll have an RSP packet to
fetch the xsave layout from the remote side, we'll still need this
fallback for when connecting to remotes that don't support the packet.

Simon
  

On 11/27/23 11:30 AM, Simon Marchi wrote:
> On 11/21/23 17:22, John Baldwin wrote:
>> On 11/21/23 2:09 PM, 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
>>
>> This fixes the regression for me locally.  It would perhaps be cleaner
>> to isolate the regression to the remote target by moving the new logic
>> in i386_gdbarch_init into remote::fetch_x86_xsave_layout.  The one
>> caveat of that is getting access to the right tdesc.  I could perhaps
>> change target::fetch_x86_xsave_layout to pass down the tdesc pointer
>> (or maybe the gdbarch_info?).  Alternatively it might be sufficient to
>> just use target_current_description() as the tdesc in remote.c?  It
>> wasn't clear to me if the relevant tdesc in this case was always the
>> same as target_current_description() (though I suspect it probably is).
> 
> My understanding is that the fallback is used for targets that can debug
> x86, where xcr0 may claim that the inferior has extended state
> registers, but where the target is unable to provide an xsave layout.
> There probably won't ever be another target than the remote target
> in this situation, so if you can make the fix local to the remote
> target that would be nice.  But really I would also be fine with what
> you have.
> 
> I think that using target_current_description in
> remote_target::fetch_x86_xsave_layout would be fine.
> When connecting to a remote target, fetch_x86_xsave_layout would be
> called here:
> 
>      (top-gdb) bt
>      #0  0x0000555562d27c3a in target_ops::fetch_x86_xsave_layout (this=0x61b000044480) at /home/smarchi/src/binutils-gdb/gdb/target-delegates.c:4568
>      #1  0x0000555562c86416 in target_fetch_x86_xsave_layout () at /home/smarchi/src/binutils-gdb/gdb/target.c:803
>      #2  0x0000555561a672d2 in i386_gdbarch_init (info=..., arches=0x602000019bf0) at /home/smarchi/src/binutils-gdb/gdb/i386-tdep.c:8575
>      #3  0x0000555560749019 in gdbarch_find_by_info (info=...) at /home/smarchi/src/binutils-gdb/gdb/arch-utils.c:1417
>      #4  0x000055556070de0b in gdbarch_update_p (info=...) at /home/smarchi/src/binutils-gdb/gdb/arch-utils.c:599
>      #5  0x0000555562bb7e25 in target_find_description () at /home/smarchi/src/binutils-gdb/gdb/target-descriptions.c:504
>      #6  0x00005555626a6d0a in remote_target::start_remote_1 (this=0x61b000044480, from_tty=1, extended_p=0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:5077
>      #7  0x00005555626ab0be in remote_target::start_remote (this=0x61b000044480, from_tty=1, extended_p=0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:5316
>      #8  0x00005555626b329b in remote_target::open_1 (name=0x60200001a398 ":1234", from_tty=1, extended_p=0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:6175
>      #9  0x00005555626ab3f5 in remote_target::open (name=0x60200001a398 ":1234", from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/remote.c:5338
>      #10 0x0000555562c86879 in open_target (args=0x60200001a398 ":1234", from_tty=1, command=0x61200004f3c0) at /home/smarchi/src/binutils-gdb/gdb/target.c:824
>      #11 0x0000555560d50343 in cmd_func (cmd=0x61200004f3c0, args=0x60200001a398 ":1234", from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2735
>      #12 0x0000555562dd9f5c in execute_command (p=0x60200001a39c "4", from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/top.c:575
>      #13 0x000055556174be63 in command_handler (command=0x60200001a390 "") at /home/smarchi/src/binutils-gdb/gdb/event-top.c:566
>      #14 0x000055556174d212 in command_line_handler (rl=...) at /home/smarchi/src/binutils-gdb/gdb/event-top.c:802
>      #15 0x0000555562f3c3e1 in tui_command_line_handler (rl=...) at /home/smarchi/src/binutils-gdb/gdb/tui/tui-interp.c:104
>      #16 0x0000555561749de2 in gdb_rl_callback_handler (rl=0x60200001a330 "tar rem :1234") at /home/smarchi/src/binutils-gdb/gdb/event-top.c:259
>      #17 0x00007ffff7f7b96f in rl_callback_read_char () from /usr/lib/libreadline.so.8
>      #18 0x000055556174978f in gdb_rl_callback_read_char_wrapper_noexcept () at /home/smarchi/src/binutils-gdb/gdb/event-top.c:195
>      #19 0x00005555617499e7 in gdb_rl_callback_read_char_wrapper (client_data=0x6110000007c0) at /home/smarchi/src/binutils-gdb/gdb/event-top.c:234
>      #20 0x0000555563092d1c in stdin_event_handler (error=0, client_data=0x6110000007c0) at /home/smarchi/src/binutils-gdb/gdb/ui.c:155
>      #21 0x000055556378a1e4 in handle_file_event (file_ptr=0x60700000bc40, ready_mask=1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:573
>      #22 0x000055556378ab7a in gdb_wait_for_event (block=1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:694
>      #23 0x00005555637887f3 in gdb_do_one_event (mstimeout=-1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:264
>      #24 0x0000555561eeb6b3 in start_event_loop () at /home/smarchi/src/binutils-gdb/gdb/main.c:407
>      #25 0x0000555561eebeba in captured_command_loop () at /home/smarchi/src/binutils-gdb/gdb/main.c:471
>      #26 0x0000555561ef1535 in captured_main (data=0x7ffff35135a0) at /home/smarchi/src/binutils-gdb/gdb/main.c:1324
>      #27 0x0000555561ef1672 in gdb_main (args=0x7ffff35135a0) at /home/smarchi/src/binutils-gdb/gdb/main.c:1343
>      #28 0x00005555603daedf in main (argc=5, argv=0x7fffffffdd18) at /home/smarchi/src/binutils-gdb/gdb/gdb.c:39
> 
> The target description (if any) has been saved in inferior::tdesc_info
> earlier in target_find_description (frame 5).

Ok.

> As to what to pass down to fetch_x86_xsave_layout (if you wanted to
> avoid using target_find_description), I think it depends on what the
> semantic of target_ops::fetch_x86_xsave_layout is:
> 
>   - Does the core of GDB see the xsave layout as a target-specific thing
>     (meaning there is always a single xsave layout for a whole target)?
>     If so I wouldn't pass anything down.  The remote target could then
>     find an inferior it owns and use this inferior's tdesc to generate a
>     fallback xsave layout.
> 
>   - Does the core of GDB see the xsave layout as an inferior-specific
>     thing (meaning that different inferiors of a given target can have
>     different xsave layouts)?  If so I would pass the inferior down.
> 
> The current targets that support returning an xsave layout return a
> per-target value.  So the question is: when we add support for fetching
> the xsave layout from a remote, do we make it global for the whole
> remote, or do we make it per-process?
> 
> Also, my understanding is that even when we'll have an RSP packet to
> fetch the xsave layout from the remote side, we'll still need this
> fallback for when connecting to remotes that don't support the packet.

I believe the layout would be target-wide (all the processes on a remote
target would share the same layout).  I also think what we'd add to
the RSP is fetching NT_X86_CPUID and computing a layout from that in the
remote target.
  

On 11/27/23 12:47 PM, John Baldwin wrote:
> On 11/27/23 11:30 AM, Simon Marchi wrote:
>> On 11/21/23 17:22, John Baldwin wrote:
>>> On 11/21/23 2:09 PM, 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
>>>
>>> This fixes the regression for me locally.  It would perhaps be cleaner
>>> to isolate the regression to the remote target by moving the new logic
>>> in i386_gdbarch_init into remote::fetch_x86_xsave_layout.  The one
>>> caveat of that is getting access to the right tdesc.  I could perhaps
>>> change target::fetch_x86_xsave_layout to pass down the tdesc pointer
>>> (or maybe the gdbarch_info?).  Alternatively it might be sufficient to
>>> just use target_current_description() as the tdesc in remote.c?  It
>>> wasn't clear to me if the relevant tdesc in this case was always the
>>> same as target_current_description() (though I suspect it probably is).
>>
>> My understanding is that the fallback is used for targets that can debug
>> x86, where xcr0 may claim that the inferior has extended state
>> registers, but where the target is unable to provide an xsave layout.
>> There probably won't ever be another target than the remote target
>> in this situation, so if you can make the fix local to the remote
>> target that would be nice.  But really I would also be fine with what
>> you have.

I took at stab at moving this into the remote target, but ran into an
ugly bit which is that this requires calling these new functions
from i38[67]-tdep.c from remote.c.  This means remote::fetch_x86_xsave_layout
would need a suitable #define so it could be conditional on the the i386
target being enabled.  The posted patch does avoid this problem by keeping
the logic internal to i38[67]-tdep.c.
  

On 11/27/23 15:47, John Baldwin wrote:
> On 11/27/23 11:30 AM, Simon Marchi wrote:
>> As to what to pass down to fetch_x86_xsave_layout (if you wanted to
>> avoid using target_find_description), I think it depends on what the
>> semantic of target_ops::fetch_x86_xsave_layout is:
>>
>>   - Does the core of GDB see the xsave layout as a target-specific thing
>>     (meaning there is always a single xsave layout for a whole target)?
>>     If so I wouldn't pass anything down.  The remote target could then
>>     find an inferior it owns and use this inferior's tdesc to generate a
>>     fallback xsave layout.
>>
>>   - Does the core of GDB see the xsave layout as an inferior-specific
>>     thing (meaning that different inferiors of a given target can have
>>     different xsave layouts)?  If so I would pass the inferior down.
>>
>> The current targets that support returning an xsave layout return a
>> per-target value.  So the question is: when we add support for fetching
>> the xsave layout from a remote, do we make it global for the whole
>> remote, or do we make it per-process?
>>
>> Also, my understanding is that even when we'll have an RSP packet to
>> fetch the xsave layout from the remote side, we'll still need this
>> fallback for when connecting to remotes that don't support the packet.
> 
> I believe the layout would be target-wide (all the processes on a remote
> target would share the same layout).  I also think what we'd add to
> the RSP is fetching NT_X86_CPUID and computing a layout from that in the
> remote target.

If you think a target-wide layout is sufficient, I'm fine with that.

I sometimes wonder if one could write a gdbserver / remote stub that
acts as a proxy / funnel for multiple systems, in which case the
different processes could have different xsave layouts.  But there might
be other things in the protocol that prevent it, the first thing being
the pid clashes.  Maybe it's not something we should aim to support at
all, given that nowadays one can connect GDB to multiple remote targets
if needed.

Feel free to add my:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

... to your current patch.  We can always make adjustements later when
we implement the support for remote, but the important today is to
unblock the release.

Simon
  

On 11/27/23 16:04, John Baldwin wrote:
> On 11/27/23 12:47 PM, John Baldwin wrote:
>> On 11/27/23 11:30 AM, Simon Marchi wrote:
>>> On 11/21/23 17:22, John Baldwin wrote:
>>>> On 11/21/23 2:09 PM, 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
>>>>
>>>> This fixes the regression for me locally.  It would perhaps be cleaner
>>>> to isolate the regression to the remote target by moving the new logic
>>>> in i386_gdbarch_init into remote::fetch_x86_xsave_layout.  The one
>>>> caveat of that is getting access to the right tdesc.  I could perhaps
>>>> change target::fetch_x86_xsave_layout to pass down the tdesc pointer
>>>> (or maybe the gdbarch_info?).  Alternatively it might be sufficient to
>>>> just use target_current_description() as the tdesc in remote.c?  It
>>>> wasn't clear to me if the relevant tdesc in this case was always the
>>>> same as target_current_description() (though I suspect it probably is).
>>>
>>> My understanding is that the fallback is used for targets that can debug
>>> x86, where xcr0 may claim that the inferior has extended state
>>> registers, but where the target is unable to provide an xsave layout.
>>> There probably won't ever be another target than the remote target
>>> in this situation, so if you can make the fix local to the remote
>>> target that would be nice.  But really I would also be fine with what
>>> you have.
> 
> I took at stab at moving this into the remote target, but ran into an
> ugly bit which is that this requires calling these new functions
> from i38[67]-tdep.c from remote.c.  This means remote::fetch_x86_xsave_layout
> would need a suitable #define so it could be conditional on the the i386
> target being enabled.  The posted patch does avoid this problem by keeping
> the logic internal to i38[67]-tdep.c.

Ack, thanks for trying.

Simon