Re: [PATCH 0/3] Create arch_lwp_info class hierarchy

[Pedro Alves <palves at redhat dot com>]

On 08/12/2017 12:29 PM, Simon Marchi wrote:

> But then I realized that I forgot to include the header for s390, and
> the compiler (when building for a s390 host) didn't warn me.  This is
> dangerous and fragile since we end up with two definitions of
> arch_lwp_info (the s390 one and that fallback one), and nothing to warn
> about it.

Not sure whether that's really such a bad problem.  It seems like the
sort of thing that'd crash quite visibly/quickly if you actually try to
run the resulting binary.  It's not like we're adding ports
every day.  :-)

> So I changed it to listing explicitly the architectures that
> don't defined their own arch_lwp_info:
> 
> ...
> #elif  defined __alpha__ || defined __powerpc__ || ...
> /* Define a dummy arch_lwp_info for arches that don't define one.  */
> struct arch_lwp_info {};
> #else
> # error "Missing arch-specific include."
> #endif
> 
> That would work, but requires listing all the arches that need the
> fallback definition of arch_lwp_info, so it gets pretty ugly.
> 
> Any idea to make this simple but safe?  Otherwise, I'll just go with the
> current version of the patch.

There's a 3rd, much simpler option.

The problem we ran into is one of blurred division of responsibility: the
arch-specific code is responsible for allocating the arch-specific
arch_lwp_info, which it must be, because it's not possible to allocate an
incomplete type (must know its size), but we free the object in common code,
where the type is opaque.

We can solve the original xfree poisoning problem by simply making the
arch-specific code responsible for releasing arch_lwp_info too, where the
type is known, just like it is responsible for allocating it.

See below a proof of concept doing that for the x86 port.  Making other
archs do the same should be trivial.  Using new/delete/cdtors/in-class
initialization for arch_lwp_info would be a separate, orthogonal change
(and so would class-ification of linux_nat_new_thread, linux_target_ops,
etc.).

>From 40188bdb4ff7eae2876960808672c72529c934b5 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 14 Aug 2017 12:16:11 +0100
Subject: [PATCH] arch-specific code deletes arch-specific bits

---
 gdb/gdbserver/linux-low.h     |  4 ++++
 gdb/gdbserver/linux-x86-low.c |  1 +
 gdb/linux-nat.c               | 18 +++++++++++++++++-
 gdb/linux-nat.h               |  3 +++
 gdb/nat/x86-linux.c           |  6 ++++++
 gdb/nat/x86-linux.h           |  4 ++++
 gdb/x86-linux-nat.c           |  1 +
 7 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index 4e7fbec..e7f76d1 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -194,6 +194,10 @@ struct linux_target_ops
      allocate it here.  */
   void (*new_thread) (struct lwp_info *);
 
+  /* Hook to call when a thread is being deleted.  If extra per-thread
+     architecture-specific data is needed, delete it here.  */
+  void (*delete_thread) (struct lwp_info *);
+
   /* Hook to call, if any, when a new fork is attached.  */
   void (*new_fork) (struct process_info *parent, struct process_info *child);
 
diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index 12c7a1b..338d203 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -2961,6 +2961,7 @@ struct linux_target_ops the_low_target =
   x86_siginfo_fixup,
   x86_linux_new_process,
   x86_linux_new_thread,
+  x86_linux_delete_thread,
   x86_linux_new_fork,
   x86_linux_prepare_to_resume,
   x86_linux_process_qsupported,
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 3dc3637..6be4f5b 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -197,6 +197,9 @@ static struct target_ops linux_ops_saved;
 /* The method to call, if any, when a new thread is attached.  */
 static void (*linux_nat_new_thread) (struct lwp_info *);
 
+/* The method to call, if any, when a thread is destroyed.  */
+static void (*linux_nat_delete_thread) (struct lwp_info *);
+
 /* The method to call, if any, when a new fork is attached.  */
 static linux_nat_new_fork_ftype *linux_nat_new_fork;
 
@@ -837,7 +840,9 @@ static int check_ptrace_stopped_lwp_gone (struct lwp_info *lp);
 static void
 lwp_free (struct lwp_info *lp)
 {
-  xfree (lp->arch_private);
+  /* Let the arch specific bits release arch_lwp_info.  */
+  if (linux_nat_delete_thread != NULL)
+    linux_nat_delete_thread (lp);
   delete lp;
 }
 
@@ -4863,6 +4868,17 @@ linux_nat_set_new_thread (struct target_ops *t,
   linux_nat_new_thread = new_thread;
 }
 
+/* Register a method to call whenever a new thread is attached.  */
+void
+linux_nat_set_delete_thread (struct target_ops *t,
+			     void (*delete_thread) (struct lwp_info *))
+{
+  /* Save the pointer.  We only support a single registered instance
+     of the GNU/Linux native target, so we do not need to map this to
+     T.  */
+  linux_nat_delete_thread = delete_thread;
+}
+
 /* See declaration in linux-nat.h.  */
 
 void
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index a783674..8d8e033 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -169,6 +169,9 @@ void linux_nat_add_target (struct target_ops *);
 /* Register a method to call whenever a new thread is attached.  */
 void linux_nat_set_new_thread (struct target_ops *, void (*) (struct lwp_info *));
 
+/* Register a method to call whenever a new thread is deleted.  */
+void linux_nat_set_delete_thread (struct target_ops *,
+				  void (*) (struct lwp_info *));
 
 /* Register a method to call whenever a new fork is attached.  */
 typedef void (linux_nat_new_fork_ftype) (struct lwp_info *parent,
diff --git a/gdb/nat/x86-linux.c b/gdb/nat/x86-linux.c
index b499e74..e2e9cee 100644
--- a/gdb/nat/x86-linux.c
+++ b/gdb/nat/x86-linux.c
@@ -65,6 +65,12 @@ x86_linux_new_thread (struct lwp_info *lwp)
   lwp_set_debug_registers_changed (lwp, 1);
 }
 
+void
+x86_linux_delete_thread (struct lwp_info *lwp)
+{
+  xfree (lwp_arch_private_info (lwp));
+}
+
 /* See nat/x86-linux.h.  */
 
 void
diff --git a/gdb/nat/x86-linux.h b/gdb/nat/x86-linux.h
index 1b7fee4..32c4922 100644
--- a/gdb/nat/x86-linux.h
+++ b/gdb/nat/x86-linux.h
@@ -39,6 +39,10 @@ extern int lwp_debug_registers_changed (struct lwp_info *lwp);
 
 extern void x86_linux_new_thread (struct lwp_info *lwp);
 
+/* Function to call when a thread is being deleted.  */
+
+extern void x86_linux_delete_thread (struct lwp_info *lwp);
+
 /* Function to call prior to resuming a thread.  */
 
 extern void x86_linux_prepare_to_resume (struct lwp_info *lwp);
diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c
index 2c4afb1..d0480bd 100644
--- a/gdb/x86-linux-nat.c
+++ b/gdb/x86-linux-nat.c
@@ -387,6 +387,7 @@ x86_linux_add_target (struct target_ops *t)
 {
   linux_nat_add_target (t);
   linux_nat_set_new_thread (t, x86_linux_new_thread);
+  linux_nat_set_delete_thread (t, x86_linux_delete_thread);
   linux_nat_set_new_fork (t, x86_linux_new_fork);
   linux_nat_set_forget_process (t, x86_forget_process);
   linux_nat_set_prepare_to_resume (t, x86_linux_prepare_to_resume);
-- 
2.5.5