Re: [PATCH v3 07/10] s390: Hook s390 into OSABI mechanism

["Ulrich Weigand" <uweigand@de.ibm.com>]

Philipp Rudo wrote:

> +  /* The DWARF unwinders must be appended before the ABI is initialized.
> +     Otherwise it is possible that a ABI default unwinder gets called before
> +     the DWARF unwinder even gets the chance.  */
> +  dwarf2_append_unwinders (gdbarch);

It would be better to also move the related dwarf2 routines here (see below).

> -  /* Use default target description if none provided by the target.  */
> -  if (!tdesc_has_registers (tdesc))
> -    {
> -      if (tdep->abi == ABI_LINUX_S390)
> -	tdesc = tdesc_s390_linux32;
> -      else
> -	tdesc = tdesc_s390x_linux64;
> -    }
> -  tdep->tdesc = tdesc;
> +  gdbarch_init_osabi (info, gdbarch);

One (potential) problem with this is that gdbarch_init_osabi is now
called very early.  This means an OSABI has no way of overriding any
of the gdbarch functions that are installed below this point.

Usually, most targets try to call gdbarch_init_osabi as late as possible
so as to make such overrides possible, e.g. at the place where we now
call linux_init_abi.

I understand you moved this call so early so that you can get a default
tdesc from ABI code if no tdesc is specified.  However:

- Even so, many of the gdbarch routines do not depend in any way on
  the selected tdep->abi or tdesc.  At least those should preferably
  still be done before the gdbarch_init_osabi call.

- It also might be worthwhile to reconsider whether we even need to
  use a default (fallback) tdesc at all.  The only targets where we
  do not get a tdesc would be remote connection to a gdbserver that
  does not send a tdesc (but all gdbservers built in the last 10 years
  do so ...) or some other older remote stub (e.g. in qemu) that doesn't
  send a tdesc.  Maybe it is not necessary to try to support this any
  longer.  But that is probably a separate discussion, and it isn't
  necessary for this issue to block this patch set from going in.

>    set_gdbarch_num_regs (gdbarch, S390_NUM_REGS);
>    set_gdbarch_sp_regnum (gdbarch, S390_SP_REGNUM);
>    set_gdbarch_fp0_regnum (gdbarch, S390_F0_REGNUM);
> +  set_gdbarch_guess_tracepoint_registers (gdbarch,
> +					  s390_guess_tracepoint_registers);
>    set_gdbarch_stab_reg_to_regnum (gdbarch, s390_dwarf_reg_to_regnum);
>    set_gdbarch_dwarf2_reg_to_regnum (gdbarch, s390_dwarf_reg_to_regnum);
>    set_gdbarch_value_from_register (gdbarch, s390_value_from_register);
> -  set_gdbarch_core_read_description (gdbarch, s390_core_read_description);
> -  set_gdbarch_iterate_over_regset_sections (gdbarch,
> -					    s390_iterate_over_regset_sections);
> -  set_gdbarch_cannot_store_register (gdbarch, s390_cannot_store_register);
> -  set_gdbarch_write_pc (gdbarch, s390_write_pc);
> -  set_gdbarch_guess_tracepoint_registers (gdbarch, s390_guess_tracepoint_registers);

There's no reason to move that last function around, it seems.

>    /* Frame handling.  */
>    dwarf2_frame_set_init_reg (gdbarch, s390_dwarf2_frame_init_reg);
>    dwarf2_frame_set_adjust_regnum (gdbarch, s390_adjust_frame_regnum);
> -  dwarf2_append_unwinders (gdbarch);
>    frame_base_append_sniffer (gdbarch, dwarf2_frame_base_sniffer);

All the dwarf2 routines should move up in one block.

> -  frame_unwind_append_unwinder (gdbarch, &s390_stub_frame_unwind);
> -  frame_unwind_append_unwinder (gdbarch, &s390_sigtramp_frame_unwind);
> -  frame_unwind_append_unwinder (gdbarch, &s390_frame_unwind);
> -  frame_base_set_default (gdbarch, &s390_frame_base);

The sigtramp unwinder should move to ABI code, but not the others.
I note that you now move them back in patch 09/10, but they shouldn't
even be moved out in the first place :-)

I've also looked at the other patches in this series, and they look
good to me.  So unless Andreas has any further comments, I think the
series is good to go in once the above comments have been addressed.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com