[RFA/RFC] Fix broken user-thread debugging on x86-solaris

[RFA/RFC] Fix broken user-thread debugging on x86-solaris

[Joel Brobecker]

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

Hello,

While testing GDB on x86-solaris, I noticed that the debugger now prints
a warning during startup:

    % ./gdb
    [GDB will not be able to debug user-mode threads: ld.so.1: gdb.ref: fatal: relocation error: file /usr/lib/libthread_db.so.1: symbol ps_lgetLDT: referenced symbol not found]
    GNU gdb 6.8.50.20080311-cvs
    Copyright (C) 2008 Free Software Foundation, Inc.
    [...]

The problem was introduced when we killed the TM file, which was
defining the TM_I386SOL2_H macro that a couple of units were actually
using (ugh!).

Basically, the problem on x86-solaris is that, in order to load
libthread_db.so, we need to define ps_lgetLDT or the loading
will fail due to this symbol being undefined.  This function is
defined inside sol-thread.c but is conditionalized on TM_I386SOL2_H
being defined:

    #ifdef TM_I386SOL2_H
    
    /* Reads the local descriptor table of a LWP.  */
    
    ps_err_e
    ps_lgetLDT (gdb_ps_prochandle_t ph, lwpid_t lwpid,
                struct ssd *pldt)
    {

Similarly, a couple of other functions inside procfs.c that
this function uses are conditionalized on the same macro:
proc_get_LDT_entry and procfs_find_LDT_entry. This stuff is
entirely x86-solaris specific, so cannot be enabled on sparc-solaris.

I think the most classical way of fixing this would be to move these
to their own file, such as sol-x86-thread.c. But I'm not sure whether
this is going to be the prefered solution in this case, because
the procfs routines need access to certain fields of a structure
that is not public (struct procinfo).

In this version, I went to the simplest, which is to have configure
define a new macro (HAVE_X86_SOLARIS_USER_THREADS) when on x86-solaris
and the thread_db library is available.  I then replaced TM_I386SOL2_H
by the new macro.

If I were to move the 3 functions to their own file, I would have
to create procfs.h to provide an opaque definition of struct procinfo.
I would also add a few accessor routines (to get the pid and the ctl_fd),
and make a few routines such as find_procinfo or proc_get_gregs non-static.
I don't mind doing this work, but I want to make sure others prefer it too.

2008-03-12  Joel Brobecker  <brobecker@adacore.com>

        * configure.ac (HAVE_X86_SOLARIS_USER_THREADS): Define this macro
        if we are building a native x86-solaris debugger that has access
        to user-threads.
        * configure, config.in: Regenerate.
        * sol-thread.c, procfs.c : Use HAVE_X86_SOLARIS_USER_THREADS
        in place of TM_I386SOL2_H.

Tested on x86-solaris. No regression.
Opinions?

BTW: We should fix this in the branch as well.

Thanks,
-- 
Joel

[-- Attachment #2: x86-user-threads.diff --]
[-- Type: text/plain, Size: 3155 bytes --]

Index: configure.ac
===================================================================
--- configure.ac	(revision 34930)
+++ configure.ac	(working copy)
@@ -1221,6 +1221,11 @@ if test ${build} = ${host} -a ${host} = 
             [Define if <proc_service.h> on solaris uses int instead of
              size_t, and assorted other type changes.])
 	 fi
+         if test "${gdb_host_cpu}" = "i386" ; then
+            AC_DEFINE(HAVE_X86_SOLARIS_USER_THREADS, 1,
+            [Define if the thread layer uses the user-thread debug library
+             on an x86-solaris system])
+         fi
       else
          AC_MSG_RESULT(no)
       fi
Index: configure
===================================================================
--- configure	(revision 34930)
+++ configure	(working copy)
@@ -22865,6 +22865,13 @@ cat >>confdefs.h <<\_ACEOF
 _ACEOF
 
 	 fi
+         if test "${gdb_host_cpu}" = "i386" ; then
+
+cat >>confdefs.h <<\_ACEOF
+#define HAVE_X86_SOLARIS_USER_THREADS 1
+_ACEOF
+
+         fi
       else
          echo "$as_me:$LINENO: result: no" >&5
 echo "${ECHO_T}no" >&6
Index: config.in
===================================================================
--- config.in	(revision 34930)
+++ config.in	(working copy)
@@ -482,6 +482,10 @@
 /* Define to 1 if `vfork' works. */
 #undef HAVE_WORKING_VFORK
 
+/* Define if the thread layer uses the user-thread debug library on an
+   x86-solaris system */
+#undef HAVE_X86_SOLARIS_USER_THREADS
+
 /* Define to 1 if you have the `XML_StopParser' function. */
 #undef HAVE_XML_STOPPARSER
 
Index: sol-thread.c
===================================================================
--- sol-thread.c	(revision 34930)
+++ sol-thread.c	(working copy)
@@ -1298,7 +1298,7 @@ ps_pdmodel (gdb_ps_prochandle_t ph, int 
 }
 #endif /* PR_MODEL_LP64 */
 
-#ifdef TM_I386SOL2_H
+#ifdef HAVE_X86_SOLARIS_USER_THREADS
 
 /* Reads the local descriptor table of a LWP.  */
 
@@ -1326,7 +1326,7 @@ ps_lgetLDT (gdb_ps_prochandle_t ph, lwpi
     /* LDT not found.  */
     return PS_ERR;
 }
-#endif /* TM_I386SOL2_H */
+#endif /* HAVE_X86_SOLARIS_USER_THREADS */
 \f
 
 /* Convert PTID to printable form.  */
Index: procfs.c
===================================================================
--- procfs.c	(revision 34930)
+++ procfs.c	(working copy)
@@ -3028,7 +3028,7 @@ proc_set_watchpoint (procinfo *pi, CORE_
 #endif
 }
 
-#ifdef TM_I386SOL2_H		/* Is it hokey to use this? */
+#ifdef HAVE_X86_SOLARIS_USER_THREADS
 
 #include <sys/sysi86.h>
 
@@ -3120,7 +3120,7 @@ proc_get_LDT_entry (procinfo *pi, int ke
 #endif
 }
 
-#endif /* TM_I386SOL2_H */
+#endif /* HAVE_X86_SOLARIS_USER_THREADS */
 
 /* =============== END, non-thread part of /proc  "MODULE" =============== */
 
@@ -5549,7 +5549,7 @@ procfs_stopped_by_watchpoint (ptid_t pti
   return 0;
 }
 
-#ifdef TM_I386SOL2_H
+#ifdef HAVE_X86_SOLARIS_USER_THREADS
 /*
  * Function: procfs_find_LDT_entry
  *
@@ -5587,7 +5587,7 @@ procfs_find_LDT_entry (ptid_t ptid)
   /* Find the matching entry and return it. */
   return proc_get_LDT_entry (pi, key);
 }
-#endif /* TM_I386SOL2_H */
+#endif /* HAVE_X86_SOLARIS_USER_THREADS */
 
 /*
  * Memory Mappings Functions:

Re: [RFA/RFC] Fix broken user-thread debugging on x86-solaris

[Daniel Jacobowitz]

On Wed, Mar 12, 2008 at 10:08:14AM -0700, Joel Brobecker wrote:
> In this version, I went to the simplest, which is to have configure
> define a new macro (HAVE_X86_SOLARIS_USER_THREADS) when on x86-solaris
> and the thread_db library is available.  I then replaced TM_I386SOL2_H
> by the new macro.

How about just #if defined(__i386__) || defined(__x86_64__), if we're
going to use macros?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA/RFC] Fix broken user-thread debugging on x86-solaris

[Joel Brobecker]

> > In this version, I went to the simplest, which is to have configure
> > define a new macro (HAVE_X86_SOLARIS_USER_THREADS) when on x86-solaris
> > and the thread_db library is available.  I then replaced TM_I386SOL2_H
> > by the new macro.
> 
> How about just #if defined(__i386__) || defined(__x86_64__), if we're
> going to use macros?

I don't mind, but we also need to check for solaris too, since
procfs is used with other OSes as well. So something like this?

    #if (defined(__i386__) || defined(__x86_64__)) && defined (sun)

It's funny you would prefer this approach over the more functional
macro - I thought that people would crucify me if I suggested something
like this :). Would you prefer to isolate the code in a separate file
like I also suggested?

-- 
Joel

Re: [RFA/RFC] Fix broken user-thread debugging on x86-solaris

[Daniel Jacobowitz]

On Wed, Mar 12, 2008 at 11:04:45AM -0700, Joel Brobecker wrote:
> I don't mind, but we also need to check for solaris too, since
> procfs is used with other OSes as well. So something like this?
> 
>     #if (defined(__i386__) || defined(__x86_64__)) && defined (sun)
> 
> It's funny you would prefer this approach over the more functional
> macro - I thought that people would crucify me if I suggested something
> like this :). Would you prefer to isolate the code in a separate file
> like I also suggested?

I have no preference whatsoever; just whatever is easier in practice.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA/RFC] Fix broken user-thread debugging on x86-solaris

[Joel Brobecker]

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

> I have no preference whatsoever; just whatever is easier in practice.

That works for me :) - we can always adjust later if I hear some more
comments. Here is what I ended up checking-in (I used the opportunity
to add a little comment explaining why the function is needed, as well as
grouping the two LDT-related functions in procfs.c together).

2008-02-12  Joel Brobecker  <brobecker@gnat.com>

        * sol-thread.c: Replace use of TM_I386SOL2_H by an expression
        that is true only on x86-solaris and x86_64-solaris.
        * procfs.c: Likewise. Move procfs_find_LDT_entry up together
        with proc_get_LDT_entry.

Tested on x86-solaris, no regression. Applied to head and branch.

Thanks!
-- 
Joel

[-- Attachment #2: x86-solaris-thread.diff --]
[-- Type: text/plain, Size: 3769 bytes --]

Index: sol-thread.c
===================================================================
RCS file: /cvs/src/src/gdb/sol-thread.c,v
retrieving revision 1.63
diff -u -p -r1.63 sol-thread.c
--- sol-thread.c	23 Jan 2008 11:26:28 -0000	1.63
+++ sol-thread.c	12 Mar 2008 18:39:54 -0000
@@ -1283,9 +1283,12 @@ ps_pdmodel (gdb_ps_prochandle_t ph, int 
 }
 #endif /* PR_MODEL_LP64 */
 
-#ifdef TM_I386SOL2_H
+#if (defined(__i386__) || defined(__x86_64__)) && defined (sun)
 
-/* Reads the local descriptor table of a LWP.  */
+/* Reads the local descriptor table of a LWP.
+
+   This function is necessary on x86-solaris only.  Without it, the loading
+   of libthread_db would fail because of ps_lgetLDT being undefined.  */
 
 ps_err_e
 ps_lgetLDT (gdb_ps_prochandle_t ph, lwpid_t lwpid,
@@ -1311,7 +1314,7 @@ ps_lgetLDT (gdb_ps_prochandle_t ph, lwpi
     /* LDT not found.  */
     return PS_ERR;
 }
-#endif /* TM_I386SOL2_H */
+#endif
 \f
 
 /* Convert PTID to printable form.  */
Index: procfs.c
===================================================================
RCS file: /cvs/src/src/gdb/procfs.c,v
retrieving revision 1.85
diff -u -p -r1.85 procfs.c
--- procfs.c	25 Jan 2008 00:09:49 -0000	1.85
+++ procfs.c	12 Mar 2008 18:39:55 -0000
@@ -2919,7 +2919,7 @@ proc_set_watchpoint (procinfo *pi, CORE_
 #endif
 }
 
-#ifdef TM_I386SOL2_H		/* Is it hokey to use this? */
+#if (defined(__i386__) || defined(__x86_64__)) && defined (sun)
 
 #include <sys/sysi86.h>
 
@@ -3011,7 +3011,45 @@ proc_get_LDT_entry (procinfo *pi, int ke
 #endif
 }
 
-#endif /* TM_I386SOL2_H */
+/*
+ * Function: procfs_find_LDT_entry
+ *
+ * Input:
+ *   ptid_t ptid;	// The GDB-style pid-plus-LWP.
+ *
+ * Return:
+ *   pointer to the corresponding LDT entry.
+ */
+
+struct ssd *
+procfs_find_LDT_entry (ptid_t ptid)
+{
+  gdb_gregset_t *gregs;
+  int            key;
+  procinfo      *pi;
+
+  /* Find procinfo for the lwp. */
+  if ((pi = find_procinfo (PIDGET (ptid), TIDGET (ptid))) == NULL)
+    {
+      warning (_("procfs_find_LDT_entry: could not find procinfo for %d:%d."),
+	       PIDGET (ptid), TIDGET (ptid));
+      return NULL;
+    }
+  /* get its general registers. */
+  if ((gregs = proc_get_gregs (pi)) == NULL)
+    {
+      warning (_("procfs_find_LDT_entry: could not read gregs for %d:%d."),
+	       PIDGET (ptid), TIDGET (ptid));
+      return NULL;
+    }
+  /* Now extract the GS register's lower 16 bits. */
+  key = (*gregs)[GS] & 0xffff;
+
+  /* Find the matching entry and return it. */
+  return proc_get_LDT_entry (pi, key);
+}
+
+#endif
 
 /* =============== END, non-thread part of /proc  "MODULE" =============== */
 
@@ -5331,46 +5369,6 @@ procfs_stopped_by_watchpoint (ptid_t pti
   return 0;
 }
 
-#ifdef TM_I386SOL2_H
-/*
- * Function: procfs_find_LDT_entry
- *
- * Input:
- *   ptid_t ptid;	// The GDB-style pid-plus-LWP.
- *
- * Return:
- *   pointer to the corresponding LDT entry.
- */
-
-struct ssd *
-procfs_find_LDT_entry (ptid_t ptid)
-{
-  gdb_gregset_t *gregs;
-  int            key;
-  procinfo      *pi;
-
-  /* Find procinfo for the lwp. */
-  if ((pi = find_procinfo (PIDGET (ptid), TIDGET (ptid))) == NULL)
-    {
-      warning (_("procfs_find_LDT_entry: could not find procinfo for %d:%d."),
-	       PIDGET (ptid), TIDGET (ptid));
-      return NULL;
-    }
-  /* get its general registers. */
-  if ((gregs = proc_get_gregs (pi)) == NULL)
-    {
-      warning (_("procfs_find_LDT_entry: could not read gregs for %d:%d."),
-	       PIDGET (ptid), TIDGET (ptid));
-      return NULL;
-    }
-  /* Now extract the GS register's lower 16 bits. */
-  key = (*gregs)[GS] & 0xffff;
-
-  /* Find the matching entry and return it. */
-  return proc_get_LDT_entry (pi, key);
-}
-#endif /* TM_I386SOL2_H */
-
 /*
  * Memory Mappings Functions:
  */