diff mbox series

[v2] nptl_db: Support different libpthread/ld.so load orders (bug 27744)

Message ID 87czuum5yb.fsf@oldenburg.str.redhat.com
State Superseded
Headers show
Series [v2] nptl_db: Support different libpthread/ld.so load orders (bug 27744) | expand

Commit Message

Florian Weimer April 16, 2021, 6:37 p.m. UTC
libthread_db is loaded once GDB encounters libpthread, and at this
point, ld.so may not have been processed by GDB yet. As a result,
_rtld_global cannot be accessed by regular means from libthread_db.
To make this work until GDB can be fixed, acess _rtld_global through
a pointer stored in libpthread.

The new test does not reproduce bug 27744 with
--disable-hardcoded-path-in-tests, but is still a valid smoke test.
With --enable-hardcoded-path-in-tests, it is necessary to avoid
add-symbol-file because this can tickle a GDB bug.

Fixes commit 1daccf403b1bd86370eb94edca794dc106d02039 ("nptl: Move
stack list variables into _rtld_global").

---
v2: Fixes suggested by Pedro.  The static tests is now actually built
    and run.

 nptl/Makefile                        |  19 ++++-
 nptl/pthread_create.c                |   8 +++
 nptl/tst-pthread-gdb-attach-static.c |   1 +
 nptl/tst-pthread-gdb-attach.c        | 134 +++++++++++++++++++++++++++++++++++
 nptl_db/structs.def                  |   3 +-
 nptl_db/td_init.c                    |  15 ++--
 nptl_db/thread_dbP.h                 |   2 +
 7 files changed, 171 insertions(+), 11 deletions(-)

Comments

Emil Velikov April 19, 2021, 10:50 a.m. UTC | #1
Hi Florian,

On Fri, 16 Apr 2021 at 19:37, Florian Weimer <fweimer@redhat.com> wrote:
>
> libthread_db is loaded once GDB encounters libpthread, and at this
> point, ld.so may not have been processed by GDB yet. As a result,
> _rtld_global cannot be accessed by regular means from libthread_db.
> To make this work until GDB can be fixed, acess _rtld_global through
> a pointer stored in libpthread.
>
> The new test does not reproduce bug 27744 with
> --disable-hardcoded-path-in-tests, but is still a valid smoke test.
> With --enable-hardcoded-path-in-tests, it is necessary to avoid
> add-symbol-file because this can tickle a GDB bug.
>
> Fixes commit 1daccf403b1bd86370eb94edca794dc106d02039 ("nptl: Move
> stack list variables into _rtld_global").
>
> ---
> v2: Fixes suggested by Pedro.  The static tests is now actually built
>     and run.
>
Thanks for the prompt fix. Both v1 and v2 fix the issue for me with
gimp and steam.

Feel free to add:
Tested-by: Emil Velikov <emil.velikov@collabora.com>

Are there any plans for glibc 2.33.1 or shall I ask the Arch Linux
maintainers to include this in the package?

Thanks again,
Emil
Florian Weimer April 19, 2021, 10:57 a.m. UTC | #2
* Emil Velikov:

> Thanks for the prompt fix. Both v1 and v2 fix the issue for me with
> gimp and steam.
>
> Feel free to add:
> Tested-by: Emil Velikov <emil.velikov@collabora.com>

Thanks.

> Are there any plans for glibc 2.33.1 or shall I ask the Arch Linux
> maintainers to include this in the package?

I'm going to backport it to the release branch, but we probably won't
make an actual 2.33.1 release from it.  So it depends on what Arch Linux
does with the release branch.

Florian
Emil Velikov April 27, 2021, 11:46 a.m. UTC | #3
Hi Florian,

On Mon, 19 Apr 2021 at 11:57, Florian Weimer <fweimer@redhat.com> wrote:

> > Are there any plans for glibc 2.33.1 or shall I ask the Arch Linux
> > maintainers to include this in the package?
>
> I'm going to backport it to the release branch, but we probably won't
> make an actual 2.33.1 release from it.  So it depends on what Arch Linux
> does with the release branch.
>
I can see the patch landed in master \o/ but it's missing in the 2.33 branch.
Did it slip through the cracks or you're simply EBUSY with the
nptl/c11 pthread to libc transition?

Thanks
Emil
Florian Weimer April 27, 2021, 12:11 p.m. UTC | #4
* Emil Velikov:

> On Mon, 19 Apr 2021 at 11:57, Florian Weimer <fweimer@redhat.com> wrote:
>
>> > Are there any plans for glibc 2.33.1 or shall I ask the Arch Linux
>> > maintainers to include this in the package?
>>
>> I'm going to backport it to the release branch, but we probably won't
>> make an actual 2.33.1 release from it.  So it depends on what Arch Linux
>> does with the release branch.

> I can see the patch landed in master \o/ but it's missing in the 2.33 branch.
> Did it slip through the cracks or you're simply EBUSY with the
> nptl/c11 pthread to libc transition?

I wanted to give it a test in Fedora rawhide, but we cannot rebuild
glibc there due to an unrelated issue right now.

I can backport it blindly if it helps.

Thanks,
Florian
Florian Weimer April 30, 2021, 7:20 a.m. UTC | #5
* Florian Weimer:

> * Emil Velikov:
>
>> On Mon, 19 Apr 2021 at 11:57, Florian Weimer <fweimer@redhat.com> wrote:
>>
>>> > Are there any plans for glibc 2.33.1 or shall I ask the Arch Linux
>>> > maintainers to include this in the package?
>>>
>>> I'm going to backport it to the release branch, but we probably won't
>>> make an actual 2.33.1 release from it.  So it depends on what Arch Linux
>>> does with the release branch.
>
>> I can see the patch landed in master \o/ but it's missing in the 2.33 branch.
>> Did it slip through the cracks or you're simply EBUSY with the
>> nptl/c11 pthread to libc transition?
>
> I wanted to give it a test in Fedora rawhide, but we cannot rebuild
> glibc there due to an unrelated issue right now.
>
> I can backport it blindly if it helps.

I've now backported it to the 2.33 release branch.

Thanks,
Florian
Emil Velikov May 4, 2021, 11:48 a.m. UTC | #6
On Fri, 30 Apr 2021 at 08:20, Florian Weimer <fweimer@redhat.com> wrote:

> I've now backported it to the 2.33 release branch.
>
Much appreciated, thank you.

-Emil
diff mbox series

Patch

diff --git a/nptl/Makefile b/nptl/Makefile
index 8fe92d43fa..e665d37e52 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -313,7 +313,8 @@  tests = tst-attr2 tst-attr3 tst-default-attr \
 	tst-thread-affinity-sched \
 	tst-pthread-defaultattr-free \
 	tst-pthread-attr-sigmask \
-	tst-pthread-timedlock-lockloop
+	tst-pthread-timedlock-lockloop \
+	tst-pthread-gdb-attach tst-pthread-gdb-attach-static
 
 tests-container =  tst-pthread-getattr
 
@@ -359,6 +360,19 @@  CPPFLAGS-test-cond-printers.c := $(CFLAGS-printers-tests)
 CPPFLAGS-test-rwlockattr-printers.c := $(CFLAGS-printers-tests)
 CPPFLAGS-test-rwlock-printers.c := $(CFLAGS-printers-tests)
 
+# Reuse the CFLAGS setting for the GDB attaching test.  It needs
+# debugging information.
+CFLAGS-tst-pthread-gdb-attach.c := $(CFLAGS-printers-tests)
+CPPFLAGS-tst-pthread-gdb-attach.c := $(CFLAGS-printers-tests)
+ifeq ($(build-shared)$(build-hardcoded-path-in-tests),yesno)
+CPPFLAGS-tst-pthread-gdb-attach.c += -DDO_ADD_SYMBOL_FILE=1
+else
+CPPFLAGS-tst-pthread-gdb-attach.c += -DDO_ADD_SYMBOL_FILE=0
+endif
+CFLAGS-tst-pthread-gdb-attach-static.c := $(CFLAGS-printers-tests)
+CPPFLAGS-tst-pthread-gdb-attach-static.c := \
+  $(CFLAGS-printers-tests) -DDO_ADD_SYMBOL_FILE=0
+
 ifeq ($(build-shared),yes)
 tests-printers-libs := $(shared-thread-library)
 else
@@ -430,7 +444,8 @@  link-libc-static := $(common-objpfx)libc.a $(static-gnulib) \
 tests-static += tst-stackguard1-static \
 		tst-cancel24-static \
 		tst-mutex8-static tst-mutexpi8-static tst-sem11-static \
-		tst-sem12-static tst-cond11-static
+		tst-sem12-static tst-cond11-static \
+		tst-pthread-gdb-attach-static
 
 tests += tst-cancel24-static
 
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 6c645aff48..f13d8e44a4 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -51,6 +51,14 @@  static td_thr_events_t __nptl_threads_events __attribute_used__;
 /* Pointer to descriptor with the last event.  */
 static struct pthread *__nptl_last_event __attribute_used__;
 
+#ifdef SHARED
+/* This variable is used to access _rtld_global from libthread_db.  If
+   GDB loads libpthread before ld.so, it is not possible to resolve
+   _rtld_global directly during libpthread initialization.  */
+static struct rtld_global *__nptl_rtld_global __attribute_used__
+  = &_rtld_global;
+#endif
+
 /* Number of threads running.  */
 unsigned int __nptl_nthreads = 1;
 
diff --git a/nptl/tst-pthread-gdb-attach-static.c b/nptl/tst-pthread-gdb-attach-static.c
new file mode 100644
index 0000000000..e159632cac
--- /dev/null
+++ b/nptl/tst-pthread-gdb-attach-static.c
@@ -0,0 +1 @@ 
+#include "tst-pthread-gdb-attach.c"
diff --git a/nptl/tst-pthread-gdb-attach.c b/nptl/tst-pthread-gdb-attach.c
new file mode 100644
index 0000000000..47f2e15347
--- /dev/null
+++ b/nptl/tst-pthread-gdb-attach.c
@@ -0,0 +1,134 @@ 
+/* Smoke testing GDB process attach with thread-local variable access.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+/* This test runs GDB against a forked copy of itself, to check
+   whether libthread_db can be loaded, and that access to thread-local
+   variables works.  */
+
+#include <support/check.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xthread.h>
+#include <support/xunistd.h>
+#include <support/xstdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+
+/* Starts out as zero, changed to 1 or 2 by the debugger, depending on
+   the thread.  */
+__thread volatile int altered_by_debugger;
+
+/* Writes the GDB script to run the test to PATH.  */
+static void
+write_gdbscript (const char *path, int tested_pid)
+{
+  FILE *fp = xfopen (path, "w");
+  fprintf (fp,
+           "set trace-commands on\n"
+           "set debug libthread-db 1\n"
+#if DO_ADD_SYMBOL_FILE
+           /* Do not do this unconditionally to work around a GDB
+              assertion failure: ../../gdb/symtab.c:6404:
+              internal-error: CORE_ADDR get_msymbol_address(objfile*,
+              const minimal_symbol*): Assertion `(objf->flags &
+              OBJF_MAINLINE) == 0' failed.  */
+           "add-symbol-file %1$s/nptl/tst-pthread-gdb-attach\n"
+#endif
+           "set auto-load safe-path %1$s/nptl_db\n"
+           "set libthread-db-search-path %1$s/nptl_db\n"
+           "attach %2$d\n",
+           support_objdir_root, tested_pid);
+  fputs ("break debugger_inspection_point\n"
+         "continue\n"
+         "thread 1\n"
+         "print altered_by_debugger\n"
+         "print altered_by_debugger = 1\n"
+         "thread 2\n"
+         "print altered_by_debugger\n"
+         "print altered_by_debugger = 2\n"
+         "continue\n",
+         fp);
+  xfclose (fp);
+}
+
+/* The test sets a breakpoint on this function and alters the
+   altered_by_debugger thread-local variable.  */
+void __attribute__ ((weak))
+debugger_inspection_point (void)
+{
+}
+
+/* Thread function for the test thread in the subprocess.  */
+static void *
+subprocess_thread (void *closure)
+{
+  /* Wait until altered_by_debugger changes the value away from 0.  */
+  while (altered_by_debugger == 0)
+    {
+      usleep (100 * 1000);
+      debugger_inspection_point ();
+    }
+
+  TEST_COMPARE (altered_by_debugger, 2);
+  return NULL;
+}
+
+/* This function implements the subprocess under test.  It creates a
+   second thread, waiting for its value to change to 2, and checks
+   that the main thread also changed its value to 1.  */
+static void
+in_subprocess (void)
+{
+  pthread_t thr = xpthread_create (NULL, subprocess_thread, NULL);
+  TEST_VERIFY (xpthread_join (thr) == NULL);
+  TEST_COMPARE (altered_by_debugger, 1);
+  _exit (0);
+}
+
+static int
+do_test (void)
+{
+  pid_t tested_pid = xfork ();
+  if (tested_pid == 0)
+    in_subprocess ();
+  char *tested_pid_string = xasprintf ("%d", tested_pid);
+
+  char *gdbscript;
+  xclose (create_temp_file ("tst-pthread-gdb-attach-", &gdbscript));
+  write_gdbscript (gdbscript, tested_pid);
+
+  pid_t gdb_pid = xfork ();
+  if (gdb_pid == 0)
+    {
+      clearenv ();
+      xdup2 (STDOUT_FILENO, STDERR_FILENO);
+      execlp ("gdb", "gdb", "-nx", "-batch", "-x", gdbscript, NULL);
+    }
+
+  int status;
+  TEST_COMPARE (xwaitpid (gdb_pid, &status, 0), gdb_pid);
+  TEST_COMPARE (status, 0);
+  TEST_COMPARE (xwaitpid (tested_pid, &status, 0), tested_pid);
+  TEST_COMPARE (status, 0);
+
+  free (tested_pid_string);
+  free (gdbscript);
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/nptl_db/structs.def b/nptl_db/structs.def
index 999a9fc35a..8a613dd2f5 100644
--- a/nptl_db/structs.def
+++ b/nptl_db/structs.def
@@ -100,8 +100,7 @@  DB_STRUCT_FIELD (pthread, dtvp)
 #endif
 
 #if !(IS_IN (libpthread) && !defined SHARED)
-DB_STRUCT (rtld_global)
-DB_RTLD_VARIABLE (_rtld_global)
+DB_VARIABLE (__nptl_rtld_global)
 #endif
 DB_RTLD_GLOBAL_FIELD (dl_tls_dtv_slotinfo_list)
 DB_RTLD_GLOBAL_FIELD (dl_stack_user)
diff --git a/nptl_db/td_init.c b/nptl_db/td_init.c
index 1d15681228..06b5adc5c2 100644
--- a/nptl_db/td_init.c
+++ b/nptl_db/td_init.c
@@ -33,13 +33,14 @@  td_init (void)
 bool
 __td_ta_rtld_global (td_thragent_t *ta)
 {
-  if (ta->ta_addr__rtld_global == 0
-      && td_mod_lookup (ta->ph, LD_SO, SYM__rtld_global,
-                        &ta->ta_addr__rtld_global) != PS_OK)
+  if (ta->ta_addr__rtld_global == 0)
     {
-      ta->ta_addr__rtld_global = (void*)-1;
-      return false;
+      psaddr_t rtldglobalp;
+      if (DB_GET_VALUE (rtldglobalp, ta, __nptl_rtld_global, 0) == TD_OK)
+        ta->ta_addr__rtld_global = rtldglobalp;
+      else
+        ta->ta_addr__rtld_global = (void *) -1;
     }
-  else
-    return ta->ta_addr__rtld_global != (void*)-1;
+
+  return ta->ta_addr__rtld_global != (void *)-1;
 }
diff --git a/nptl_db/thread_dbP.h b/nptl_db/thread_dbP.h
index 580a70c471..712fa3aeb6 100644
--- a/nptl_db/thread_dbP.h
+++ b/nptl_db/thread_dbP.h
@@ -108,6 +108,8 @@  struct td_thragent
 # undef DB_SYMBOL
 # undef DB_VARIABLE
 
+  psaddr_t ta_addr__rtld_global;
+
   /* The method of locating a thread's th_unique value.  */
   enum
     {