public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 1/7] [gdb/testsuite] Factor out proc get_portnum
@ 2024-04-15 15:56 Tom de Vries
  2024-04-15 15:56 ` [PATCH 2/7] [gdb/testsuite] Make portnum a persistent global Tom de Vries
                   ` (6 more replies)
  0 siblings, 7 replies; 15+ messages in thread
From: Tom de Vries @ 2024-04-15 15:56 UTC (permalink / raw)
  To: gdb-patches

In gdbserver_start, we have some code that determines what port number to use:
...
    # Port id -- either specified in baseboard file, or managed here.
    if [target_info exists gdb,socketport] {
       set portnum [target_info gdb,socketport]
    } else {
       # Bump the port number to avoid conflicts with hung ports.
       incr portnum
    }
...

Factor this out into a new proc get_portnum.

Tested on aarch64-linux.
---
 gdb/testsuite/lib/gdbserver-support.exp | 40 ++++++++++++++++++-------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
index 727a66e2ab1..bf000119db6 100644
--- a/gdb/testsuite/lib/gdbserver-support.exp
+++ b/gdb/testsuite/lib/gdbserver-support.exp
@@ -129,8 +129,31 @@ proc gdb_target_cmd { args } {
     return [expr $res == 0 ? 0 : 1]
 }
 
-global portnum
-set portnum "2345"
+# Return a usable port number.
+
+proc get_portnum {} {
+    if { [target_info exists gdb,socketport] } {
+	# Hard-coded in target board.
+	return [target_info gdb,socketport]
+    }
+
+    # Not hard-coded in target board.  Return increasing port numbers,
+    # starting at $initial_portnum, to avoid conflicts with hung ports.
+    set initial_portnum 2345
+
+    # Currently available port number.
+    global portnum
+
+    # Initialize, if necessary.
+    if { ![info exists portnum] } {
+	set portnum $initial_portnum
+    }
+
+    # Return currently available port number, and update it.
+    set res $portnum
+    incr portnum
+    return $res
+}
 
 # Locate the gdbserver binary.  Returns "" if gdbserver could not be found.
 
@@ -247,16 +270,10 @@ proc gdbserver_default_get_comm_port { port } {
 # Returns the target protocol and socket to connect to.
 
 proc gdbserver_start { options arguments } {
-    global portnum
     global GDB_TEST_SOCKETHOST
 
     # Port id -- either specified in baseboard file, or managed here.
-    if [target_info exists gdb,socketport] {
-	set portnum [target_info gdb,socketport]
-    } else {
-	# Bump the port number to avoid conflicts with hung ports.
-	incr portnum
-    }
+    set portnum [get_portnum]
 
     # Extract the local and remote host ids from the target board struct.
     if { [info exists GDB_TEST_SOCKETHOST] } {
@@ -372,10 +389,11 @@ proc gdbserver_start { options arguments } {
 	    -re "Listening on" { }
 	    -re "Can't (bind address|listen on socket): Address already in use\\.\r\n" {
 		verbose -log "Port $portnum is already in use."
-		if ![target_info exists gdb,socketport] {
+		set other_portnum [get_portnum]
+		if { $other_portnum != $portnum } {
 		    # Bump the port number to avoid the conflict.
 		    wait -i $expect_out(spawn_id)
-		    incr portnum
+		    set portnum $other_portnum
 		    continue
 		}
 	    }

base-commit: 9dd918142787246ea7ed53494d9cbc6b51486133
-- 
2.35.3


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/7] [gdb/testsuite] Make portnum a persistent global
  2024-04-15 15:56 [PATCH 1/7] [gdb/testsuite] Factor out proc get_portnum Tom de Vries
@ 2024-04-15 15:56 ` Tom de Vries
  2024-05-03 20:20   ` Tom Tromey
  2024-04-15 15:56 ` [PATCH 3/7] [gdb/testsuite] Factor out proc with_lock Tom de Vries
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Tom de Vries @ 2024-04-15 15:56 UTC (permalink / raw)
  To: gdb-patches

When instrumenting get_portnum using:
...
    puts "PORTNUM: $res"
...
and running:
...
$ cd build/gdb
$ make check TESTS=gdb.server/*.exp
...
we get:
...
Running gdb.server/target-exec-file.exp ...
PORTNUM: 2345
Running gdb.server/stop-reply-no-thread-multi.exp ...
PORTNUM: 2345
PORTNUM: 2346
PORTNUM: 2347
PORTNUM: 2348
PORTNUM: 2349
PORTNUM: 2350
...

So, while get_portnum does return increasing numbers in a single test-case, it
restarts at each test-case.

This is a regression since the introduction of persistent globals.

Fix this by using "gdb_persistent_global portnum", such that we get:
...
Running gdb.server/target-exec-file.exp ...
PORTNUM: 2345
Running gdb.server/stop-reply-no-thread-multi.exp ...
PORTNUM: 2346
PORTNUM: 2347
PORTNUM: 2348
PORTNUM: 2349
PORTNUM: 2350
PORTNUM: 2351
...

Tested on aarch64-linux.
---
 gdb/testsuite/lib/gdbserver-support.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
index bf000119db6..0f97ce9c0fd 100644
--- a/gdb/testsuite/lib/gdbserver-support.exp
+++ b/gdb/testsuite/lib/gdbserver-support.exp
@@ -142,7 +142,7 @@ proc get_portnum {} {
     set initial_portnum 2345
 
     # Currently available port number.
-    global portnum
+    gdb_persistent_global portnum
 
     # Initialize, if necessary.
     if { ![info exists portnum] } {
-- 
2.35.3


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 3/7] [gdb/testsuite] Factor out proc with_lock
  2024-04-15 15:56 [PATCH 1/7] [gdb/testsuite] Factor out proc get_portnum Tom de Vries
  2024-04-15 15:56 ` [PATCH 2/7] [gdb/testsuite] Make portnum a persistent global Tom de Vries
@ 2024-04-15 15:56 ` Tom de Vries
  2024-05-03 20:22   ` Tom Tromey
  2024-04-15 15:56 ` [PATCH 4/7] [gdb/testsuite] Factor out proc lock_dir Tom de Vries
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Tom de Vries @ 2024-04-15 15:56 UTC (permalink / raw)
  To: gdb-patches

Factor out proc with_lock from with_rocm_gpu_lock, and move required procs
lock_file_acquire and lock_file_release to lib/gdb-utils.exp.

Tested on aarch64-linux.
---
 gdb/testsuite/lib/gdb-utils.exp | 59 +++++++++++++++++++++++++++++++++
 gdb/testsuite/lib/rocm.exp      | 55 +-----------------------------
 2 files changed, 60 insertions(+), 54 deletions(-)

diff --git a/gdb/testsuite/lib/gdb-utils.exp b/gdb/testsuite/lib/gdb-utils.exp
index 34752081b60..4205f8d1a22 100644
--- a/gdb/testsuite/lib/gdb-utils.exp
+++ b/gdb/testsuite/lib/gdb-utils.exp
@@ -138,3 +138,62 @@ proc version_compare { l1 op l2 } {
     }
     return 1
 }
+
+# Acquire lock file LOCKFILE.  Tries forever until the lock file is
+# successfully created.
+
+proc lock_file_acquire {lockfile} {
+    verbose -log "acquiring lock file: $::subdir/${::gdb_test_file_name}.exp"
+    while {true} {
+	if {![catch {open $lockfile {WRONLY CREAT EXCL}} rc]} {
+	    set msg "locked by $::subdir/${::gdb_test_file_name}.exp"
+	    verbose -log "lock file: $msg"
+	    # For debugging, put info in the lockfile about who owns
+	    # it.
+	    puts  $rc $msg
+	    flush $rc
+	    return [list $rc $lockfile]
+	}
+	after 10
+    }
+}
+
+# Release a lock file.
+
+proc lock_file_release {info} {
+    verbose -log "releasing lock file: $::subdir/${::gdb_test_file_name}.exp"
+
+    if {![catch {fconfigure [lindex $info 0]}]} {
+	if {![catch {
+	    close [lindex $info 0]
+	    file delete -force [lindex $info 1]
+	} rc]} {
+	    return ""
+	} else {
+	    return -code error "Error releasing lockfile: '$rc'"
+	}
+    } else {
+	error "invalid lock"
+    }
+}
+
+# Run body under lock LOCK_FILE.
+
+proc with_lock { lock_file body } {
+    if {[info exists ::GDB_PARALLEL]} {
+	set lock_rc [lock_file_acquire $lock_file]
+    }
+
+    set code [catch {uplevel 1 $body} result]
+
+    if {[info exists ::GDB_PARALLEL]} {
+	lock_file_release $lock_rc
+    }
+
+    if {$code == 1} {
+	global errorInfo errorCode
+	return -code $code -errorinfo $errorInfo -errorcode $errorCode $result
+    } else {
+	return -code $code $result
+    }
+}
diff --git a/gdb/testsuite/lib/rocm.exp b/gdb/testsuite/lib/rocm.exp
index ab21db6685c..7dd7ef3f3b5 100644
--- a/gdb/testsuite/lib/rocm.exp
+++ b/gdb/testsuite/lib/rocm.exp
@@ -108,68 +108,15 @@ gdb_caching_proc allow_hipcc_tests {} {
 # at a time.
 set gpu_lock_filename $objdir/gpu-parallel.lock
 
-# Acquire lock file LOCKFILE.  Tries forever until the lock file is
-# successfully created.
-
-proc lock_file_acquire {lockfile} {
-    verbose -log "acquiring lock file: $::subdir/${::gdb_test_file_name}.exp"
-    while {true} {
-	if {![catch {open $lockfile {WRONLY CREAT EXCL}} rc]} {
-	    set msg "locked by $::subdir/${::gdb_test_file_name}.exp"
-	    verbose -log "lock file: $msg"
-	    # For debugging, put info in the lockfile about who owns
-	    # it.
-	    puts  $rc $msg
-	    flush $rc
-	    return [list $rc $lockfile]
-	}
-	after 10
-    }
-}
-
-# Release a lock file.
-
-proc lock_file_release {info} {
-    verbose -log "releasing lock file: $::subdir/${::gdb_test_file_name}.exp"
-
-    if {![catch {fconfigure [lindex $info 0]}]} {
-	if {![catch {
-	    close [lindex $info 0]
-	    file delete -force [lindex $info 1]
-	} rc]} {
-	    return ""
-	} else {
-	    return -code error "Error releasing lockfile: '$rc'"
-	}
-    } else {
-	error "invalid lock"
-    }
-}
-
 # Run body under the GPU lock.  Also calls gdb_exit before releasing
 # the GPU lock.
 
 proc with_rocm_gpu_lock { body } {
-    if {[info exists ::GDB_PARALLEL]} {
-	set lock_rc [lock_file_acquire $::gpu_lock_filename]
-    }
-
-    set code [catch {uplevel 1 $body} result]
+    with_lock $::gpu_lock_filename $body
 
     # In case BODY returned early due to some testcase failing, and
     # left GDB running, debugging the GPU.
     gdb_exit
-
-    if {[info exists ::GDB_PARALLEL]} {
-	lock_file_release $lock_rc
-    }
-
-    if {$code == 1} {
-	global errorInfo errorCode
-	return -code $code -errorinfo $errorInfo -errorcode $errorCode $result
-    } else {
-	return -code $code $result
-    }
 }
 
 # Return true if all the devices support debugging multiple processes
-- 
2.35.3


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 4/7] [gdb/testsuite] Factor out proc lock_dir
  2024-04-15 15:56 [PATCH 1/7] [gdb/testsuite] Factor out proc get_portnum Tom de Vries
  2024-04-15 15:56 ` [PATCH 2/7] [gdb/testsuite] Make portnum a persistent global Tom de Vries
  2024-04-15 15:56 ` [PATCH 3/7] [gdb/testsuite] Factor out proc with_lock Tom de Vries
@ 2024-04-15 15:56 ` Tom de Vries
  2024-05-03 20:23   ` Tom Tromey
  2024-04-15 15:56 ` [PATCH 5/7] [gdb/testsuite] Move gpu-parallel.lock to cache dir Tom de Vries
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Tom de Vries @ 2024-04-15 15:56 UTC (permalink / raw)
  To: gdb-patches

In lib/rocm.exp we have:
...
set gpu_lock_filename $objdir/gpu-parallel.lock
...

This decides both the lock file name and directory.

Factor out a new proc lock_dir that decides on the directory, leaving just:
...
set gpu_lock_filename gpu-parallel.lock
...

Tested on aarch64-linux.
---
 gdb/testsuite/lib/gdb-utils.exp | 7 +++++++
 gdb/testsuite/lib/rocm.exp      | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/lib/gdb-utils.exp b/gdb/testsuite/lib/gdb-utils.exp
index 4205f8d1a22..c0b96d3c2bf 100644
--- a/gdb/testsuite/lib/gdb-utils.exp
+++ b/gdb/testsuite/lib/gdb-utils.exp
@@ -177,10 +177,17 @@ proc lock_file_release {info} {
     }
 }
 
+# Return directory where we keep lock files.
+
+proc lock_dir {} {
+    return $objdir
+}
+
 # Run body under lock LOCK_FILE.
 
 proc with_lock { lock_file body } {
     if {[info exists ::GDB_PARALLEL]} {
+	set lock_file [file join [lock_dir] $lock_file]
 	set lock_rc [lock_file_acquire $lock_file]
     }
 
diff --git a/gdb/testsuite/lib/rocm.exp b/gdb/testsuite/lib/rocm.exp
index 7dd7ef3f3b5..2276bb3640e 100644
--- a/gdb/testsuite/lib/rocm.exp
+++ b/gdb/testsuite/lib/rocm.exp
@@ -106,7 +106,7 @@ gdb_caching_proc allow_hipcc_tests {} {
 
 # The lock file used to ensure that only one GDB has access to the GPU
 # at a time.
-set gpu_lock_filename $objdir/gpu-parallel.lock
+set gpu_lock_filename gpu-parallel.lock
 
 # Run body under the GPU lock.  Also calls gdb_exit before releasing
 # the GPU lock.
-- 
2.35.3


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 5/7] [gdb/testsuite] Move gpu-parallel.lock to cache dir
  2024-04-15 15:56 [PATCH 1/7] [gdb/testsuite] Factor out proc get_portnum Tom de Vries
                   ` (2 preceding siblings ...)
  2024-04-15 15:56 ` [PATCH 4/7] [gdb/testsuite] Factor out proc lock_dir Tom de Vries
@ 2024-04-15 15:56 ` Tom de Vries
  2024-05-03 20:25   ` Tom Tromey
  2024-04-15 15:56 ` [PATCH 6/7] [gdb/testsuite] Use unique portnum in parallel testing Tom de Vries
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Tom de Vries @ 2024-04-15 15:56 UTC (permalink / raw)
  To: gdb-patches

The lock directory returned by lock_dir is currently $objdir.

It seems possible to leave a stale lock file that blocks progress in a
following run.

Fix this by using a directory that is guaranteed to be initially empty when
using GDB_PARALLEL, like temp or cache.

In gdb/testsuite/README I found:
...
cache in particular is used to share data across invocations of runtest
...
which seems appropriate, so let's use cache for this.

Tested on aarch64-linux.
---
 gdb/testsuite/lib/gdb-utils.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/lib/gdb-utils.exp b/gdb/testsuite/lib/gdb-utils.exp
index c0b96d3c2bf..1f30d807ff0 100644
--- a/gdb/testsuite/lib/gdb-utils.exp
+++ b/gdb/testsuite/lib/gdb-utils.exp
@@ -180,7 +180,7 @@ proc lock_file_release {info} {
 # Return directory where we keep lock files.
 
 proc lock_dir {} {
-    return $objdir
+    return [make_gdb_parallel_path cache]
 }
 
 # Run body under lock LOCK_FILE.
-- 
2.35.3


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 6/7] [gdb/testsuite] Use unique portnum in parallel testing
  2024-04-15 15:56 [PATCH 1/7] [gdb/testsuite] Factor out proc get_portnum Tom de Vries
                   ` (3 preceding siblings ...)
  2024-04-15 15:56 ` [PATCH 5/7] [gdb/testsuite] Move gpu-parallel.lock to cache dir Tom de Vries
@ 2024-04-15 15:56 ` Tom de Vries
  2024-05-03 20:27   ` Tom Tromey
  2024-04-15 15:56 ` [PATCH 7/7] [gdb/testsuite] Use unique portnum in parallel testing (check//% case) Tom de Vries
  2024-05-03 20:19 ` [PATCH 1/7] [gdb/testsuite] Factor out proc get_portnum Tom Tromey
  6 siblings, 1 reply; 15+ messages in thread
From: Tom de Vries @ 2024-04-15 15:56 UTC (permalink / raw)
  To: gdb-patches

When instrumenting get_portnum using:
...
puts "PORTNUM: $res"
...
and running:
...
$ cd build/gdb
$ make check-parallel -j2 TESTS=gdb.server/*.exp
...
we run into:
...
Running gdb.server/abspath.exp ...
PORTNUM: 2345
...
and:
...
Running gdb.server/bkpt-other-inferior.exp ...
PORTNUM: 2345
...

This is because the test-cases are run in independent runtest invocations.

Fix this by handling the parallel case in get_portnum using:
- a file $objdir/cache/portnum to keep the portnum variable, and
- a file $objdir/cache/portnum.lock to serialize access to it.

Tested on aarch64-linux.
---
 gdb/testsuite/lib/gdbserver-support.exp | 47 ++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 9 deletions(-)

diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
index 0f97ce9c0fd..41ad5e6cbfb 100644
--- a/gdb/testsuite/lib/gdbserver-support.exp
+++ b/gdb/testsuite/lib/gdbserver-support.exp
@@ -141,18 +141,47 @@ proc get_portnum {} {
     # starting at $initial_portnum, to avoid conflicts with hung ports.
     set initial_portnum 2345
 
-    # Currently available port number.
-    gdb_persistent_global portnum
+    if { ![info exists ::GDB_PARALLEL] } {
+	# Sequential case.
 
-    # Initialize, if necessary.
-    if { ![info exists portnum] } {
-	set portnum $initial_portnum
+	# Currently available port number.
+	gdb_persistent_global portnum
+
+	# Initialize, if necessary.
+	if { ![info exists portnum] } {
+	    set portnum $initial_portnum
+	}
+
+	# Return currently available port number, and update it.
+	set res $portnum
+	incr portnum
+	return $res
+    }
+
+    # Parallel case.
+    with_lock portnum.lock {
+	# Keep portnum file alongside the lock that guards it.
+	set portnum_file [lock_dir]/portnum
+
+	if { [file exists $portnum_file] } {
+	    set fd [open $portnum_file r]
+	    set portnum [read $fd]
+	    close $fd
+
+	    set portnum [string trim $portnum]
+	} else {
+	    # Initialize.
+	    set portnum $initial_portnum
+	}
+
+	set next_portnum [expr $portnum + 1]
+
+	set fd [open $portnum_file w]
+	puts $fd $next_portnum
+	close $fd
     }
 
-    # Return currently available port number, and update it.
-    set res $portnum
-    incr portnum
-    return $res
+    return $portnum
 }
 
 # Locate the gdbserver binary.  Returns "" if gdbserver could not be found.
-- 
2.35.3


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 7/7] [gdb/testsuite] Use unique portnum in parallel testing (check//% case)
  2024-04-15 15:56 [PATCH 1/7] [gdb/testsuite] Factor out proc get_portnum Tom de Vries
                   ` (4 preceding siblings ...)
  2024-04-15 15:56 ` [PATCH 6/7] [gdb/testsuite] Use unique portnum in parallel testing Tom de Vries
@ 2024-04-15 15:56 ` Tom de Vries
  2024-05-03 20:30   ` Tom Tromey
  2024-05-03 20:19 ` [PATCH 1/7] [gdb/testsuite] Factor out proc get_portnum Tom Tromey
  6 siblings, 1 reply; 15+ messages in thread
From: Tom de Vries @ 2024-04-15 15:56 UTC (permalink / raw)
  To: gdb-patches

Make target check//% is the gdb variant of a similar gcc make target [1].

When running tests using check//%:
...
$ cd build/gdb
$ make check//unix/{-fPIE/-pie,-fno-PIE/-no-pie} -j2 TESTS=gdb.server/*.exp
...
we get:
...
$ cat build/gdb/testsuite.unix.-fPIE.-pie/cache/portnum
2427
$ cat build/gdb/testsuite.unix.-fno-PIE.-no-pie/cache/portnum
2423
...

The problem is that there are two portnum files used in parallel.

Fix this by:
- creating a common lockdir build/gdb/testsuite.lockdir for make target
  check//%,
- passing this down to the runtests invocations using variable GDB_LOCK_DIR,
  and
- using GDB_LOCK_DIR in lock_dir.

Tested on aarch64-linux.

PR testsuite/31632
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31632

[1] https://gcc.gnu.org/install/test.html
---
 gdb/Makefile.in                 | 8 ++++++--
 gdb/testsuite/lib/gdb-utils.exp | 5 +++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 23894ea4a4d..5a63f166107 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -2002,6 +2002,10 @@ check-all-boards: force
 	  $(MAKE) $(TARGET_FLAGS_TO_PASS) check-all-boards; \
 	else true; fi
 
+testsuite.lockdir: force
+	rm -rf $@
+	mkdir -p $@
+
 # The idea is to parallelize testing of multilibs, for example:
 #   make -j3 check//sh-hms-sim/{-m1,-m2,-m3,-m3e,-m4}/{,-nofpu}
 # will run 3 concurrent sessions of check, eventually testing all 10
@@ -2010,7 +2014,7 @@ check-all-boards: force
 # used, this rule will harmlessly fail to match.  Used FORCE_PARALLEL to
 # prevent serialized checking due to the passed RUNTESTFLAGS.
 # FIXME: use config.status --config not --version, when available.
-check//%: force
+check//%: force testsuite.lockdir
 	@if [ -f testsuite/config.status ]; then \
 	  rootme=`pwd`; export rootme; \
 	  rootsrc=`cd $(srcdir); pwd`; export rootsrc; \
@@ -2028,7 +2032,7 @@ check//%: force
 	     ); \
 	  else :; fi && cd $$testdir && \
 	  $(MAKE) $(TARGET_FLAGS_TO_PASS) \
-	    RUNTESTFLAGS="--target_board=$$variant $(RUNTESTFLAGS)" \
+	    RUNTESTFLAGS="GDB_LOCK_DIR=$$rootme/testsuite.lockdir --target_board=$$variant $(RUNTESTFLAGS)" \
 	    FORCE_PARALLEL=$(if $(FORCE_PARALLEL),1,$(if $(RUNTESTFLAGS),,1)) \
 	    "$$target"; \
 	else true; fi
diff --git a/gdb/testsuite/lib/gdb-utils.exp b/gdb/testsuite/lib/gdb-utils.exp
index 1f30d807ff0..95c53d030d8 100644
--- a/gdb/testsuite/lib/gdb-utils.exp
+++ b/gdb/testsuite/lib/gdb-utils.exp
@@ -180,6 +180,11 @@ proc lock_file_release {info} {
 # Return directory where we keep lock files.
 
 proc lock_dir {} {
+    if { [info exists ::GDB_LOCK_DIR] } {
+	# When using check//.
+	return $::GDB_LOCK_DIR
+    }
+
     return [make_gdb_parallel_path cache]
 }
 
-- 
2.35.3


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/7] [gdb/testsuite] Factor out proc get_portnum
  2024-04-15 15:56 [PATCH 1/7] [gdb/testsuite] Factor out proc get_portnum Tom de Vries
                   ` (5 preceding siblings ...)
  2024-04-15 15:56 ` [PATCH 7/7] [gdb/testsuite] Use unique portnum in parallel testing (check//% case) Tom de Vries
@ 2024-05-03 20:19 ` Tom Tromey
  6 siblings, 0 replies; 15+ messages in thread
From: Tom Tromey @ 2024-05-03 20:19 UTC (permalink / raw)
  To: Tom de Vries; +Cc: gdb-patches

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> In gdbserver_start, we have some code that determines what port number to use:
Tom> ...
Tom>     # Port id -- either specified in baseboard file, or managed here.
Tom>     if [target_info exists gdb,socketport] {
Tom>        set portnum [target_info gdb,socketport]
Tom>     } else {
Tom>        # Bump the port number to avoid conflicts with hung ports.
Tom>        incr portnum
Tom>     }
Tom> ...

Tom> Factor this out into a new proc get_portnum.

Tom> Tested on aarch64-linux.

Looks good.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/7] [gdb/testsuite] Make portnum a persistent global
  2024-04-15 15:56 ` [PATCH 2/7] [gdb/testsuite] Make portnum a persistent global Tom de Vries
@ 2024-05-03 20:20   ` Tom Tromey
  0 siblings, 0 replies; 15+ messages in thread
From: Tom Tromey @ 2024-05-03 20:20 UTC (permalink / raw)
  To: Tom de Vries; +Cc: gdb-patches

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> This is a regression since the introduction of persistent globals.

Tom> Fix this by using "gdb_persistent_global portnum", such that we get:

Ok.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/7] [gdb/testsuite] Factor out proc with_lock
  2024-04-15 15:56 ` [PATCH 3/7] [gdb/testsuite] Factor out proc with_lock Tom de Vries
@ 2024-05-03 20:22   ` Tom Tromey
  0 siblings, 0 replies; 15+ messages in thread
From: Tom Tromey @ 2024-05-03 20:22 UTC (permalink / raw)
  To: Tom de Vries; +Cc: gdb-patches

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Factor out proc with_lock from with_rocm_gpu_lock, and move required procs
Tom> lock_file_acquire and lock_file_release to lib/gdb-utils.exp.

Looks good.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/7] [gdb/testsuite] Factor out proc lock_dir
  2024-04-15 15:56 ` [PATCH 4/7] [gdb/testsuite] Factor out proc lock_dir Tom de Vries
@ 2024-05-03 20:23   ` Tom Tromey
  0 siblings, 0 replies; 15+ messages in thread
From: Tom Tromey @ 2024-05-03 20:23 UTC (permalink / raw)
  To: Tom de Vries; +Cc: gdb-patches

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> In lib/rocm.exp we have:
Tom> ...
Tom> set gpu_lock_filename $objdir/gpu-parallel.lock
Tom> ...

Tom> This decides both the lock file name and directory.

Tom> Factor out a new proc lock_dir that decides on the directory, leaving just:
Tom> ...
Tom> set gpu_lock_filename gpu-parallel.lock
Tom> ...

Ok
Approved-By: Tom Tromey <tom@tromey.com>

Tom

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/7] [gdb/testsuite] Move gpu-parallel.lock to cache dir
  2024-04-15 15:56 ` [PATCH 5/7] [gdb/testsuite] Move gpu-parallel.lock to cache dir Tom de Vries
@ 2024-05-03 20:25   ` Tom Tromey
  0 siblings, 0 replies; 15+ messages in thread
From: Tom Tromey @ 2024-05-03 20:25 UTC (permalink / raw)
  To: Tom de Vries; +Cc: gdb-patches

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> The lock directory returned by lock_dir is currently $objdir.
Tom> It seems possible to leave a stale lock file that blocks progress in a
Tom> following run.

Yeah.  Normally I think the idea is that 'make check' first deletes
things that might cause problems.

Tom> Fix this by using a directory that is guaranteed to be initially empty when
Tom> using GDB_PARALLEL, like temp or cache.

Makes sense to me.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 6/7] [gdb/testsuite] Use unique portnum in parallel testing
  2024-04-15 15:56 ` [PATCH 6/7] [gdb/testsuite] Use unique portnum in parallel testing Tom de Vries
@ 2024-05-03 20:27   ` Tom Tromey
  0 siblings, 0 replies; 15+ messages in thread
From: Tom Tromey @ 2024-05-03 20:27 UTC (permalink / raw)
  To: Tom de Vries; +Cc: gdb-patches

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> This is because the test-cases are run in independent runtest invocations.

Tom> Fix this by handling the parallel case in get_portnum using:
Tom> - a file $objdir/cache/portnum to keep the portnum variable, and
Tom> - a file $objdir/cache/portnum.lock to serialize access to it.

Thank you.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 7/7] [gdb/testsuite] Use unique portnum in parallel testing (check//% case)
  2024-04-15 15:56 ` [PATCH 7/7] [gdb/testsuite] Use unique portnum in parallel testing (check//% case) Tom de Vries
@ 2024-05-03 20:30   ` Tom Tromey
  2024-05-04  7:48     ` Tom de Vries
  0 siblings, 1 reply; 15+ messages in thread
From: Tom Tromey @ 2024-05-03 20:30 UTC (permalink / raw)
  To: Tom de Vries; +Cc: gdb-patches

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Fix this by:
Tom> - creating a common lockdir build/gdb/testsuite.lockdir for make target
Tom>   check//%,
Tom> - passing this down to the runtests invocations using variable GDB_LOCK_DIR,
Tom>   and
Tom> - using GDB_LOCK_DIR in lock_dir.

I think I've never once used check//.
I'm not even sure I was aware of its existence.

Tom>  proc lock_dir {} {
Tom> +    if { [info exists ::GDB_LOCK_DIR] } {
Tom> +	# When using check//.
Tom> +	return $::GDB_LOCK_DIR
Tom> +    }
Tom> +
Tom>      return [make_gdb_parallel_path cache]
Tom>  }

I guess this approach of not always requiring GDB_LOCK_DIR is handy when
running runtest by hand... I was thinking it might be better to unify
all the scenarios here, but really this doesn't seem so complicated.

Approved-By: Tom Tromey <tom@tromey.com>


Tom

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 7/7] [gdb/testsuite] Use unique portnum in parallel testing (check//% case)
  2024-05-03 20:30   ` Tom Tromey
@ 2024-05-04  7:48     ` Tom de Vries
  0 siblings, 0 replies; 15+ messages in thread
From: Tom de Vries @ 2024-05-04  7:48 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

On 5/3/24 22:30, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Fix this by:
> Tom> - creating a common lockdir build/gdb/testsuite.lockdir for make target
> Tom>   check//%,
> Tom> - passing this down to the runtests invocations using variable GDB_LOCK_DIR,
> Tom>   and
> Tom> - using GDB_LOCK_DIR in lock_dir.
> 
> I think I've never once used check//.
> I'm not even sure I was aware of its existence.
> 

I know it from the suse / opensuse gdb package ( 
https://build.opensuse.org/projects/devel:gcc/packages/gdb/files/gdb.spec?expand=1 
), which I think inherited it from the fedora gdb package.

> Tom>  proc lock_dir {} {
> Tom> +    if { [info exists ::GDB_LOCK_DIR] } {
> Tom> +	# When using check//.
> Tom> +	return $::GDB_LOCK_DIR
> Tom> +    }
> Tom> +
> Tom>      return [make_gdb_parallel_path cache]
> Tom>  }
> 
> I guess this approach of not always requiring GDB_LOCK_DIR is handy when
> running runtest by hand... I was thinking it might be better to unify
> all the scenarios here, but really this doesn't seem so complicated.
> 
> Approved-By: Tom Tromey <tom@tromey.com>
> 

Thanks for the review, I'll commit after a round of retesting.

Thanks,
- Tom


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2024-05-04  7:48 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-15 15:56 [PATCH 1/7] [gdb/testsuite] Factor out proc get_portnum Tom de Vries
2024-04-15 15:56 ` [PATCH 2/7] [gdb/testsuite] Make portnum a persistent global Tom de Vries
2024-05-03 20:20   ` Tom Tromey
2024-04-15 15:56 ` [PATCH 3/7] [gdb/testsuite] Factor out proc with_lock Tom de Vries
2024-05-03 20:22   ` Tom Tromey
2024-04-15 15:56 ` [PATCH 4/7] [gdb/testsuite] Factor out proc lock_dir Tom de Vries
2024-05-03 20:23   ` Tom Tromey
2024-04-15 15:56 ` [PATCH 5/7] [gdb/testsuite] Move gpu-parallel.lock to cache dir Tom de Vries
2024-05-03 20:25   ` Tom Tromey
2024-04-15 15:56 ` [PATCH 6/7] [gdb/testsuite] Use unique portnum in parallel testing Tom de Vries
2024-05-03 20:27   ` Tom Tromey
2024-04-15 15:56 ` [PATCH 7/7] [gdb/testsuite] Use unique portnum in parallel testing (check//% case) Tom de Vries
2024-05-03 20:30   ` Tom Tromey
2024-05-04  7:48     ` Tom de Vries
2024-05-03 20:19 ` [PATCH 1/7] [gdb/testsuite] Factor out proc get_portnum Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).