Re: [PATCH v2 07/11] s390: Hook s390 into OSABI mechanism

[Philipp Rudo <prudo@linux.vnet.ibm.com>]

Hi Uli,

On Tue, 5 Dec 2017 14:20:55 +0100 (CET)
"Ulrich Weigand" <uweigand@de.ibm.com> wrote:

> Philipp Rudo wrote:
> 
> Just a couple of quick comments on the common vs. Linux split.
> 
> > +  set_gdbarch_guess_tracepoint_registers (gdbarch,
> > +					  s390_guess_tracepoint_registers);  
> 
> This is OS-independent as far as I can see.

Ok, I'll move it back.
 
> > +  /* Frame handling.  */
> > +  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);  
> 
> All of that *except* the sigtramp unwinder is OS-independent.  In fact,
> if move the prolog-based sniffer to common code, you'll see that you no
> longer need to export various internal routines from s390-tdep.c to
> s390-linux-tdep.c.
> 
> This may require swapping the order of the stub and the sigtramp unwinder,
> but that should be harmless.  In the end you should have this sequence:
> 
> - first, in common code, announce all the DWARF-based unwinders
> - then, in Linux ABI code, announce the sigtramp unwinder
> - finally, back in common code, announce all the fallback unwinders

That's not true.  At least for the kernel the unwinders (except the DWARF-based)
failed and I had to write my own one.  Especially the fact that the kernel has 
multiple stack locations and no distinct 'End of Stack' caused problems.

However if I go with your approach and add my unwinder in the kernel ABI code
as default, i.e. that it always catches the call, it should work.  But that
won't be nice code.  That's why I moved them to the Linux ABI.

> >    set_gdbarch_process_record (gdbarch, s390_process_record);
> >    set_gdbarch_process_record_signal (gdbarch, s390_linux_record_signal);  
> 
> The first of these should be generic, only the second is Linux specific
> (that's why there are two different callbacks to begin with!).

Ok, I'll move the first one back.

> > +  /* Miscellaneous.  */
> > +  set_gdbarch_stap_is_single_operand (gdbarch, s390_stap_is_single_operand);
> > +  set_gdbarch_gcc_target_options (gdbarch, s390_gcc_target_options);
> > +  set_gdbarch_gnu_triplet_regexp (gdbarch, s390_gnu_triplet_regexp);  
> 
> Are these really Linux-specific?

No, not really.  They should be common for the architecture for all systems
with working GDB/GCC/BFD. I'll move them to the common file.

Thanks
Philipp