[PATCH 0/4] libthread_db initialization changes related to upcoming glibc-2.34

[PATCH 0/4] libthread_db initialization changes related to upcoming glibc-2.34

[Kevin Buettner]

This patch series contains changes necessary for GDB to be able to
debug threaded programs using glibc-2.34.

The actual change to GDB sources are contained in the first patch; the
remainder deal with several testing issues that I ran into.

Kevin Buettner (4):
  libthread_db initialization changes related to upcoming glibc-2.34
  testsuite/glib-2.34: Match/consume optional libthread_db related
    output
  print-symbol-loading.exp: Allow libc symbols to be already loaded
  mi-sym-info.exp: Increase timeout for 114-symbol-info-functions

 gdb/linux-thread-db.c                         | 24 +++++++-
 gdb/solib.c                                   | 10 +++-
 .../gdb.base/execl-update-breakpoints.exp     |  1 +
 .../gdb.base/fork-print-inferior-events.exp   |  3 +-
 .../gdb.base/print-symbol-loading.exp         | 15 ++++-
 gdb/testsuite/gdb.mi/mi-sym-info.exp          | 56 ++++++++++---------
 6 files changed, 73 insertions(+), 36 deletions(-)

-- 
2.31.1

[PATCH 1/4] libthread_db initialization changes related to upcoming glibc-2.34

[Kevin Buettner]

This commit makes some adjustments to accomodate the upcoming
glibc-2.34 release.  Beginning with glibc-2.34, functionality formerly
contained in libpthread has been moved to libc.  For the time being,
libpthread.so still exists in the file system, but it won't show up in
ldd output and therefore won't be able to trigger initialization of
libthread_db related code.  E.g...

Fedora 34 / glibc-2.33.9000:

[kev@f34-2 gdb]$ ldd testsuite/outputs/gdb.threads/tls/tls
	linux-vdso.so.1 (0x00007ffcf94fa000)
	libstdc++.so.6 => /lib64/libstdc++.so.6 (0x00007ff0ba9af000)
	libm.so.6 => /lib64/libm.so.6 (0x00007ff0ba8d4000)
	libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007ff0ba8b9000)
	libc.so.6 => /lib64/libc.so.6 (0x00007ff0ba6c6000)
	/lib64/ld-linux-x86-64.so.2 (0x00007ff0babf0000)

Fedora 34 / glibc-2.33:

[kev@f34-1 gdb]$ ldd testsuite/outputs/gdb.threads/tls/tls
	linux-vdso.so.1 (0x00007fff32dc0000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f815f6de000)
	libstdc++.so.6 => /lib64/libstdc++.so.6 (0x00007f815f4bf000)
	libm.so.6 => /lib64/libm.so.6 (0x00007f815f37b000)
	libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007f815f360000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f815f191000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f815f721000)

Note that libpthread is missing from the ldd output for the
glibc-2.33.9000 machine.

This means that (unless we happen to think of some entirely different
mechanism), we'll now need to potentially match "libc" in addition to
"libpthread" as libraries which might be thread libraries.  This
accounts for the change made in solib.c.  Note that the new code
attempts to match "/libc." via strstr().  That trailing dot (".")
avoids inadvertently matching libraries such as libcrypt (and
all the other many libraries which begin with "libc").

To avoid attempts to load libthread_db when encountering older
versions of libc, we now attempt to find "pthread_create" (which is a
symbol that we'd expect to be in any pthread library) in the
associated objfile.  This accounts for the changes in
linux-thread-db.c.

I think that other small adjustments will need to be made elsewhere
too.  I've been working through regressions on my glibc-2.33.9000
machine; I've fixed some fairly "obvious" changes in the testsuite
(which are in other commits).  For the rest, it's not yet clear to me
whether the handful of remaining failures represent a problem in glibc
or gdb.  I'm still investigating, however, I'll note that these are
problems that I only see on my glibc-2.33.9000 machine.

gdb/ChangeLog:

	* solib.c (libpthread_name_p): Match "libc" in addition
	to "libpthread".
	* linux-thread-db.c (libpthread_objfile_p): New function.
	(libpthread_name_p): Adjust preexisting callers to use
	libpthread_objfile_p().
---
 gdb/linux-thread-db.c | 24 +++++++++++++++++++++---
 gdb/solib.c           | 10 ++++++++--
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index d1e8c22ac96..5b4e5a8654f 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -799,6 +799,24 @@ check_thread_db (struct thread_db_info *info, bool log_progress)
   return test_passed;
 }
 
+/* Predicate which tests whether objfile OBJ refers to the library
+   containing pthread related symbols.  Historically, this library has
+   been named in such a way that looking for "libpthread" in the name
+   was sufficient to identify it.  As of glibc-2.34, the C library
+   (libc) contains the thread library symbols.  Therefore we check
+   that the name matches a possible thread library, but we also check
+   that it contains at least one of the symbols (pthread_create) that
+   we'd expect to find in the thread library.  */
+
+static bool
+libpthread_objfile_p (objfile *obj)
+{
+  return (libpthread_name_p (objfile_name (obj))
+          && lookup_minimal_symbol ("pthread_create",
+	                            NULL,
+				    obj).minsym != NULL);
+}
+
 /* Attempt to initialize dlopen()ed libthread_db, described by INFO.
    Return true on success.
    Failure could happen if libthread_db does not have symbols we expect,
@@ -1072,7 +1090,7 @@ try_thread_db_load_from_pdir (const char *subdir)
     return false;
 
   for (objfile *obj : current_program_space->objfiles ())
-    if (libpthread_name_p (objfile_name (obj)))
+    if (libpthread_objfile_p (obj))
       {
 	if (try_thread_db_load_from_pdir_1 (obj, subdir))
 	  return true;
@@ -1181,7 +1199,7 @@ static bool
 has_libpthread (void)
 {
   for (objfile *obj : current_program_space->objfiles ())
-    if (libpthread_name_p (objfile_name (obj)))
+    if (libpthread_objfile_p (obj))
       return true;
 
   return false;
@@ -1286,7 +1304,7 @@ thread_db_new_objfile (struct objfile *objfile)
 	 of the list of shared libraries to load, and in an app of several
 	 thousand shared libraries, this can otherwise be painful.  */
       && ((objfile->flags & OBJF_MAINLINE) != 0
-	  || libpthread_name_p (objfile_name (objfile))))
+	  || libpthread_objfile_p (objfile)))
     check_for_thread_db ();
 }
 
diff --git a/gdb/solib.c b/gdb/solib.c
index 2df52118143..88b201ff6a0 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -900,12 +900,18 @@ Do you need \"set solib-search-path\" or \"set sysroot\"?"),
 
    Uses a fairly simplistic heuristic approach where we check
    the file name against "/libpthread".  This can lead to false
-   positives, but this should be good enough in practice.  */
+   positives, but this should be good enough in practice.
+
+   In glibc-2.34 (and later), functions formerly residing
+   in libpthread have been moved to libc, so "/libc." needs
+   to be checked too.  (Matching the "." will avoid matching
+   libraries such as libcrypt.)  */
 
 bool
 libpthread_name_p (const char *name)
 {
-  return (strstr (name, "/libpthread") != NULL);
+  return (strstr (name, "/libpthread") != NULL
+          || strstr (name, "/libc.") != NULL );
 }
 
 /* Return non-zero if SO is the libpthread shared library.  */
-- 
2.31.1

[PATCH 2/4] testsuite/glib-2.34: Match/consume optional libthread_db related output

[Kevin Buettner]

When using glibc-2.34, we now see messages related to the loading of
the thread library for non-thread programs.  E.g.  for the test case,
gdb.base/execl-update-breakpoints.exp, we will see the following when
starting the program:

(gdb) break -qualified main
Breakpoint 1 at 0x100118c: file /ironwood1/sourceware-git/f34-2-glibc244_fix/bld/../../worktree-glibc244_fix/gdb/testsuite/gdb.base/execl-update-breakpoints.c, line 34.
(gdb) run
Starting program: [...]/execl-update-breakpoints1
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

The two lines of output related to libthread_db are new; we didn't see
these in the past.  This is a side effect of libc now containing the
pthread API - we can no longer tell whether the program is
multi-threaded by simply looking for libpthread.so.  That said, I
think that we now want to load libthread_db anyway since it's used to
resolve TLS variables; i.e. we need it for correctly determining the
value of errno.

This commit adds the necessary regular expressions to match this
(optional) additional output in the two tests which were failing
without it.

gdb/testsuite/ChangeLog:

	* gdb.base/execl-update-breakpoints.exp: Add regular
	expression for optionally matching output related to
	libthread_db.
	* gdb.base/fork-print-inferior-events.exp: Likewise.
---
 gdb/testsuite/gdb.base/execl-update-breakpoints.exp   | 1 +
 gdb/testsuite/gdb.base/fork-print-inferior-events.exp | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/execl-update-breakpoints.exp b/gdb/testsuite/gdb.base/execl-update-breakpoints.exp
index 63e741e85cd..f9cd7e861c4 100644
--- a/gdb/testsuite/gdb.base/execl-update-breakpoints.exp
+++ b/gdb/testsuite/gdb.base/execl-update-breakpoints.exp
@@ -132,6 +132,7 @@ proc test { always_inserted } {
 	"Continuing\\.\r\n" \
 	"${not_nl} is executing new program: ${not_nl}\r\n" \
 	"(Reading ${not_nl} from remote target\\.\\.\\.\r\n)*" \
+	"(?:.Thread debugging using .*? enabled.\r\nUsing .*? library .*?\\.\r\n)?" \
 	"\r\n" \
 	"Breakpoint 1, main.*$gdb_prompt $"
     set message "continue across exec"
diff --git a/gdb/testsuite/gdb.base/fork-print-inferior-events.exp b/gdb/testsuite/gdb.base/fork-print-inferior-events.exp
index 10afaf6110a..eda0f50ca7b 100644
--- a/gdb/testsuite/gdb.base/fork-print-inferior-events.exp
+++ b/gdb/testsuite/gdb.base/fork-print-inferior-events.exp
@@ -59,6 +59,7 @@ set detach_child_re "${reading_re}\\\[Detaching after fork from child .*\\\]\r\n
 set detach_parent_re "${reading_re}\\\[Detaching after fork from parent .*\\\]\r\n"
 set new_inf_re "${reading_re}\\\[New inferior $decimal \\(.*\\)\\\]\r\n"
 set inf_detached_re "${reading_re}\\\[Inferior $decimal \\(.*\\) detached\\\]\r\n"
+set thread_db_re "(?:\\\[Thread debugging using .*? enabled\\\]\r\nUsing .*? library .*?\\.\r\n)?"
 
 set expected_output [list \
 			 "${attach_child_re}${new_inf_re}${detach_parent_re}${inf_detached_re}" \
@@ -84,7 +85,7 @@ foreach_with_prefix print_inferior_events { "on" "off" } {
 	    set output [lindex $expected_output $i]
 	    # Always add the "Starting program..." string so that we
 	    # match exactly the lines we want.
-	    set output "Starting program: $binfile\\s*\r\n${output}${exited_normally_re}"
+	    set output "Starting program: $binfile\\s*\r\n${thread_db_re}${output}${thread_db_re}${exited_normally_re}"
 	    set i [expr $i + 1]
 	    gdb_test "run" $output
 	}
-- 
2.31.1

[PATCH 3/4] print-symbol-loading.exp: Allow libc symbols to be already loaded

[Kevin Buettner]

One consequence of changing libpthread_name_p() in solib.c to (also)
match libc is that the symbols for libc will now be loaded by
solib_add() in solib.c.  I think this is mostly harmless because
we'll likely want these symbols to be loaded anyway, but it did cause
two failures in gdb.base/print-symbol-loading.exp.

Specifically...

1)

sharedlibrary .*
(gdb) PASS: gdb.base/print-symbol-loading.exp: shlib off: load shared-lib

now looks like this:

sharedlibrary .*
Symbols already loaded for /lib64/libc.so.6
(gdb) PASS: gdb.base/print-symbol-loading.exp: shlib off: load shared-lib

2)

sharedlibrary .*
Loading symbols for shared libraries: .*
(gdb) PASS: gdb.base/print-symbol-loading.exp: shlib brief: load shared-lib

now looks like this:

sharedlibrary .*
Loading symbols for shared libraries: .*
Symbols already loaded for /lib64/libc.so.6
(gdb) PASS: gdb.base/print-symbol-loading.exp: shlib brief: load shared-lib

Fixing case #2 ended up being easier than #1.  #1 had been using
gdb_test_no_output to correctly match this no-output case.  I
ended up replacing it with gdb_test_multiple, matching the exact
expected output for each of the two now acceptable cases.

For case #2, I simply added an optional non-capturing group
for the potential new output.

gdb/testsuite/ChangeLog:

	* gdb.base/print-symbol-loading.exp (proc test_load_shlib):
	Allow "Symbols already loaded for..." messages.
---
 gdb/testsuite/gdb.base/print-symbol-loading.exp | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.base/print-symbol-loading.exp b/gdb/testsuite/gdb.base/print-symbol-loading.exp
index b8eb1c844bd..6e176de351e 100644
--- a/gdb/testsuite/gdb.base/print-symbol-loading.exp
+++ b/gdb/testsuite/gdb.base/print-symbol-loading.exp
@@ -96,6 +96,7 @@ test_load_core full
 
 proc test_load_shlib { print_symbol_loading } {
     global binfile
+    global gdb_prompt
     with_test_prefix "shlib ${print_symbol_loading}" {
 	clean_restart ${binfile}
 	gdb_test_no_output "set auto-solib-add off"
@@ -106,12 +107,20 @@ proc test_load_shlib { print_symbol_loading } {
 	set test_name "load shared-lib"
 	switch ${print_symbol_loading} {
 	    "off" {
-		gdb_test_no_output "sharedlibrary .*" \
-		    ${test_name}
+		set cmd "sharedlibrary .*"
+		set cmd_regex [string_to_regexp $cmd]
+		gdb_test_multiple $cmd $test_name {
+        	    -re "^$cmd_regex\r\n$gdb_prompt $" {
+			pass $test_name
+		    }
+        	    -re "^$cmd_regex\r\nSymbols already loaded for.*?\\/libc\\..*?\r\n$gdb_prompt $" {
+			pass $test_name
+		    }
+		}
 	    }
 	    "brief" {
 		gdb_test "sharedlibrary .*" \
-		    "Loading symbols for shared libraries: \\.\\*" \
+		    "Loading symbols for shared libraries: \\.\\*.*?(?:Symbols already loaded for .*?libc)?" \
 		    ${test_name}
 	    }
 	    "full" {
-- 
2.31.1

[PATCH 4/4] mi-sym-info.exp: Increase timeout for 114-symbol-info-functions

[Kevin Buettner]

Loading libc.so's symbols increased the amount of time needed for
114-symbol-info-function to fetch symbols, causing a timeout during my
testing.  I enclosed the entire block with a "with_timeout_factor 4",
which fixes the problem for me.  (Using 2 also fixed it for me, but it
might not be enough when running this test on slower machines.)

gdb/testsuite/ChangeLog:

	* gdb.mi/mi-sym-info.exp (114-symbol-info-function test): Increase
	timeout.
---
 gdb/testsuite/gdb.mi/mi-sym-info.exp | 56 ++++++++++++++--------------
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/gdb/testsuite/gdb.mi/mi-sym-info.exp b/gdb/testsuite/gdb.mi/mi-sym-info.exp
index 18f85182a7b..dcd6f7d9187 100644
--- a/gdb/testsuite/gdb.mi/mi-sym-info.exp
+++ b/gdb/testsuite/gdb.mi/mi-sym-info.exp
@@ -123,33 +123,35 @@ gdb_test_multiple $cmd $testname -prompt "${mi_gdb_prompt}$" {
 # (from the symbol table).  There's often so much output output from
 # this command that we overflow expect's buffers, avoid this by
 # fetching the output piece by piece.
-set testname "List all functions"
-set cmd "114-symbol-info-functions --include-nondebug"
-set state 0
-gdb_test_multiple $cmd ${testname} -prompt "${mi_gdb_prompt}$" {
-    -re "114\\^done,symbols=\{" {
-	if { $state == 0 } { set state 1 }
-	exp_continue
-    }
-    -re "debug=\\\[${symtab_re}" {
-	if { $state == 1 } { set state 2 }
-	exp_continue
-    }
-    -re ",${symtab_re}" {
-	exp_continue
-    }
-    -re "\\\],nondebug=\\\[" {
-	if { $state == 2 } { set state 3 }
-	exp_continue
-    }
-    -re "\{address=${qstr},name=${qstr}\}," {
-	exp_continue
-    }
-    -re "\{address=${qstr},name=${qstr}\}\\\]\}\r\n${mi_gdb_prompt}$" {
-	if { $state == 3 } {
-	    pass $gdb_test_name
-	} else {
-	    fail $gdb_test_name
+with_timeout_factor 4 {
+    set testname "List all functions"
+    set cmd "114-symbol-info-functions --include-nondebug"
+    set state 0
+    gdb_test_multiple $cmd ${testname} -prompt "${mi_gdb_prompt}$" {
+	-re "114\\^done,symbols=\{" {
+	    if { $state == 0 } { set state 1 }
+	    exp_continue
+	}
+	-re "debug=\\\[${symtab_re}" {
+	    if { $state == 1 } { set state 2 }
+	    exp_continue
+	}
+	-re ",${symtab_re}" {
+	    exp_continue
+	}
+	-re "\\\],nondebug=\\\[" {
+	    if { $state == 2 } { set state 3 }
+	    exp_continue
+	}
+	-re "\{address=${qstr},name=${qstr}\}," {
+	    exp_continue
+	}
+	-re "\{address=${qstr},name=${qstr}\}\\\]\}\r\n${mi_gdb_prompt}$" {
+	    if { $state == 3 } {
+		pass $gdb_test_name
+	    } else {
+		fail $gdb_test_name
+	    }
 	}
     }
 }
-- 
2.31.1

Re: [PATCH 1/4] libthread_db initialization changes related to upcoming glibc-2.34

[Simon Marchi]

> diff --git a/gdb/solib.c b/gdb/solib.c
> index 2df52118143..88b201ff6a0 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -900,12 +900,18 @@ Do you need \"set solib-search-path\" or \"set sysroot\"?"),
>  
>     Uses a fairly simplistic heuristic approach where we check
>     the file name against "/libpthread".  This can lead to false
> -   positives, but this should be good enough in practice.  */
> +   positives, but this should be good enough in practice.
> +
> +   In glibc-2.34 (and later), functions formerly residing
> +   in libpthread have been moved to libc, so "/libc." needs
> +   to be checked too.  (Matching the "." will avoid matching
> +   libraries such as libcrypt.)  */

I'd prefer that you use "As of glibc-2.34...", as you've used
elsewhere.  Saying "In glibc-2.34 and later" assumes that it will be
like this forever, which we don't know.

Otherwise, LGTM.

Simon

Re: [PATCH 2/4] testsuite/glib-2.34: Match/consume optional libthread_db related output

[Simon Marchi]

On 2021-06-10 1:26 p.m., Kevin Buettner via Gdb-patches wrote:
> When using glibc-2.34, we now see messages related to the loading of
> the thread library for non-thread programs.  E.g.  for the test case,
> gdb.base/execl-update-breakpoints.exp, we will see the following when
> starting the program:
> 
> (gdb) break -qualified main
> Breakpoint 1 at 0x100118c: file /ironwood1/sourceware-git/f34-2-glibc244_fix/bld/../../worktree-glibc244_fix/gdb/testsuite/gdb.base/execl-update-breakpoints.c, line 34.
> (gdb) run
> Starting program: [...]/execl-update-breakpoints1
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib64/libthread_db.so.1".
> 
> The two lines of output related to libthread_db are new; we didn't see
> these in the past.  This is a side effect of libc now containing the
> pthread API - we can no longer tell whether the program is
> multi-threaded by simply looking for libpthread.so.  That said, I
> think that we now want to load libthread_db anyway since it's used to
> resolve TLS variables; i.e. we need it for correctly determining the
> value of errno.
> 
> This commit adds the necessary regular expressions to match this
> (optional) additional output in the two tests which were failing
> without it.


LGTM, thanks.

Simon

Re: [PATCH 3/4] print-symbol-loading.exp: Allow libc symbols to be already loaded

[Simon Marchi]

On 2021-06-10 1:26 p.m., Kevin Buettner via Gdb-patches wrote:
> One consequence of changing libpthread_name_p() in solib.c to (also)
> match libc is that the symbols for libc will now be loaded by
> solib_add() in solib.c.  I think this is mostly harmless because
> we'll likely want these symbols to be loaded anyway, but it did cause
> two failures in gdb.base/print-symbol-loading.exp.
> 
> Specifically...
> 
> 1)
> 
> sharedlibrary .*
> (gdb) PASS: gdb.base/print-symbol-loading.exp: shlib off: load shared-lib
> 
> now looks like this:
> 
> sharedlibrary .*
> Symbols already loaded for /lib64/libc.so.6
> (gdb) PASS: gdb.base/print-symbol-loading.exp: shlib off: load shared-lib
> 
> 2)
> 
> sharedlibrary .*
> Loading symbols for shared libraries: .*
> (gdb) PASS: gdb.base/print-symbol-loading.exp: shlib brief: load shared-lib
> 
> now looks like this:
> 
> sharedlibrary .*
> Loading symbols for shared libraries: .*
> Symbols already loaded for /lib64/libc.so.6
> (gdb) PASS: gdb.base/print-symbol-loading.exp: shlib brief: load shared-lib
> 
> Fixing case #2 ended up being easier than #1.  #1 had been using
> gdb_test_no_output to correctly match this no-output case.  I
> ended up replacing it with gdb_test_multiple, matching the exact
> expected output for each of the two now acceptable cases.
> 
> For case #2, I simply added an optional non-capturing group
> for the potential new output.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/print-symbol-loading.exp (proc test_load_shlib):
> 	Allow "Symbols already loaded for..." messages.
> ---
>  gdb/testsuite/gdb.base/print-symbol-loading.exp | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/print-symbol-loading.exp b/gdb/testsuite/gdb.base/print-symbol-loading.exp
> index b8eb1c844bd..6e176de351e 100644
> --- a/gdb/testsuite/gdb.base/print-symbol-loading.exp
> +++ b/gdb/testsuite/gdb.base/print-symbol-loading.exp
> @@ -96,6 +96,7 @@ test_load_core full
>  
>  proc test_load_shlib { print_symbol_loading } {
>      global binfile
> +    global gdb_prompt
>      with_test_prefix "shlib ${print_symbol_loading}" {
>  	clean_restart ${binfile}
>  	gdb_test_no_output "set auto-solib-add off"
> @@ -106,12 +107,20 @@ proc test_load_shlib { print_symbol_loading } {
>  	set test_name "load shared-lib"
>  	switch ${print_symbol_loading} {
>  	    "off" {
> -		gdb_test_no_output "sharedlibrary .*" \
> -		    ${test_name}
> +		set cmd "sharedlibrary .*"
> +		set cmd_regex [string_to_regexp $cmd]
> +		gdb_test_multiple $cmd $test_name {
> +        	    -re "^$cmd_regex\r\n$gdb_prompt $" {
> +			pass $test_name
> +		    }
> +        	    -re "^$cmd_regex\r\nSymbols already loaded for.*?\\/libc\\..*?\r\n$gdb_prompt $" {

I'm just wondering about these regexps:

 - What does the sequence .*? mean in a regexp?
 - Doesn't having .* risk matching multiple lines, including some
   unexpected output that we wouldn't want to match?  Should we use the
   typical [^\r\n]* instead?

Simon

Re: [PATCH 3/4] print-symbol-loading.exp: Allow libc symbols to be already loaded

[Kevin Buettner]

On Fri, 11 Jun 2021 15:22:23 -0400
Simon Marchi <simon.marchi@polymtl.ca> wrote:

> On 2021-06-10 1:26 p.m., Kevin Buettner via Gdb-patches wrote:

> > ---
> >  gdb/testsuite/gdb.base/print-symbol-loading.exp | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/gdb/testsuite/gdb.base/print-symbol-loading.exp b/gdb/testsuite/gdb.base/print-symbol-loading.exp
> > index b8eb1c844bd..6e176de351e 100644
> > --- a/gdb/testsuite/gdb.base/print-symbol-loading.exp
> > +++ b/gdb/testsuite/gdb.base/print-symbol-loading.exp
> > @@ -96,6 +96,7 @@ test_load_core full
> >  
> >  proc test_load_shlib { print_symbol_loading } {
> >      global binfile
> > +    global gdb_prompt
> >      with_test_prefix "shlib ${print_symbol_loading}" {
> >  	clean_restart ${binfile}
> >  	gdb_test_no_output "set auto-solib-add off"
> > @@ -106,12 +107,20 @@ proc test_load_shlib { print_symbol_loading } {
> >  	set test_name "load shared-lib"
> >  	switch ${print_symbol_loading} {
> >  	    "off" {
> > -		gdb_test_no_output "sharedlibrary .*" \
> > -		    ${test_name}
> > +		set cmd "sharedlibrary .*"
> > +		set cmd_regex [string_to_regexp $cmd]
> > +		gdb_test_multiple $cmd $test_name {
> > +        	    -re "^$cmd_regex\r\n$gdb_prompt $" {
> > +			pass $test_name
> > +		    }
> > +        	    -re "^$cmd_regex\r\nSymbols already loaded for.*?\\/libc\\..*?\r\n$gdb_prompt $" {  
> 
> I'm just wondering about these regexps:
> 
>  - What does the sequence .*? mean in a regexp?
>  - Doesn't having .* risk matching multiple lines, including some
>    unexpected output that we wouldn't want to match?  Should we use the
>    typical [^\r\n]* instead?

The regexp ".*?" is the non-greedy variation of ".*".  I.e. whereas ".*"
matches as much as possible, ".*?" will match as little as possible.
Therefore, ".*?\r\n" will only match only as much as needed until
the next \r is found; therefore it will not match multiple lines.

Kevin

Re: [PATCH 4/4] mi-sym-info.exp: Increase timeout for 114-symbol-info-functions

[Simon Marchi]

On 2021-06-10 1:26 p.m., Kevin Buettner via Gdb-patches wrote:
> Loading libc.so's symbols increased the amount of time needed for
> 114-symbol-info-function to fetch symbols, causing a timeout during my
> testing.  I enclosed the entire block with a "with_timeout_factor 4",
> which fixes the problem for me.  (Using 2 also fixed it for me, but it
> might not be enough when running this test on slower machines.)
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.mi/mi-sym-info.exp (114-symbol-info-function test): Increase
> 	timeout.

This LGTM - as long as you can confirm that this is a case of "GDB
thinking for a long time before outputting something", and not a case of
"there's a lot of output and we are trying to chew too much in a single
match".  But it looks like the test is already written to consume little
chunks at a time, since:

    https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=2d61316c32a9fa3e14786c3312d9ca87c9298db5

I'd be curious to try it, but I don't have access to my usual Ubuntu
development machine that has debug info for libc installed (and on the
machine I'm typing this on I can't install them easily).  Anyway, it's
probably fine.

My other comment would be: there are many with_timeout_factor in that
test, I'm thinking we should just wrap the entire test in a single
`with_timeout_factor 4`.

Simon

Re: [PATCH 3/4] print-symbol-loading.exp: Allow libc symbols to be already loaded

[Kevin Buettner]

On Fri, 11 Jun 2021 12:33:33 -0700
Kevin Buettner via Gdb-patches <gdb-patches@sourceware.org> wrote:

> On Fri, 11 Jun 2021 15:22:23 -0400
> Simon Marchi <simon.marchi@polymtl.ca> wrote:
>>		    }
> > > +        	    -re "^$cmd_regex\r\nSymbols already loaded for.*?\\/libc\\..*?\r\n$gdb_prompt $" {    
> > 
> > I'm just wondering about these regexps:
> > 
> >  - What does the sequence .*? mean in a regexp?
> >  - Doesn't having .* risk matching multiple lines, including some
> >    unexpected output that we wouldn't want to match?  Should we use the
> >    typical [^\r\n]* instead?  
> 
> The regexp ".*?" is the non-greedy variation of ".*".  I.e. whereas ".*"
> matches as much as possible, ".*?" will match as little as possible.
> Therefore, ".*?\r\n" will only match only as much as needed until
> the next \r is found; therefore it will not match multiple lines.

After some IRC discussion, we both agreed that it'd be safer to use
[^\r\n]* instead.  So that line left quoted above looks like this now:

		    -re "^$cmd_regex\r\nSymbols already loaded for\[^\r\n\]*\\/libc\\.\[^\r\n\]*\r\n$gdb_prompt $" {

Kevin

Re: [PATCH 4/4] mi-sym-info.exp: Increase timeout for 114-symbol-info-functions

[Kevin Buettner]

On Fri, 11 Jun 2021 16:11:31 -0400
Simon Marchi <simon.marchi@polymtl.ca> wrote:

> On 2021-06-10 1:26 p.m., Kevin Buettner via Gdb-patches wrote:
> > Loading libc.so's symbols increased the amount of time needed for
> > 114-symbol-info-function to fetch symbols, causing a timeout during my
> > testing.  I enclosed the entire block with a "with_timeout_factor 4",
> > which fixes the problem for me.  (Using 2 also fixed it for me, but it
> > might not be enough when running this test on slower machines.)
> > 
> > gdb/testsuite/ChangeLog:
> > 
> > 	* gdb.mi/mi-sym-info.exp (114-symbol-info-function test): Increase
> > 	timeout.  
> 
> This LGTM - as long as you can confirm that this is a case of "GDB
> thinking for a long time before outputting something", 

Yes, confirmed.

> and not a case of
> "there's a lot of output and we are trying to chew too much in a single
> match".  But it looks like the test is already written to consume little
> chunks at a time, since:
> 
>     https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=2d61316c32a9fa3e14786c3312d9ca87c9298db5
> 

Agreed.

Kevin

Re: [PATCH 1/4] libthread_db initialization changes related to upcoming glibc-2.34

[Kevin Buettner]

On Fri, 11 Jun 2021 15:10:54 -0400
Simon Marchi <simon.marchi@polymtl.ca> wrote:

> > diff --git a/gdb/solib.c b/gdb/solib.c
> > index 2df52118143..88b201ff6a0 100644
> > --- a/gdb/solib.c
> > +++ b/gdb/solib.c
> > @@ -900,12 +900,18 @@ Do you need \"set solib-search-path\" or \"set sysroot\"?"),
> >  
> >     Uses a fairly simplistic heuristic approach where we check
> >     the file name against "/libpthread".  This can lead to false
> > -   positives, but this should be good enough in practice.  */
> > +   positives, but this should be good enough in practice.
> > +
> > +   In glibc-2.34 (and later), functions formerly residing
> > +   in libpthread have been moved to libc, so "/libc." needs
> > +   to be checked too.  (Matching the "." will avoid matching
> > +   libraries such as libcrypt.)  */  
> 
> I'd prefer that you use "As of glibc-2.34...", as you've used
> elsewhere.  Saying "In glibc-2.34 and later" assumes that it will be
> like this forever, which we don't know.
> 
> Otherwise, LGTM.

I've revised that comment as requested.  That portion of the comment
now looks like this:

   As of glibc-2.34, functions formerly residing in libpthread have
   been moved to libc, so "/libc." needs to be checked too.  (Matching
   the "." will avoid matching libraries such as libcrypt.) */

Kevin

Re: [PATCH 0/4] libthread_db initialization changes related to upcoming glibc-2.34

[Kevin Buettner]

After making changes requested by Simon, I've pushed this
series.

Thanks to both Carlos and Simon for the quick reviews.

Kevin

On Thu, 10 Jun 2021 10:26:37 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> This patch series contains changes necessary for GDB to be able to
> debug threaded programs using glibc-2.34.
> 
> The actual change to GDB sources are contained in the first patch; the
> remainder deal with several testing issues that I ran into.
> 
> Kevin Buettner (4):
>   libthread_db initialization changes related to upcoming glibc-2.34
>   testsuite/glib-2.34: Match/consume optional libthread_db related
>     output
>   print-symbol-loading.exp: Allow libc symbols to be already loaded
>   mi-sym-info.exp: Increase timeout for 114-symbol-info-functions
> 
>  gdb/linux-thread-db.c                         | 24 +++++++-
>  gdb/solib.c                                   | 10 +++-
>  .../gdb.base/execl-update-breakpoints.exp     |  1 +
>  .../gdb.base/fork-print-inferior-events.exp   |  3 +-
>  .../gdb.base/print-symbol-loading.exp         | 15 ++++-
>  gdb/testsuite/gdb.mi/mi-sym-info.exp          | 56 ++++++++++---------
>  6 files changed, 73 insertions(+), 36 deletions(-)
> 
> -- 
> 2.31.1
> 

Re: [PATCH 0/4] libthread_db initialization changes related to upcoming glibc-2.34

[Carlos O'Donell]

On 6/11/21 6:21 PM, Kevin Buettner wrote:
> After making changes requested by Simon, I've pushed this
> series.
> 
> Thanks to both Carlos and Simon for the quick reviews.

Thank you Kevin and Simon for prioritizing the debug experience
of GNU Toolchain developers :-)

-- 
Cheers,
Carlos.

Re: [PATCH 0/4] libthread_db initialization changes related to upcoming glibc-2.34

[Carlos O'Donell]

On 6/11/21 12:35 PM, Carlos O'Donell wrote:
>> This patch series contains changes necessary for GDB to be able to
>> debug threaded programs using glibc-2.34.
>>
>> The actual change to GDB sources are contained in the first patch; the
>> remainder deal with several testing issues that I ran into.
> 
> I have tested this in Fedora Rawhide with a new glibc, and a new gdb
> and it looks good to me. There were some additional failures which I
> do not think are related to the glibc issues, but are related to the
> new appearance of threads even when the program doesn't explicitly
> call pthread_create (because now we always load libthread_db).
> 
> For example gdb/testsuite/gdb.threads/linux-dp.exp needs some additional
> cleanup on rawhide, but it's not bad, some of the gdb_test_multiple do
> not expect thread lists.
> 
> I think this patch series goes in the right direction. It improves the
> debugging under the new glibc where libc.so.6 contains all of the
> threading functionality. As we deploy the new glibc the testsuite will
> need a little more attention.
> 
> Please consider merging this series since it unblocks upstream glibc
> testing with upstream gdb. Go GNU Toolchain :-)

Simon,

What do you think of these changes? I would really like to unblock the
upstream glibc and upstream gdb and ensure that we have a solution
in upstream gdb that works with upstream glibc.

I want to be able to recommend top-of-tree for both glibc and gdb for
testing the latest changes.

-- 
Cheers,
Carlos.

Re: [PATCH 0/4] libthread_db initialization changes related to upcoming glibc-2.34

[Carlos O'Donell]

> This patch series contains changes necessary for GDB to be able to
> debug threaded programs using glibc-2.34.
> 
> The actual change to GDB sources are contained in the first patch; the
> remainder deal with several testing issues that I ran into.

I have tested this in Fedora Rawhide with a new glibc, and a new gdb
and it looks good to me. There were some additional failures which I
do not think are related to the glibc issues, but are related to the
new appearance of threads even when the program doesn't explicitly
call pthread_create (because now we always load libthread_db).

For example gdb/testsuite/gdb.threads/linux-dp.exp needs some additional
cleanup on rawhide, but it's not bad, some of the gdb_test_multiple do
not expect thread lists.

I think this patch series goes in the right direction. It improves the
debugging under the new glibc where libc.so.6 contains all of the
threading functionality. As we deploy the new glibc the testsuite will
need a little more attention.

Please consider merging this series since it unblocks upstream glibc
testing with upstream gdb. Go GNU Toolchain :-)

> Kevin Buettner (4):
>   libthread_db initialization changes related to upcoming glibc-2.34
>   testsuite/glib-2.34: Match/consume optional libthread_db related
>     output
>   print-symbol-loading.exp: Allow libc symbols to be already loaded
>   mi-sym-info.exp: Increase timeout for 114-symbol-info-functions
> 
>  gdb/linux-thread-db.c                         | 24 +++++++-
>  gdb/solib.c                                   | 10 +++-
>  .../gdb.base/execl-update-breakpoints.exp     |  1 +
>  .../gdb.base/fork-print-inferior-events.exp   |  3 +-
>  .../gdb.base/print-symbol-loading.exp         | 15 ++++-
>  gdb/testsuite/gdb.mi/mi-sym-info.exp          | 56 ++++++++++---------
>  6 files changed, 73 insertions(+), 36 deletions(-)
> 
> -- 
> 2.31.1


-- 
Cheers,
Carlos.