Re: [PATCH] gdb: Add a default_unwind_pc method for gdbarch

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 2018-03-07 05:04 PM, Andrew Burgess wrote:
> As requested / suggested by Yao in this mail:
> 
>    https://sourceware.org/ml/gdb-patches/2018-03/msg00133.html
> 
> this patch provides a default implementation of unwind_pc and switches
> over targets that I believe are "obviously" using the default strategy.
> 
> I've been pretty conservative about switching targets over, so targets
> like arc that have some logging in place, but otherwise do the
> standard thing, are left with their own unwind_pc method.
> 
> In frame.c I've removed the gdbarch_unwind_pc_p check (the predicate
> is gone now) as it's assumed that all targets will have an
> implementation either the default, or their own.  This function is
> reindented, so it looks like a bigger change than it really is.
> 
> The problem with this patch is testing, there's been very little.  I
> tested that every target I could build GDB for before still builds
> after this patch, which covers all of the targets I touched, however,
> I've done no functionality testing on the built GDBs.
> 
> Is there any interest in merging this patch?  I'm happy to run any
> additional testing if I can be pointed to some instructions / hints
> for what to do.
> 
> Thanks,
> Andrew
> 
> --
> 
> Many architectures gdbarch unwind_pc method follow the same pattern.
> Provide a default method and convert targets to use it where possible.

Hi Andrew,

Patches that reduce duplication are always welcome :)

Do you have access to the buildbot to at least test your patch there?  It
only covers a fraction of the arches supported by GDB, but it's better than
nothing.  We can't expect a contributor to test on all supported arches.

Things that I think could be added to this patch:

- aarch64 seems like it could be converted too
- sparc too.  It passes tdep->pc_regnum, but from what I can tell
  gdbarch_pc_regnum is set to the same value, so it should be equivalent.

And maybe for a future patch, I wonder if we could generalize even more the
default method by applying in order:

  1. frame_unwind_register (next_frame, gdbarch_pc_regnum (gdbarch), buf);
  2. extract_typed_address (buf, builtin_type (gdbarch)->builtin_func_ptr);
  3. gdbarch_addr_bits_remove (gdbarch, addr)

That would allow removing a bunch of other implementations.  If an architecture
doesn't specify a custom pointer_to_address gdbarch method, extract_typed_address
will end up calling extract_unsigned_integer, which is similar to the current
behavior (just a different path to get there).  Arches that don't specify a custom
addr_bits_remove method use core_addr_identity, which is a no-op.

And for another future patch, could we apply the same treatment to *_unwind_sp?
Even *_dummy_id seem like it's always the same pattern.

Simon