Re: [PATCH 0/6] Add gdbarch-checking script

[Keith Seitz <keiths@redhat.com>]

Hi, Tom,

[It's me again!]

On 11/4/24 1:14 PM, Tom Tromey wrote:
> I was curious if any gdbarch methods were unused, so I wrote a script
> to check this.  It works by scanning the source and looking for calls
> to gdbarch methods and for arches setting the parameter.
> 
> This found a few methods that are obsolete.  These are removed in the
> series.

This is very nice... I've looked over the series, and I only have
two minor requests (patch nos. 1 & 5). Otherwise, this series
LGTM.

> A few other gdbarch settings are used by gdb but not set by any
> existing arch.  I haven't removed these since they maybe needed more
> discussion:
> 
>      stap_gdb_register_suffix
>      stap_integer_suffixes
>      stap_register_suffixes

I briefly discussed these with fche, and it certainly appears
that the compiler *could* output these suffixes. However, it seems
that either no one has generated a probe that does this (and/or
tried to debug it), or no one has reported any problems.

I agree these need further discussion, but while I understand
the desire to simplify, I still lean toward keeping these around.

>      bfloat16_bit
>      half_bit
> 
> Perhaps these should just both be 16-bit types unconditionally.

It appears that both these methods were just added for parallelism
with other type implementations, like "float_bit" and "float_format".
Honestly, probably what I would have done until a maintainer corrected
me during review!

>      addressable_memory_unit_size
> 
> This was added in 2015.  Not sure if there's a need for it but at the
> same time it seems necessary if we ever want to really support arches
> like this.

Wow, there's a lot of related code using this, even if nothing ever sets
the value differently from the default... I can't help but wonder if
this was needed for some port which never landed on the list or 
something? The work here, as noted in the ChangeLog entry, does note
that this work (at commit time) was not complete. [I don't know if that
has changed?]

Maybe Simon has some further insight?

In any case, these patches need not be delayed further for the sake
of dealing with *everything* it discovered.

Reviewed-By: Keith Seitz <keiths@redhat.com>

Keith

> Signed-off-by: Tom Tromey <tom@tromey.com>
> ---
> Tom Tromey (6):
>        Add check-gdbarch.py
>        Use 'invalid' rather than 'predicate' in some gdbarch functions
>        Remove solib_symbols_extension gdbarch hook
>        Remove skip_permanent_breakpoint gdbarch hook
>        Remove the print_vector_info gdbarch hook
>        Remove the auto_charset gdbarch hook
> 
>   gdb/arch-utils.c          |  12 ----
>   gdb/arch-utils.h          |   8 ---
>   gdb/breakpoint.c          |   3 +-
>   gdb/breakpoint.h          |   3 +-
>   gdb/charset.c             |   9 +--
>   gdb/check-gdbarch.py      |  87 +++++++++++++++++++++++
>   gdb/gdbarch-gen.c         | 173 ++--------------------------------------------
>   gdb/gdbarch-gen.h         |  40 -----------
>   gdb/gdbarch_components.py |  61 ++--------------
>   gdb/infcmd.c              |  21 ++----
>   gdb/infrun.c              |   7 +-
>   gdb/solib.c               |  27 --------
>   12 files changed, 118 insertions(+), 333 deletions(-)
> ---
> base-commit: ae2f3fa7581fb59ebbf80f93b7ed75f853f0ba29
> change-id: 20241104-check-unused-gdbarch-c396c0aff7fc
> 
> Best regards,