This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3 07/10] s390: Hook s390 into OSABI mechanism
- From: "Ulrich Weigand" <uweigand at de dot ibm dot com>
- To: prudo at linux dot vnet dot ibm dot com (Philipp Rudo)
- Cc: gdb-patches at sourceware dot org, arnez at linux dot vnet dot ibm dot com (Andreas Arnez), qiyaoltc at gmail dot com (Yao Qi)
- Date: Wed, 13 Dec 2017 19:42:07 +0100 (CET)
- Subject: Re: [PATCH v3 07/10] s390: Hook s390 into OSABI mechanism
- Authentication-results: sourceware.org; auth=none
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