Re: [rfc][2/3] gdbserver bi-arch support: core s390x part

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Daniel Jacobowitz wrote:

> This isn't quite good enough.  It's already possible for the
> architecture to change during a single gdbserver run, so I think we
> have to support re-calling arch_setup.  Right now you have to run in
> extended mode and replace a 32-bit binary with a 64-bit one before
> restarting it, but I'll be checking in the support for "set remote
> exec-file" shortly if I catch up on patch review :-)
> 
> All it takes is hopefully a global flag that we can reset when
> switching to a new inferior.

I see.  Something along the lines of the new version below?


> > -  unsigned long pc;
> > -  collect_register_by_name ("pswa", &pc);
> > +  if (register_size (0) == 4)
> > +    {
> > +      unsigned int pc;
> > +      collect_register_by_name ("pswa", &pc);
> >  #ifndef __s390x__
> > -  pc &= 0x7fffffff;
> > +      pc &= 0x7fffffff;
> >  #endif
> > -  return pc;
> > +      return pc;
> > +    }
> > +  else
> > +    {
> > +      unsigned long pc;
> > +      collect_register_by_name ("pswa", &pc);
> > +      return pc;
> > +    }
> >  }
> 
> This is harmlessly if dead if gdbserver is 32-bit, right?

Yes.  Note that there are two different issues: whether we need
to clear the high bit depends on the architecture of gdbserver,
but the size of the register depends on the architecture of
the inferior.  To simplify the latter issue, maybe it would be
nice if generic code had a "collect_register_as_addr" helper
that would check the register's size and convert its contents
to CORE_ADDR as appropriate?

> > +static void
> > +s390_arch_setup (void)
> > +{
> > +  /* Assume 31-bit inferior process.  */
> > +  init_registers_s390 ();
> > +
> > +  /* On a 64-bit host, check the low bit of the (31-bit) PSWM
> > +     -- if this is one, we actually have a 64-bit inferior.  */
> > +#ifdef __s390x__
> > +  {
> > +    unsigned int pswm;
> > +    collect_register_by_name ("pswm", &pswm);
> > +    if (pswm & 1)
> > +      init_registers_s390x ();
> > +  }
> > +#endif
> 
> This makes my head hurt quite a lot.  You're fetching registers into
> the cache before you know their size for sure?

Well, it is not so much about "knowing" their size -- on a 64-bit
gdbserver, ptrace will always give me 64-bit register values.  I
can simply choose whether to view those as if they were 32-bit by
installing the s390 register map -- which I do.

> I think using ptrace directly in this case might be more obvious.

I tried this, but couldn't quite figure out which PID to use ...
Also, getting all the header files and defines in place in the low
target file (which doesn't use ptrace at all right now) seemed a
bit tedious -- so I went with the above solution instead.

Bye,
Ulrich


ChangeLog:

	* configure.srv [s390x-*-linux*]: Set srv_regobj to include both
	reg-s390.o and reg-s390x.o.

	* linux-low.c (new_inferior): New global variable.
	(linux_create_inferior, linux_attach): Set it.
	(linux_wait_for_process): Call the_low_target.arch_setup after the
	target has stopped for the first time.
	(initialize_low): Do not call the_low_target.arch_setup.

	* linux-s390-low.c (s390_get_pc): Support bi-arch operation.
	(s390_set_pc): Likewise.
	(s390_arch_setup): New function.
	(the_low_target): Use s390_arch_setup as arch_setup routine.

	* regcache.c (realloc_register_cache): New function.
	(set_register_cache): Call it for each existing regcache.


diff -urNp gdb-orig/gdb/gdbserver/configure.srv gdb-head/gdb/gdbserver/configure.srv
--- gdb-orig/gdb/gdbserver/configure.srv	2008-01-29 23:10:39.000000000 +0100
+++ gdb-head/gdb/gdbserver/configure.srv	2008-01-29 23:21:53.000000000 +0100
@@ -138,7 +138,7 @@ case "${target}" in
 			srv_linux_regsets=yes
 			srv_linux_thread_db=yes
 			;;
-  s390x-*-linux*)	srv_regobj=reg-s390x.o
+  s390x-*-linux*)	srv_regobj="reg-s390.o reg-s390x.o"
 			srv_tgtobj="linux-low.o linux-s390-low.o"
 			srv_linux_usrregs=yes
 			srv_linux_regsets=yes
diff -urNp gdb-orig/gdb/gdbserver/linux-low.c gdb-head/gdb/gdbserver/linux-low.c
--- gdb-orig/gdb/gdbserver/linux-low.c	2008-01-29 23:21:45.000000000 +0100
+++ gdb-head/gdb/gdbserver/linux-low.c	2008-01-29 23:24:41.000000000 +0100
@@ -107,6 +107,11 @@ static int thread_db_active;
 
 static int must_set_ptrace_flags;
 
+/* This flag is true iff we've just created or attached to a new inferior
+   but it has not stopped yet.  As soon as it does, we need to call the
+   low target's arch_setup callback.  */
+static int new_inferior;
+
 static void linux_resume_one_process (struct inferior_list_entry *entry,
 				      int step, int signal, siginfo_t *info);
 static void linux_resume (struct thread_resume *resume_info);
@@ -291,6 +296,7 @@ linux_create_inferior (char *program, ch
   new_process = add_process (pid);
   add_thread (pid, new_process, pid);
   must_set_ptrace_flags = 1;
+  new_inferior = 1;
 
   return pid;
 }
@@ -346,6 +352,8 @@ linux_attach (unsigned long pid)
   process = (struct process_info *) find_inferior_id (&all_processes, pid);
   process->stop_expected = 0;
 
+  new_inferior = 1;
+
   return 0;
 }
 
@@ -606,6 +614,16 @@ retry:
 
   (*childp)->last_status = *wstatp;
 
+  /* Architecture-specific setup after inferior is running.
+     This needs to happen after we have attached to the inferior
+     and it is stopped for the first time, but before we access
+     any inferior registers.  */
+  if (new_inferior)
+    {
+      the_low_target.arch_setup ();
+      new_inferior = 1;
+    }
+
   if (debug_threads
       && WIFSTOPPED (*wstatp))
     {
@@ -2060,7 +2078,6 @@ initialize_low (void)
   set_target_ops (&linux_target_ops);
   set_breakpoint_data (the_low_target.breakpoint,
 		       the_low_target.breakpoint_len);
-  the_low_target.arch_setup ();
   linux_init_signals ();
   linux_test_for_tracefork ();
 }
diff -urNp gdb-orig/gdb/gdbserver/linux-s390-low.c gdb-head/gdb/gdbserver/linux-s390-low.c
--- gdb-orig/gdb/gdbserver/linux-s390-low.c	2008-01-29 23:21:45.000000000 +0100
+++ gdb-head/gdb/gdbserver/linux-s390-low.c	2008-01-29 23:21:53.000000000 +0100
@@ -102,24 +102,61 @@ static const unsigned char s390_breakpoi
 static CORE_ADDR
 s390_get_pc ()
 {
-  unsigned long pc;
-  collect_register_by_name ("pswa", &pc);
+  if (register_size (0) == 4)
+    {
+      unsigned int pc;
+      collect_register_by_name ("pswa", &pc);
 #ifndef __s390x__
-  pc &= 0x7fffffff;
+      pc &= 0x7fffffff;
 #endif
-  return pc;
+      return pc;
+    }
+  else
+    {
+      unsigned long pc;
+      collect_register_by_name ("pswa", &pc);
+      return pc;
+    }
 }
 
 static void
 s390_set_pc (CORE_ADDR newpc)
 {
-  unsigned long pc = newpc;
+  if (register_size (0) == 4)
+    {
+      unsigned int pc = newpc;
 #ifndef __s390x__
-  pc |= 0x80000000;
+      pc |= 0x80000000;
 #endif
-  supply_register_by_name ("pswa", &pc);
+      supply_register_by_name ("pswa", &pc);
+    }
+  else
+    {
+      unsigned long pc = newpc;
+      supply_register_by_name ("pswa", &pc);
+    }
 }
 
+
+static void
+s390_arch_setup (void)
+{
+  /* Assume 31-bit inferior process.  */
+  init_registers_s390 ();
+
+  /* On a 64-bit host, check the low bit of the (31-bit) PSWM
+     -- if this is one, we actually have a 64-bit inferior.  */
+#ifdef __s390x__
+  {
+    unsigned int pswm;
+    collect_register_by_name ("pswm", &pswm);
+    if (pswm & 1)
+      init_registers_s390x ();
+  }
+#endif
+}
+
+
 static int
 s390_breakpoint_at (CORE_ADDR pc)
 {
@@ -130,11 +167,7 @@ s390_breakpoint_at (CORE_ADDR pc)
 
 
 struct linux_target_ops the_low_target = {
-#ifndef __s390x__
-  init_registers_s390,
-#else
-  init_registers_s390x,
-#endif
+  s390_arch_setup,
   s390_num_regs,
   s390_regmap,
   s390_cannot_fetch_register,
diff -urNp gdb-orig/gdb/gdbserver/regcache.c gdb-head/gdb/gdbserver/regcache.c
--- gdb-orig/gdb/gdbserver/regcache.c	2008-01-29 23:10:39.000000000 +0100
+++ gdb-head/gdb/gdbserver/regcache.c	2008-01-29 23:21:53.000000000 +0100
@@ -121,6 +121,15 @@ free_register_cache (void *regcache_p)
   free (regcache);
 }
 
+static void
+realloc_register_cache (struct inferior_list_entry *thread_p)
+{
+  struct thread_info *thread = (struct thread_info *) thread_p;
+
+  free_register_cache (inferior_regcache_data (thread));
+  set_inferior_regcache_data (thread, new_register_cache ());
+}
+
 void
 set_register_cache (struct reg *regs, int n)
 {
@@ -137,6 +146,9 @@ set_register_cache (struct reg *regs, in
     }
 
   register_bytes = offset / 8;
+
+  /* Re-allocate all pre-existing register caches.  */
+  for_each_inferior (&all_threads, realloc_register_cache);
 }
 
 void

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com