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

[Philipp Rudo <prudo at linux dot vnet dot ibm dot com>]

Hi Uli,

On Wed, 13 Dec 2017 19:42:07 +0100 (CET)
"Ulrich Weigand" <uweigand@de.ibm.com> wrote:

> 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).

See comment 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.

Actually this was intended. ...

> 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:

... I moved init_osabi so early so only the functions which are overwritten by
the osabi (or needed for other reasons like the dwarf unwinder) are set before
it.  I hoped this would make the gdbarch_init clearer as the list before
init_osabi is shorter and you can say for sure that all hooks set after are
shared between all OSes. So you are not surprised with some special cases you
don't expect.  Of course this also means that if you want to overwrite a
function you first have to move it before osabi_init.  For me that is a small
price to pay.

However when you prefer the osabi_init to be done later I can move it to where
linux_init_abi is called today. In that case i can avoid moving
dwarf2_append_unwinders. Otherwise I will move the other
dwarf2 related routines up.  Just tell me what you prefer.

> - 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.

Yep you are right.  I moved the function in patch #9 (the split) to reflect its
new position in code after the split.  For some reason the hunk decided to
wander around :-)

Fixed it.
 
> >    /* 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.

See comment above.
 
> > -  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 :-)

And an other hunk with the desire to get into a patch it doesn't belong to.
Fixed it. 

> 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.

Thanks
Philipp