Re: [PATCH 3/3] gdb: remove target_ops parameter from gdbarch_core_read_description

[Simon Marchi <simon.marchi@polymtl.ca>]

On 11/29/22 14:16, John Baldwin wrote:
> On 11/28/22 6:50 PM, Simon Marchi via Gdb-patches wrote:
>> Following the previous patch ("gdb: change order of core_target
>> initialization"), it is no longer necessary for the core target to pass
>> a pointer to itself to gdbarch_core_read_description.  Implementations
>> of this method can just do regular calls to target_read_auxv, through
>> the inferior target stack.
>>
>> This allows removing the target_ops parameter in
>> gdbarch_core_read_description and a bunch of functions called
>> downstream.  Some auxv-related functions received the auxv data as a
>> parameter, since they could not assume it could be read from the current
>> inferior, that is also no longer needed.
>>
>> Regression tested.  I also tested manually against an AArch64 MTE test
>> case that Luis Machado sent me a while ago when we were working on this
>> auxv caching issue, the desired behavior of reporting the MTE tags when
>> opening the code still worked.
>>
>> Change-Id: I1e00361209028e9b65dde085d383cf950a7b5e3a
>> ---
>>   gdb/aarch64-fbsd-tdep.c   |  3 +--
>>   gdb/aarch64-linux-tdep.c  |  8 +++-----
>>   gdb/amd64-fbsd-tdep.c     |  4 +---
>>   gdb/amd64-linux-tdep.c    |  4 +---
>>   gdb/arc-linux-tdep.c      |  4 +---
>>   gdb/arm-fbsd-tdep.c       | 24 +++++-------------------
>>   gdb/arm-fbsd-tdep.h       | 15 +++++----------
>>   gdb/arm-linux-tdep.c      |  7 ++-----
>>   gdb/auxv.c                | 11 ++---------
>>   gdb/auxv.h                |  4 ----
>>   gdb/corelow.c             |  2 +-
>>   gdb/gdbarch-components.py |  2 +-
>>   gdb/gdbarch-gen.h         |  4 ++--
>>   gdb/gdbarch.c             |  4 ++--
>>   gdb/i386-fbsd-tdep.c      |  4 +---
>>   gdb/i386-linux-tdep.c     |  4 +---
>>   gdb/linux-tdep.c          | 24 ++++++++++--------------
>>   gdb/linux-tdep.h          | 14 ++++++--------
>>   gdb/mips-linux-tdep.c     |  4 +---
>>   gdb/ppc-linux-tdep.c      |  7 ++-----
>>   gdb/s390-linux-tdep.c     |  6 ++----
>>   21 files changed, 50 insertions(+), 109 deletions(-)
>>
>> diff --git a/gdb/arm-fbsd-tdep.c b/gdb/arm-fbsd-tdep.c
>> index 75ee08eba506..4ea238de7c7f 100644
>> --- a/gdb/arm-fbsd-tdep.c
>> +++ b/gdb/arm-fbsd-tdep.c
>> @@ -215,10 +215,11 @@ arm_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
>>   /* See arm-fbsd-tdep.h.  */
>>     const struct target_desc *
>> -arm_fbsd_read_description_auxv (const gdb::optional<gdb::byte_vector> &auxv,
>> -                target_ops *target, gdbarch *gdbarch, bool tls)
>> +arm_fbsd_read_description_auxv (gdbarch *gdbarch, bool tls)
>>   {
>>     CORE_ADDR arm_hwcap = 0;
>> +  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
>> +  target_ops *target = current_inferior ()->top_target ();
>>       if (!auxv.has_value ()
>>         || target_auxv_search (*auxv, target, gdbarch, AT_FREEBSD_HWCAP,
>> @@ -239,29 +240,14 @@ arm_fbsd_read_description_auxv (const gdb::optional<gdb::byte_vector> &auxv,
>>     return arm_read_description (ARM_FP_TYPE_NONE, tls);
>>   }
>>   -/* See arm-fbsd-tdep.h.  */
>> -
>> -const struct target_desc *
>> -arm_fbsd_read_description_auxv (bool tls)
>> -{
>> -  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
>> -  return arm_fbsd_read_description_auxv (auxv,
>> -                     current_inferior ()->top_target (),
>> -                     current_inferior ()->gdbarch,
>> -                     tls);
>> -}
>> -
> 
> This function is used by arm-fbsd-nat.c in the read_description target
> method.  Probably that function just needs to be updated to pass in the
> gdbarch explicitly rather than keeping this wrapper method though:
> 
> diff --git a/gdb/arm-fbsd-nat.c b/gdb/arm-fbsd-nat.c
> index 340b8e0d710..0ed3104d12d 100644
> --- a/gdb/arm-fbsd-nat.c
> +++ b/gdb/arm-fbsd-nat.c
> @@ -96,7 +96,7 @@ arm_fbsd_nat_target::read_description ()
>  #ifdef PT_GETREGSET
>    tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
>  #endif
> -  desc = arm_fbsd_read_description_auxv (tls);
> +  desc = arm_fbsd_read_description_auxv (current_inferior ()->gdbarch, tls);
>    if (desc == NULL)
>      desc = this->beneath ()->read_description ();
>    return desc;

Woops, thanks for pointing it out.  Actually, I'll leave the
one-parameter version there and adapt it, so no change should be needed
in the nat file.

Thanks,

Simon