This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA] Add FORCE_LOCAL_GDB_QUIT_WAIT testsuite parameter.


On Mon, 2018-12-03 at 12:39 +0000, Pedro Alves wrote:
> On 12/02/2018 11:46 PM, Philippe Waroquiers wrote:
> > This patch helps to run GDB testsuite under valgrind, by ensuring
> > that the valgrind output completely goes into the gdb.log file
> > (approach suggested by Pedro on irc).
> 
> I'd first prefer to see about making this unconditional.
> Does the explicit "quit" slow down native non-valgrind testing
> noticeably, for example?  Always doing the "quit" would help
> catch bugs around GDB crashing on termination -- IIRC, I've fixed
> at least a bug like that that I only noticed happened because
> I found some rogue gdb core dumps in the testsuite dir.
Find attached the current state of implementing a 'clean quit' of GDB
at the end of a test.

As it is now, it is not suitable to be activated unconditionally;
but maybe the patch can be further improved.
The difference in timing is significant:
  time make check RUNTESTFLAGS="GDB=$PWD/gdb" FORCE_PARALLEL="1" -j1
  real	24m59.404s
  user	14m1.620s
  sys	2m30.536s
  time make check RUNTESTFLAGS="GDB=$PWD/gdb FORCE_LOCAL_GDB_QUIT_WAIT=1" FORCE_PARALLEL="1" -j1
  real	47m3.698s
  user	10m39.088s
  sys	1m49.892s

A few observations:
  * unclear why, but the FORCE uses less user and less sys cpu.
  * a very significant part of the real time diff is because
    the FORCE QUIT WAIT encounters times out for some tests,
    instead of doing a clean quit.
    I have not found a proper way to have both a clean quit
    without timeout, and not lose the valgrind output.
    The tests that have some timeouts are:
      gdb.base/dprintf-detach.exp
      gdb.base/jit.exp
      gdb.base/pie-fork.exp
      gdb.base/watchpoint-hw-attach.exp
      gdb.mi/mi-break.exp
      gdb.mi/mi-exec-run.exp
      gdb.mi/mi-watch.exp
      gdb.mi/new-ui-mi-sync.exp
      gdb.mi/user-selected-context-sync.exp
      gdb.server/abspath.exp
      gdb.server/connect-stopped-target.exp
      gdb.server/connect-without-multi-process.exp
      gdb.server/file-transfer.exp
      gdb.server/no-thread-db.exp
      gdb.server/run-without-local-binary.exp
      gdb.server/server-exec-info.exp
      gdb.server/server-mon.exp
      gdb.server/server-run.exp
      gdb.server/solib-list.exp
      gdb.server/stop-reply-no-thread.exp
      gdb.server/wrapper.exp
      gdb.threads/process-dies-while-handling-bp.exp
   The worst test is dprintf-detach, which adds 6 minutes of timeout.
  * even with the below patch, the valgrind output is incomplete
    for a few tests.  As far as I can see, the lost output is
    mostly due to time outs.

I have to admit that I am a little bit lost in the mysteries
of dejagnu/expect.
Whatever I am trying to change in the 'clean quit' logic,
it either increases the nr of timeouts, or loses some
valgrind output, or causes 'channel exp9 not open' errors.

So, some advice about how to improve the logic (for both
the normal and the mi 'clean quit' logic) would be welcome.

Thanks
Philippe.



 
From faad8a448467ac2b74dee6740d5d0d7540d6b264 Mon Sep 17 00:00:00 2001
From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
Date: Sun, 2 Dec 2018 16:18:37 +0100
Subject: [PATCH 1/2] Add FORCE_LOCAL_GDB_QUIT_WAIT testsuite parameter.

This patch helps to run GDB testsuite under valgrind, by ensuring
that the valgrind output completely goes into the gdb.log file
(approach suggested by Pedro on irc).

By default, dejagnu terminates a local GDB by closing the pty
that GDB uses. GDB then exits directly.

However, when GDB is run in a wrapper/tool (such as valgrind) that
does more output after the end of the test, this way of killing GDB
means the wrapper/tool output is dropped, as the pty on which the
wrapper/tool writes its output will be closed before the tool
has finished producing its results.

testsuite/ChangeLog

2018-12-27  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* lib/gdb.exp (default_gdb_exit): If FORCE_LOCAL_GDB_QUIT_WAIT
	is set, terminates GDB by sending an interrupt (in case GDB
	not yet expecting a quit command) then send the quit command,
	and wait for GDB to cause an eof.
	* README: Describe new FORCE_LOCAL_GDB_QUIT_WAIT variable.
---
 gdb/testsuite/README      | 16 +++++++++++++++
 gdb/testsuite/lib/gdb.exp | 41 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/README b/gdb/testsuite/README
index b5e75b9a79..53d9d27a71 100644
--- a/gdb/testsuite/README
+++ b/gdb/testsuite/README
@@ -293,6 +293,22 @@ can do:
 
 	make check TS=1 TS_FORMAT='[%b %H:%S]'
 
+FORCE_LOCAL_GDB_QUIT_WAIT
+
+In a local setup (GDB running locally), this variable tells dejagnu
+to terminate GDB by sending a quit command to it, and then wait for
+GDB to terminate.  This is a.o. useful when running GDB under valgrind
+to ensure that the valgrind results produced at the end of execution
+(such as the leak search) are properly saved to the gdb.log file.
+Example of usage:
+
+	make check RUNTESTFLAGS="GDB=gdb_valgrind\ $PWD/gdb FORCE_LOCAL_GDB_QUIT_WAIT=1" FORCE_PARALLEL="1" -j3
+
+
+Note: GDB is best run under valgrind with a bunch of specific options, so you
+might use a wrapper script (e.g. gdb_valgrind) that sets the appropriate
+options to run GDB.
+
 Race detection
 **************
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index c5fda7477e..ea94ca08f5 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -1432,6 +1432,7 @@ proc gdb_reinitialize_dir { subdir } {
 proc default_gdb_exit {} {
     global GDB
     global INTERNAL_GDBFLAGS GDBFLAGS
+    global FORCE_LOCAL_GDB_QUIT_WAIT
     global verbose
     global gdb_spawn_id inferior_spawn_id
     global inotify_log_file
@@ -1470,8 +1471,44 @@ proc default_gdb_exit {} {
 	}
     }
 
-    if ![is_remote host] {
-	remote_close host
+    if {![is_remote host] } {
+	# If FORCE_LOCAL_GDB_QUIT_WAIT, do our best to capture the full output
+	# of the launched GDB.
+	# This is in particular needed when running GDB under valgrind,
+	# as valgrind produces some output after the dejagnu test is
+	# terminated.
+	if { [info exists FORCE_LOCAL_GDB_QUIT_WAIT] } {
+	    send_gdb "\003"
+	    gdb_expect 30 {
+		-re ".*Quit" { }
+		timeout { fail "force quit interrupt timeout" }
+		eof { }
+	    }
+	    send_gdb "quit\n"
+	    gdb_expect 30 {
+		-re "y or n.*" {
+		    send_gdb "y\n"
+		    exp_continue
+		    # ??? If the above exp_continue is commented out, then the
+		    # output of valgrind is incomplete.  But keeping the above
+		    # exp_continue causes the below 'force quit timeout'
+		    # e.g. with gdb.base/pie-fork.exp ???
+		}
+		eof { }
+		timeout {
+		    fail "force quit timeout"
+		    set gdb_pid [spawn_id_get_pid $gdb_spawn_id]
+		    remote_exec host "kill -TERM $gdb_pid"
+		}
+		-re "DOSEXIT code" { }
+	    }
+	    # wait -i $gdb_spawn_id
+	    # ??? Trying to bypass the above pie-fork.exp timeout by commenting
+	    # out exp_continue but adding a 'wait' here causes a 'Child process
+	    # exited abnormally' problem at the end of execution.
+	} else {
+	    remote_close host
+	}
     }
     unset gdb_spawn_id
     unset inferior_spawn_id
-- 
2.19.2

From f2ec452344d49108f3769cb8096806fa7c2d17ee Mon Sep 17 00:00:00 2001
From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
Date: Thu, 27 Dec 2018 00:14:36 +0100
Subject: [PATCH 2/2] Implement FORCE_LOCAL_GDB_QUIT_WAIT in mi mode

Change mi_gdb_exit similarly to gdb_exit.
---
 gdb/testsuite/lib/mi-support.exp | 60 +++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index 235d03e902..8b02134675 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -55,6 +55,8 @@ proc mi_gdb_exit {} {
 proc mi_uncatched_gdb_exit {} {
     global GDB
     global INTERNAL_GDBFLAGS GDBFLAGS
+    global FORCE_LOCAL_GDB_QUIT_WAIT
+    global async
     global verbose
     global gdb_spawn_id gdb_main_spawn_id
     global mi_spawn_id inferior_spawn_id
@@ -91,7 +93,63 @@ proc mi_uncatched_gdb_exit {} {
     }
 
     if ![is_remote host] {
-	remote_close host
+	# The logic here should be similar to gdb.exp (default_gdb_exit).
+	# See comments/explanations in gdb.exp.
+
+	if { [info exists FORCE_LOCAL_GDB_QUIT_WAIT] } {
+
+	    # # Send the interrupt request.
+	    # if { $async } {
+	    # 	mi_gdb_test "888-exec-interrupt" "888\\^done" "interrupt"
+	    # } else {
+	    # 	send_gdb "\003"
+	    # }
+	    # mi_expect_interrupt
+	    # ??? no idea how to properly interrupt in mi mode.
+	    # ??? and should we interrupt in mi mode ?
+
+	    # Some mi tests are failing with time out, such as as mi-watch.exp,
+	    # with:
+# (gdb) 
+# PASS: gdb.mi/mi-watch.exp: mi-mode=separate: wp-type=hw: watchpoint trigger
+# 999-gdb-exit
+# 999^exit
+# =thread-exited,id="1",group-id="i1"
+# =thread-group-exited,id="i1"
+# FAIL: gdb.mi/mi-watch.exp: mi-mode=separate: wp-type=hw: mi quit timeout
+# Executing on host: kill -TERM 0    (timeout = 300)
+# spawn -ignore SIGHUP kill -TERM 0
+	    # and I cannot see any difference with a succesful test,
+	    # such as mi-return.exp:
+# PASS: gdb.mi/mi-eval.exp: eval A + 3
+# 999-gdb-exit
+# 999^exit
+# =thread-exited,id="1",group-id="i1"
+# =thread-group-exited,id="i1"
+# testcase /bd/home/philippe/gdb/git/build_obvious/gdb/testsuite/../../../obvious/gdb/testsuite/gdb.mi/mi-eval.exp completed in 0 seconds
+
+	    send_gdb "999-gdb-exit\n"
+	    gdb_expect 30 {
+		-re "y or n.*" {
+		    send_gdb "y\n"
+		    exp_continue
+		}
+		-re "Undefined command.*$gdb_prompt $" {
+		    send_gdb "quit\n"
+		    exp_continue
+		}
+		eof { }
+		timeout {
+		    fail "mi force quit timeout"
+		    set gdb_pid [spawn_id_get_pid $mi_spawn_id]
+		    remote_exec host "kill -TERM $gdb_pid"
+		}
+		-re "DOSEXIT code" { }
+	    }
+	    # wait -i $gdb_spawn_id
+	} else {
+	    remote_close host
+	}
     }
     unset gdb_spawn_id
     unset gdb_main_spawn_id
-- 
2.19.2


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]