Re: [PATCH v5 02/19] gdb: Store an x86_xsave_layout in i386_gdbarch_tdep.

[John Baldwin <jhb@FreeBSD.org>]

On 5/3/23 9:22 AM, Simon Marchi wrote:
> On 4/27/23 17:00, John Baldwin wrote:
>> This structure is fetched from the current target in i386_gdbarch_init
>> via a new "fetch_x86_xsave_layout" target method.
> 
> I got:
> 
> Applying: gdb: Store an x86_xsave_layout in i386_gdbarch_tdep.
> .git/rebase-apply/patch:147: new blank line at EOF.
> +
> warning: 1 line adds whitespace errors.

Humm, this is is target-delegates.c which is generated by
make-target-delegates.py.  The current version of this file also
has an extra blank line of EOF.

>> ---
>>   gdb/i386-tdep.c        | 19 ++++++++++++++++++-
>>   gdb/i386-tdep.h        |  4 ++++
>>   gdb/target-debug.h     | 20 ++++++++++++++++++++
>>   gdb/target-delegates.c | 27 +++++++++++++++++++++++++++
>>   gdb/target.c           |  6 ++++++
>>   gdb/target.h           |  7 +++++++
>>   6 files changed, 82 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
>> index 1ab9fc0e87d..43f2ae6c14e 100644
>> --- a/gdb/i386-tdep.c
>> +++ b/gdb/i386-tdep.c
>> @@ -8504,8 +8504,24 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>     int bnd0_regnum;
>>     int num_bnd_cooked;
>>   
>> +  x86_xsave_layout xsave_layout = target_fetch_x86_xsave_layout ();
>> +
>>     /* If there is already a candidate, use it.  */
>> -  arches = gdbarch_list_lookup_by_info (arches, &info);
>> +  for (arches = gdbarch_list_lookup_by_info (arches, &info);
>> +       arches != NULL;
>> +       arches = gdbarch_list_lookup_by_info (arches->next, &info))
>> +    {
>> +      /* Check that the XSAVE layout of ARCHES matches the layout for
>> +	 the current target.  */
>> +      i386_gdbarch_tdep *other_tdep
>> +	= gdbarch_tdep<i386_gdbarch_tdep> (arches->gdbarch);
>> +
>> +      if (other_tdep->xsave_layout != xsave_layout)
>> +	continue;
>> +
>> +      break;
>> +    }
>> +
>>     if (arches != NULL)
>>       return arches->gdbarch;
> 
> I think that returningd from the loop would be a tiny bit simpler
> (replacing the break with `return arches->gdbarch`.  You could drop this
> if.

Ok.  I had thought this pattern was used more consistently in various *gdbarch_init
routines, but looking at them now, aarch64 does an early return as you mention,
and MIPS does a weird hybrid.  riscv does something akin to the above.  I'll change
it to do a == and quick return.

I did have to update patch 1 to add an operator== for this to work FYI.

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

-- 
John Baldwin