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

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

[-- Attachment #1: Type: text/plain, Size: 2206 bytes --]

Hi Uli,

On Thu, 14 Dec 2017 16:44:31 +0100 (CET)
"Ulrich Weigand" <uweigand@de.ibm.com> wrote:

> Philipp Rudo wrote:
> 
> > ... 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.  
> 
> Yes, please do move init_osabi to be done later.

I played around and got the fixup patch attached.  For some reason however now
some python testcases fail. In particular 

FAIL: gdb.python/py-breakpoint.exp: test_bkpt_qualified: qualified false
FAIL: gdb.python/py-breakpoint.exp: test_bkpt_qualified: qualified true
FAIL: gdb.python/py-breakpoint.exp: test_bkpt_qualified: qualified true and explicit
FAIL: gdb.python/py-breakpoint.exp: test_bkpt_qualified: qualified false and explicit
FAIL: gdb.python/py-breakpoint.exp: test_bkpt_qualified: -q in spec string and qualified false

each with the error message

Traceback (most recent call last):^M                                            
  File "<string>", line 1, in <module>^M                                        
TypeError: argument 10 (impossible<bad format char>)^M                          
Error while executing Python code.^M

I have absolutely no clue what causes this as the gdbarch'es (with and without
the patch) seem to be identical.  As this is my last day before the Christmas
vacation I cannot have a closer look at it right now.  So finalizing the split
up will have to wait until next year.

Merry Christmas and see you next year
Philipp

[-- Attachment #2: 0001-fixup-s390-Hook-s390-into-OSABI-mechanism.patch --]
[-- Type: text/x-patch, Size: 7700 bytes --]

From e332d4fc2665707a125e63d17465b2aede5a554a Mon Sep 17 00:00:00 2001
From: Philipp Rudo <prudo@linux.vnet.ibm.com>
Date: Fri, 15 Dec 2017 12:51:45 +0100
Subject: [PATCH] fixup! s390: Hook s390 into OSABI mechanism

---
 gdb/s390-linux-tdep.c | 159 ++++++++++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 75 deletions(-)

diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
index a4e27f03fc..f43c0d38f3 100644
--- a/gdb/s390-linux-tdep.c
+++ b/gdb/s390-linux-tdep.c
@@ -8020,54 +8020,6 @@ 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;
 
-  /* 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);
-
-  gdbarch_init_osabi (info, gdbarch);
-
-  /* Check any target description for validity.  */
-  gdb_assert (tdesc_has_registers (tdep->tdesc));
-  if (!s390_tdesc_valid (tdep, tdesc_data))
-    {
-      tdesc_data_cleanup (tdesc_data);
-      xfree (tdep);
-      gdbarch_free (gdbarch);
-      return NULL;
-    }
-
-  /* Determine vector ABI.  */
-#ifdef HAVE_ELF
-  if (tdep->have_vx
-      && info.abfd != NULL
-      && info.abfd->format == bfd_object
-      && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour
-      && bfd_elf_get_obj_attr_int (info.abfd, OBJ_ATTR_GNU,
-				   Tag_GNU_S390_ABI_Vector) == 2)
-    tdep->vector_abi = S390_VECTOR_ABI_128;
-#endif
-
-  /* Find a candidate among extant architectures.  */
-  for (arches = gdbarch_list_lookup_by_info (arches, &info);
-       arches != NULL;
-       arches = gdbarch_list_lookup_by_info (arches->next, &info))
-    {
-      struct gdbarch_tdep *tmp = gdbarch_tdep (arches->gdbarch);
-      if (!tmp)
-	continue;
-      /* A program can 'choose' not to use the vector registers when they
-	 are present.  Leading to the same tdesc but different tdep and
-	 thereby a different gdbarch.  */
-      if (tmp->vector_abi != tdep->vector_abi)
-	continue;
-
-      tdesc_data_cleanup (tdesc_data);
-      xfree (tdep);
-      gdbarch_free (gdbarch);
-      return arches->gdbarch;
-    }
-
   set_gdbarch_believe_pcc_promotion (gdbarch, 0);
   set_gdbarch_char_signed (gdbarch, 0);
 
@@ -8108,26 +8060,6 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_ax_pseudo_register_push_stack
       (gdbarch, s390_ax_pseudo_register_push_stack);
   set_gdbarch_gen_return_address (gdbarch, s390_gen_return_address);
-  tdesc_use_registers (gdbarch, tdep->tdesc, tdesc_data);
-  set_gdbarch_register_name (gdbarch, s390_register_name);
-
-  /* Assign pseudo register numbers.  */
-  first_pseudo_reg = gdbarch_num_regs (gdbarch);
-  last_pseudo_reg = first_pseudo_reg;
-  if (tdep->have_upper)
-    {
-      tdep->gpr_full_regnum = last_pseudo_reg;
-      last_pseudo_reg += 16;
-    }
-  if (tdep->have_vx)
-    {
-      tdep->v0_full_regnum = last_pseudo_reg;
-      last_pseudo_reg += 16;
-    }
-  tdep->pc_regnum = last_pseudo_reg++;
-  tdep->cc_regnum = last_pseudo_reg++;
-  set_gdbarch_pc_regnum (gdbarch, tdep->pc_regnum);
-  set_gdbarch_num_pseudo_regs (gdbarch, last_pseudo_reg - first_pseudo_reg);
 
   /* Inferior function calls.  */
   set_gdbarch_push_dummy_call (gdbarch, s390_push_dummy_call);
@@ -8138,10 +8070,7 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   /* Frame handling.  */
   dwarf2_frame_set_init_reg (gdbarch, s390_dwarf2_frame_init_reg);
   dwarf2_frame_set_adjust_regnum (gdbarch, s390_adjust_frame_regnum);
-  frame_base_append_sniffer (gdbarch, dwarf2_frame_base_sniffer);
-  frame_unwind_append_unwinder (gdbarch, &s390_stub_frame_unwind);
-  frame_unwind_append_unwinder (gdbarch, &s390_frame_unwind);
-  frame_base_set_default (gdbarch, &s390_frame_base);
+  dwarf2_append_unwinders (gdbarch);
   set_gdbarch_unwind_pc (gdbarch, s390_unwind_pc);
   set_gdbarch_unwind_sp (gdbarch, s390_unwind_sp);
 
@@ -8152,13 +8081,13 @@ 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);
 
-  switch (tdep->abi)
+  switch (info.bfd_arch_info->mach)
     {
-    case ABI_LINUX_S390:
+    case bfd_mach_s390_31:
       set_gdbarch_addr_bits_remove (gdbarch, s390_addr_bits_remove);
       break;
 
-    case ABI_LINUX_ZSERIES:
+    case bfd_mach_s390_64:
       set_gdbarch_long_bit (gdbarch, 64);
       set_gdbarch_long_long_bit (gdbarch, 64);
       set_gdbarch_ptr_bit (gdbarch, 64);
@@ -8190,6 +8119,86 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_gcc_target_options (gdbarch, s390_gcc_target_options);
   set_gdbarch_gnu_triplet_regexp (gdbarch, s390_gnu_triplet_regexp);
 
+  /* Initialize the OSABI.  */
+  gdbarch_init_osabi (info, gdbarch);
+
+  /* Check any target description for validity.  */
+  gdb_assert (tdesc_has_registers (tdep->tdesc));
+  if (!s390_tdesc_valid (tdep, tdesc_data))
+    {
+      tdesc_data_cleanup (tdesc_data);
+      xfree (tdep);
+      gdbarch_free (gdbarch);
+      return NULL;
+    }
+
+  /* Determine vector ABI.  */
+#ifdef HAVE_ELF
+  if (tdep->have_vx
+      && info.abfd != NULL
+      && info.abfd->format == bfd_object
+      && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour
+      && bfd_elf_get_obj_attr_int (info.abfd, OBJ_ATTR_GNU,
+				   Tag_GNU_S390_ABI_Vector) == 2)
+    tdep->vector_abi = S390_VECTOR_ABI_128;
+#endif
+
+  /* Find a candidate among extant architectures.  */
+  for (arches = gdbarch_list_lookup_by_info (arches, &info);
+       arches != NULL;
+       arches = gdbarch_list_lookup_by_info (arches->next, &info))
+    {
+      struct gdbarch_tdep *tmp = gdbarch_tdep (arches->gdbarch);
+      if (!tmp)
+	continue;
+      /* A program can 'choose' not to use the vector registers when they
+	 are present.  Leading to the same tdesc but different tdep and
+	 thereby a different gdbarch.  */
+      if (tmp->vector_abi != tdep->vector_abi)
+	continue;
+
+      tdesc_data_cleanup (tdesc_data);
+      xfree (tdep);
+      gdbarch_free (gdbarch);
+      return arches->gdbarch;
+    }
+
+  /* tdesc_use_registers sets:
+      * gdbarch_num_regs
+      * gdbarch_register_name
+      * gdbarch_register_type
+      * gdbarch_remote_register_num
+      * gdbarch_register_reggroup_p
+
+    Furthermore it deletes tdesc_data. No(!) call to tdesc_data_cleanup after
+    this point or GDB crashes with a double free.
+  */
+  tdesc_use_registers (gdbarch, tdep->tdesc, tdesc_data);
+  set_gdbarch_register_name (gdbarch, s390_register_name);
+
+  /* Assign pseudo register numbers.  */
+  first_pseudo_reg = gdbarch_num_regs (gdbarch);
+  last_pseudo_reg = first_pseudo_reg;
+  if (tdep->have_upper)
+    {
+      tdep->gpr_full_regnum = last_pseudo_reg;
+      last_pseudo_reg += 16;
+    }
+  if (tdep->have_vx)
+    {
+      tdep->v0_full_regnum = last_pseudo_reg;
+      last_pseudo_reg += 16;
+    }
+  tdep->pc_regnum = last_pseudo_reg++;
+  tdep->cc_regnum = last_pseudo_reg++;
+  set_gdbarch_pc_regnum (gdbarch, tdep->pc_regnum);
+  set_gdbarch_num_pseudo_regs (gdbarch, last_pseudo_reg - first_pseudo_reg);
+
+  frame_base_append_sniffer (gdbarch, dwarf2_frame_base_sniffer);
+  frame_unwind_append_unwinder (gdbarch, &s390_stub_frame_unwind);
+  frame_unwind_append_unwinder (gdbarch, &s390_frame_unwind);
+  frame_base_set_default (gdbarch, &s390_frame_base);
+
   return gdbarch;
 }
 
-- 
2.13.5