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
@@ -30,6 +30,7 @@
#include "gdbcore.h"
#include "gdbcmd.h"
#include "objfiles.h"
+#include "osabi.h"
#include "regcache.h"
#include "trad-frame.h"
#include "frame-base.h"
@@ -8019,32 +8020,12 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
struct tdesc_arch_data *tdesc_data = tdesc_data_alloc ();
info.tdesc_data = tdesc_data;
- /* Default ABI and register size. */
- switch (info.bfd_arch_info->mach)
- {
- case bfd_mach_s390_31:
- tdep->abi = ABI_LINUX_S390;
- break;
-
- case bfd_mach_s390_64:
- tdep->abi = ABI_LINUX_ZSERIES;
- break;
-
- default:
- xfree (tdep);
- gdbarch_free (gdbarch);
- return NULL;
- }
+ /* 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);
- /* 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);
/* Check any target description for validity. */
gdb_assert (tdesc_has_registers (tdep->tdesc));
@@ -8112,15 +8093,11 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
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);
set_gdbarch_pseudo_register_read (gdbarch, s390_pseudo_register_read);
set_gdbarch_pseudo_register_write (gdbarch, s390_pseudo_register_write);
set_tdesc_pseudo_register_name (gdbarch, s390_pseudo_register_name);
@@ -8159,18 +8136,10 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
set_gdbarch_frame_align (gdbarch, s390_frame_align);
set_gdbarch_return_value (gdbarch, s390_return_value);
- /* Syscall handling. */
- set_gdbarch_get_syscall_number (gdbarch, s390_linux_get_syscall_number);
-
/* 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);
- 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);
set_gdbarch_unwind_pc (gdbarch, s390_unwind_pc);
set_gdbarch_unwind_sp (gdbarch, s390_unwind_sp);
@@ -8181,65 +8150,119 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
set_gdbarch_displaced_step_location (gdbarch, linux_displaced_step_location);
set_gdbarch_max_insn_length (gdbarch, S390_MAX_INSTR_SIZE);
- /* Note that GNU/Linux is the only OS supported on this
- platform. */
- linux_init_abi (info, gdbarch);
-
switch (tdep->abi)
{
case ABI_LINUX_S390:
set_gdbarch_addr_bits_remove (gdbarch, s390_addr_bits_remove);
- set_solib_svr4_fetch_link_map_offsets
- (gdbarch, svr4_ilp32_fetch_link_map_offsets);
-
- set_xml_syscall_file_name (gdbarch, XML_SYSCALL_FILENAME_S390);
break;
case ABI_LINUX_ZSERIES:
set_gdbarch_long_bit (gdbarch, 64);
set_gdbarch_long_long_bit (gdbarch, 64);
set_gdbarch_ptr_bit (gdbarch, 64);
- set_solib_svr4_fetch_link_map_offsets
- (gdbarch, svr4_lp64_fetch_link_map_offsets);
set_gdbarch_address_class_type_flags (gdbarch,
s390_address_class_type_flags);
set_gdbarch_address_class_type_flags_to_name (gdbarch,
s390_address_class_type_flags_to_name);
set_gdbarch_address_class_name_to_type_flags (gdbarch,
s390_address_class_name_to_type_flags);
- set_xml_syscall_file_name (gdbarch, XML_SYSCALL_FILENAME_S390X);
break;
}
- set_gdbarch_skip_trampoline_code (gdbarch, find_solib_trampoline_target);
-
- /* Enable TLS support. */
- set_gdbarch_fetch_tls_load_module_address (gdbarch,
- svr4_fetch_objfile_link_map);
-
/* SystemTap functions. */
set_gdbarch_stap_register_prefixes (gdbarch, stap_register_prefixes);
set_gdbarch_stap_register_indirection_prefixes (gdbarch,
stap_register_indirection_prefixes);
set_gdbarch_stap_register_indirection_suffixes (gdbarch,
stap_register_indirection_suffixes);
+
+ set_gdbarch_disassembler_options (gdbarch, &s390_disassembler_options);
+ set_gdbarch_valid_disassembler_options (gdbarch,
+ disassembler_options_s390 ());
+
+ /* Process record-replay */
+ set_gdbarch_process_record (gdbarch, s390_process_record);
+
+ /* 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);
- /* Support reverse debugging. */
+ return gdbarch;
+}
- set_gdbarch_process_record (gdbarch, s390_process_record);
- set_gdbarch_process_record_signal (gdbarch, s390_linux_record_signal);
+/* Initialize OSABI common for GNU/Linux on 31- and 64-bit systems. */
+static void
+s390_linux_init_abi_any (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+ linux_init_abi (info, gdbarch);
+
+ /* Register handling. */
+ set_gdbarch_core_read_description (gdbarch, s390_core_read_description);
+ set_gdbarch_iterate_over_regset_sections (gdbarch,
+ s390_iterate_over_regset_sections);
+ set_gdbarch_write_pc (gdbarch, s390_write_pc);
+ set_gdbarch_cannot_store_register (gdbarch, s390_cannot_store_register);
+
+ /* Syscall handling. */
+ set_gdbarch_get_syscall_number (gdbarch, s390_linux_get_syscall_number);
+
+ /* 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);
+ set_gdbarch_skip_trampoline_code (gdbarch, find_solib_trampoline_target);
+
+ /* Enable TLS support. */
+ set_gdbarch_fetch_tls_load_module_address (gdbarch,
+ svr4_fetch_objfile_link_map);
+
+ /* Support reverse debugging. */
+ set_gdbarch_process_record_signal (gdbarch, s390_linux_record_signal);
s390_init_linux_record_tdep (&s390_linux_record_tdep, ABI_LINUX_S390);
s390_init_linux_record_tdep (&s390x_linux_record_tdep, ABI_LINUX_ZSERIES);
+}
- set_gdbarch_disassembler_options (gdbarch, &s390_disassembler_options);
- set_gdbarch_valid_disassembler_options (gdbarch,
- disassembler_options_s390 ());
+/* Initialize OSABI for GNU/Linux on 31-bit systems. */
- return gdbarch;
+static void
+s390_linux_init_abi_31 (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+ const struct target_desc *tdesc = info.target_desc;
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
+ tdep->abi = ABI_LINUX_S390;
+ if (!tdesc_has_registers (tdesc))
+ tdesc = tdesc_s390_linux32;
+ tdep->tdesc = tdesc;
+
+ s390_linux_init_abi_any (info, gdbarch);
+
+ set_solib_svr4_fetch_link_map_offsets (gdbarch,
+ svr4_ilp32_fetch_link_map_offsets);
+ set_xml_syscall_file_name (gdbarch, XML_SYSCALL_FILENAME_S390);
+}
+
+/* Initialize OSABI for GNU/Linux on 64-bit systems. */
+
+static void
+s390_linux_init_abi_64 (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+ const struct target_desc *tdesc = info.target_desc;
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
+ tdep->abi = ABI_LINUX_ZSERIES;
+ if (!tdesc_has_registers (tdesc))
+ tdesc = tdesc_s390x_linux64;
+ tdep->tdesc = tdesc;
+
+ s390_linux_init_abi_any (info, gdbarch);
+
+ set_solib_svr4_fetch_link_map_offsets (gdbarch,
+ svr4_lp64_fetch_link_map_offsets);
+ set_xml_syscall_file_name (gdbarch, XML_SYSCALL_FILENAME_S390X);
}
void
@@ -8248,6 +8271,12 @@ _initialize_s390_tdep (void)
/* Hook us into the gdbarch mechanism. */
register_gdbarch_init (bfd_arch_s390, s390_gdbarch_init);
+ /* Hook us into the OSABI mechanism. */
+ gdbarch_register_osabi (bfd_arch_s390, bfd_mach_s390_31, GDB_OSABI_LINUX,
+ s390_linux_init_abi_31);
+ gdbarch_register_osabi (bfd_arch_s390, bfd_mach_s390_64, GDB_OSABI_LINUX,
+ s390_linux_init_abi_64);
+
/* Initialize the GNU/Linux target descriptions. */
initialize_tdesc_s390_linux32 ();
initialize_tdesc_s390_linux32v1 ();