[PATCH 0/9] Fix gdb.dap/basic-dap.exp for PIE

[PATCH 0/9] Fix gdb.dap/basic-dap.exp for PIE

[Simon Marchi]

The last patch of this series fixes a FAIL I get in
gdb.dap/basic-dap.exp.  The rest of the patches are small improvements I
made to lib/dap-support.exp that helped me get there.

Simon Marchi (9):
  gdb/testsuite/dap: use gdb_assert in gdb.dap/basic-dap.exp
  gdb/testsuite/dap: prefix some procs with _
  gdb/testsuite/dap: write requests to gdb.log
  gdb/testsuite/dap: make dap_request_and_response not catch / issue
    test result
  gdb/testsuite/dap: remove catch from dap_read_event
  gdb/testsuite/dap: pass around dicts instead of ton objects
  gdb/testsuite/dap: rename dap_read_event to
    dap_wait_for_event_and_check
  gdb/testsuite/dap: make dap_wait_for_event_and_check return preceding
    messages
  gdb/testsuite/dap: fix gdb.dap/basic-dap.exp disassembly test for PIE

 gdb/testsuite/gdb.dap/basic-dap.exp |  73 ++++++++++-------
 gdb/testsuite/lib/dap-support.exp   | 121 ++++++++++++++--------------
 2 files changed, 102 insertions(+), 92 deletions(-)

-- 
2.38.1

[PATCH 1/9] gdb/testsuite/dap: use gdb_assert in gdb.dap/basic-dap.exp

[Simon Marchi]

Use gdb_assert instead of manual pass/fail.

Change-Id: I71fbc4e37a0a1ef4783056c7424e932651fa397f
---
 gdb/testsuite/gdb.dap/basic-dap.exp | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/gdb/testsuite/gdb.dap/basic-dap.exp b/gdb/testsuite/gdb.dap/basic-dap.exp
index d3acf0c22c8..c60989d2a6f 100644
--- a/gdb/testsuite/gdb.dap/basic-dap.exp
+++ b/gdb/testsuite/gdb.dap/basic-dap.exp
@@ -75,12 +75,7 @@ set obj [dap_check_request_and_response "reset breakpoint by line number" \
 	     [format {o source [o path [%s]] breakpoints [a [o line [i %d]]]} \
 		  [list s $srcfile] $line]]
 set new_line_bpno [dap_get_breakpoint_number $obj]
-
-if {$new_line_bpno == $line_bpno} {
-    pass "re-setting kept same breakpoint number"
-} else {
-    fail "re-setting kept same breakpoint number"
-}
+gdb_assert {$new_line_bpno == $line_bpno} "re-setting kept same breakpoint number"
 
 # This uses "&address_breakpoint_here" as the address -- this is a
 # hack because we know how this is implemented under the hood.
@@ -131,21 +126,13 @@ dap_match_values "global value in main" [lindex $obj 0] \
 set obj [dap_request_and_response "evaluate non-existing variable" \
 	     evaluate {o expression [s nosuchvariable]}]
 set d [namespace eval ton::2dict [lindex $obj 0]]
-if {[dict get $d success] == "false"} {
-    pass "result of invalid request"
-} else {
-    fail "result of invalid request"
-}
+gdb_assert { [dict get $d success] == "false" } "result of invalid request"
 
 set obj [dap_check_request_and_response "disassemble one instruction" \
 	     disassemble \
 	     [format {o memoryReference [s %s] instructionCount [i 1]} \
 		  $insn_pc]]
 set d [namespace eval ton::2dict [lindex $obj 0]]
-if {[dict exists $d body instructions]} {
-    pass "instructions in disassemble output"
-} else {
-    fail "instructions in disassemble output"
-}
+gdb_assert { [dict exists $d body instructions] } "instructions in disassemble output"
 
 dap_shutdown
-- 
2.38.1

Re: [PATCH 1/9] gdb/testsuite/dap: use gdb_assert in gdb.dap/basic-dap.exp

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Use gdb_assert instead of manual pass/fail.

Ok.  Thanks for doing this, I still haven't really internalized
gdb_assert.

Tom

[PATCH 2/9] gdb/testsuite/dap: prefix some procs with _

[Simon Marchi]

Prefix some procs that are only used internally with an underscore, to
make it clear they are internal.  If they need to be used by some test
later, we can always un-prefix them.

Change-Id: Iacb8e77363b5d1f8b98d9ba5a6d115aee5c8925d
---
 gdb/testsuite/lib/dap-support.exp | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/gdb/testsuite/lib/dap-support.exp b/gdb/testsuite/lib/dap-support.exp
index adf332cd7a5..a91b5533643 100644
--- a/gdb/testsuite/lib/dap-support.exp
+++ b/gdb/testsuite/lib/dap-support.exp
@@ -98,7 +98,7 @@ proc dap_to_ton {obj} {
 }
 
 # Format the object OBJ, in TON format, as JSON and send it to gdb.
-proc dap_send_ton {obj} {
+proc _dap_send_ton {obj} {
     set json [namespace eval ton::2json $obj]
     # FIXME this is wrong for non-ASCII characters.
     set len [string length $json]
@@ -111,7 +111,7 @@ proc dap_send_ton {obj} {
 # omitted.  The sequence number of the request is automatically added,
 # and this is also the return value.  OBJ is assumed to already be in
 # TON form.
-proc dap_send_request {command {obj {}}} {
+proc _dap_send_request {command {obj {}}} {
     # We can construct this directly as a TON object.
     set result $::dap_seq
     incr ::dap_seq
@@ -120,13 +120,13 @@ proc dap_send_request {command {obj {}}} {
     if {$obj != ""} {
 	append req " arguments \[$obj\]"
     }
-    dap_send_ton $req
+    _dap_send_ton $req
     return $result
 }
 
 # Read a JSON response from gdb.  This will return a TON object on
 # success, or throw an exception on error.
-proc dap_read_json {} {
+proc _dap_read_json {} {
     set length ""
     gdb_expect {
 	-re "^Content-Length: (\[0-9\]+)\r\n" {
@@ -181,10 +181,10 @@ proc dap_read_json {} {
 # emitted.  The objects are in TON format.  If a response object is
 # seen but has the wrong sequence number or command, throw an
 # exception
-proc dap_read_response {cmd num} {
+proc _dap_read_response {cmd num} {
     set result {}
     while 1 {
-	set obj [dap_read_json]
+	set obj [_dap_read_json]
 	set d [namespace eval ton::2dict $obj]
 	if {[dict get $d type] == "response"} {
 	    if {[dict get $d request_seq] != $num} {
@@ -200,14 +200,14 @@ proc dap_read_response {cmd num} {
     }
 }
 
-# A wrapper for dap_send_request and dap_read_response.  This sends a
+# A wrapper for _dap_send_request and _dap_read_response.  This sends a
 # request to gdb and returns the result.  NAME is used to issue a pass
 # or fail; on failure, this always returns an empty string.
 proc dap_request_and_response {name command {obj {}}} {
     set result {}
     if {[catch {
-	set seq [dap_send_request $command $obj]
-	set result [dap_read_response $command $seq]
+	set seq [_dap_send_request $command $obj]
+	set result [_dap_read_response $command $seq]
     } text]} {
 	verbose "reason: $text"
 	fail $name
@@ -239,7 +239,7 @@ proc dap_check_request_and_response {name command {obj {}}} {
 # desired.  Callers not caring about this should probably use
 # dap_launch.  Returns the empty string on failure.  NAME is used as
 # the test name.
-proc dap_initialize {name} {
+proc _dap_initialize {name} {
     if {[dap_gdb_start]} {
 	return ""
     }
@@ -254,7 +254,7 @@ proc dap_initialize {name} {
 # reasonable default but can be overridden in case a test needs to
 # launch gdb more than once.
 proc dap_launch {file {name startup}} {
-    if {[dap_initialize "$name - initialize"] == ""} {
+    if {[_dap_initialize "$name - initialize"] == ""} {
 	return ""
     }
     return [dap_check_request_and_response "$name - launch" launch \
@@ -303,7 +303,7 @@ proc _dap_read_event {type} {
     while 1 {
 	# We don't do any extra error checking here for the time
 	# being; we'll just get a timeout thrown instead.
-	set obj [dap_read_json]
+	set obj [_dap_read_json]
 	set d [namespace eval ton::2dict $obj]
 	if {[dict get $d type] == "event"
 	    && [dict get $d event] == $type} {
-- 
2.38.1

Re: [PATCH 2/9] gdb/testsuite/dap: prefix some procs with _

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Prefix some procs that are only used internally with an underscore, to
Simon> make it clear they are internal.  If they need to be used by some test
Simon> later, we can always un-prefix them.

Looks good to me.
Maybe I should have done the newfangled thing and used a namespace.

Tom

[PATCH 3/9] gdb/testsuite/dap: write requests to gdb.log

[Simon Marchi]

This helps following what happens when reading gdb.log.  The downside is
that it becomes harder to tell what text is from GDB and what text is
going to GDB, but I think that seeing responses without seeing requests
is even more confusing.  At least, the lines are prefix with >>>, so
when you see this, you know that until the end of the line, it's
something that was sent to GDB, and not GDB output.

Change-Id: I1ba1acd8b16f4e64686c5ad268cc41082951c874
---
 gdb/testsuite/lib/dap-support.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/lib/dap-support.exp b/gdb/testsuite/lib/dap-support.exp
index a91b5533643..94a0d27c8a8 100644
--- a/gdb/testsuite/lib/dap-support.exp
+++ b/gdb/testsuite/lib/dap-support.exp
@@ -102,7 +102,7 @@ proc _dap_send_ton {obj} {
     set json [namespace eval ton::2json $obj]
     # FIXME this is wrong for non-ASCII characters.
     set len [string length $json]
-    verbose ">>> $json"
+    verbose -log ">>> $json"
     send_gdb "Content-Length: $len\r\n\r\n$json"
 }
 
-- 
2.38.1

Re: [PATCH 3/9] gdb/testsuite/dap: write requests to gdb.log

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> This helps following what happens when reading gdb.log.  The downside is
Simon> that it becomes harder to tell what text is from GDB and what text is
Simon> going to GDB, but I think that seeing responses without seeing requests
Simon> is even more confusing.  At least, the lines are prefix with >>>, so
Simon> when you see this, you know that until the end of the line, it's
Simon> something that was sent to GDB, and not GDB output.

Seems fine to me, and if it is ever annoying, we can always revert it.

Tom

Re: [PATCH 3/9] gdb/testsuite/dap: write requests to gdb.log

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> This helps following what happens when reading gdb.log.  The downside is
Simon> that it becomes harder to tell what text is from GDB and what text is
Simon> going to GDB, but I think that seeing responses without seeing requests
Simon> is even more confusing.  At least, the lines are prefix with >>>, so
Simon> when you see this, you know that until the end of the line, it's
Simon> something that was sent to GDB, and not GDB output.

I meant to mention -- the full DAP I/O is in the test subdir.
E.g. look at gdb/testsuite/outputs/gdb.dap/basic-dap/dap.log.1 in the
build tree.

Tom

Re: [PATCH 3/9] gdb/testsuite/dap: write requests to gdb.log

[Simon Marchi]

On 1/25/23 17:01, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> This helps following what happens when reading gdb.log.  The downside is
> Simon> that it becomes harder to tell what text is from GDB and what text is
> Simon> going to GDB, but I think that seeing responses without seeing requests
> Simon> is even more confusing.  At least, the lines are prefix with >>>, so
> Simon> when you see this, you know that until the end of the line, it's
> Simon> something that was sent to GDB, and not GDB output.
> 
> I meant to mention -- the full DAP I/O is in the test subdir.
> E.g. look at gdb/testsuite/outputs/gdb.dap/basic-dap/dap.log.1 in the
> build tree.

Yeah, I saw that.  But to debug the actual test, I found that having the
full communication in gdb.log was a bit easier, to correlate with passes
and fails.

Simon

[PATCH 4/9] gdb/testsuite/dap: make dap_request_and_response not catch / issue test result

[Simon Marchi]

Following some of my changes, dap_request_and_response was failing and I
didn't know why.  I think it's better to make it not catch any
exception, and just make it do a simple "send request, read response".
If an exception is thrown while sending a request or reading a response,
things are going really badly, it's not like we'll want to recover from
that and continue the test.

Change-Id: I27568d3547f753c3a74e3e5a730d38a8caef9356
---
 gdb/testsuite/gdb.dap/basic-dap.exp |  2 +-
 gdb/testsuite/lib/dap-support.exp   | 25 ++++++-------------------
 2 files changed, 7 insertions(+), 20 deletions(-)

diff --git a/gdb/testsuite/gdb.dap/basic-dap.exp b/gdb/testsuite/gdb.dap/basic-dap.exp
index c60989d2a6f..af55d19349f 100644
--- a/gdb/testsuite/gdb.dap/basic-dap.exp
+++ b/gdb/testsuite/gdb.dap/basic-dap.exp
@@ -123,7 +123,7 @@ set obj [dap_check_request_and_response "evaluate global in main" \
 dap_match_values "global value in main" [lindex $obj 0] \
     "body result" 25
 
-set obj [dap_request_and_response "evaluate non-existing variable" \
+set obj [dap_request_and_response \
 	     evaluate {o expression [s nosuchvariable]}]
 set d [namespace eval ton::2dict [lindex $obj 0]]
 gdb_assert { [dict get $d success] == "false" } "result of invalid request"
diff --git a/gdb/testsuite/lib/dap-support.exp b/gdb/testsuite/lib/dap-support.exp
index 94a0d27c8a8..bc99f0182a7 100644
--- a/gdb/testsuite/lib/dap-support.exp
+++ b/gdb/testsuite/lib/dap-support.exp
@@ -201,29 +201,16 @@ proc _dap_read_response {cmd num} {
 }
 
 # A wrapper for _dap_send_request and _dap_read_response.  This sends a
-# request to gdb and returns the result.  NAME is used to issue a pass
-# or fail; on failure, this always returns an empty string.
-proc dap_request_and_response {name command {obj {}}} {
-    set result {}
-    if {[catch {
-	set seq [_dap_send_request $command $obj]
-	set result [_dap_read_response $command $seq]
-    } text]} {
-	verbose "reason: $text"
-	fail $name
-    } else {
-	pass $name
-    }
-    return $result
+# request to gdb and returns the response as a dict.
+proc dap_request_and_response {command {obj {}}} {
+    set seq [_dap_send_request $command $obj]
+    return [_dap_read_response $command $seq]
 }
 
 # Like dap_request_and_response, but also checks that the response
-# indicates success.
+# indicates success.  NAME is used to issue a test result.
 proc dap_check_request_and_response {name command {obj {}}} {
-    set result [dap_request_and_response $name $command $obj]
-    if {$result == ""} {
-	return ""
-    }
+    set result [dap_request_and_response $command $obj]
     set d [namespace eval ton::2dict [lindex $result 0]]
     if {[dict get $d success] != "true"} {
 	verbose "request failure: $result"
-- 
2.38.1

[PATCH 5/9] gdb/testsuite/dap: remove catch from dap_read_event

[Simon Marchi]

This catch didn't cause me any trouble, but for the same reason as the
preceding patch, I think it's a bit better to just let any exception
propagate, to make for easier debugging.

Change-Id: I1779e62c788b77fef2d50434edf4c3d2ec5e1c4c
---
 gdb/testsuite/lib/dap-support.exp | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/gdb/testsuite/lib/dap-support.exp b/gdb/testsuite/lib/dap-support.exp
index bc99f0182a7..8c85f90c352 100644
--- a/gdb/testsuite/lib/dap-support.exp
+++ b/gdb/testsuite/lib/dap-support.exp
@@ -310,11 +310,7 @@ proc dap_read_event {name type args} {
 	set name $type
     }
 
-    if {[catch {_dap_read_event $type} result]} {
-	fail $name
-	return ""
-    }
-
+    set result [_dap_read_event $type]
     eval dap_match_values [list $name $result] $args
 
     return $result
-- 
2.38.1

[PATCH 6/9] gdb/testsuite/dap: pass around dicts instead of ton objects

[Simon Marchi]

Change-Id: I2ca47bea355bf459090bae8680c6a917350b5c3f
---
 gdb/testsuite/gdb.dap/basic-dap.exp | 15 +++++----
 gdb/testsuite/lib/dap-support.exp   | 48 ++++++++++++++---------------
 2 files changed, 30 insertions(+), 33 deletions(-)

diff --git a/gdb/testsuite/gdb.dap/basic-dap.exp b/gdb/testsuite/gdb.dap/basic-dap.exp
index af55d19349f..68d0440f692 100644
--- a/gdb/testsuite/gdb.dap/basic-dap.exp
+++ b/gdb/testsuite/gdb.dap/basic-dap.exp
@@ -53,8 +53,7 @@ set line_bpno [dap_get_breakpoint_number $obj]
 
 # Check the new breakpoint event.
 set ok 0
-foreach event [lindex $obj 1] {
-    set d [namespace eval ton::2dict $event]
+foreach d [lindex $obj 1] {
     if {[dict get $d type] != "event"
 	|| [dict get $d event] != "breakpoint"} {
 	continue
@@ -84,8 +83,8 @@ set obj [dap_check_request_and_response "set breakpoint by address" \
 	     {o breakpoints [a [o instructionReference [s &address_breakpoint_here]]]}]
 set insn_bpno [dap_get_breakpoint_number $obj]
 
-set d [namespace eval ton::2dict [lindex $obj 0]]
-set bplist [dict get $d body breakpoints]
+set response [lindex $obj 0]
+set bplist [dict get $response body breakpoints]
 set insn_pc [dict get [lindex $bplist 0] instructionReference]
 
 dap_check_request_and_response "start inferior" configurationDone
@@ -125,14 +124,14 @@ dap_match_values "global value in main" [lindex $obj 0] \
 
 set obj [dap_request_and_response \
 	     evaluate {o expression [s nosuchvariable]}]
-set d [namespace eval ton::2dict [lindex $obj 0]]
-gdb_assert { [dict get $d success] == "false" } "result of invalid request"
+set response [lindex $obj 0]
+gdb_assert { [dict get $response success] == "false" } "result of invalid request"
 
 set obj [dap_check_request_and_response "disassemble one instruction" \
 	     disassemble \
 	     [format {o memoryReference [s %s] instructionCount [i 1]} \
 		  $insn_pc]]
-set d [namespace eval ton::2dict [lindex $obj 0]]
-gdb_assert { [dict exists $d body instructions] } "instructions in disassemble output"
+set response [lindex $obj 0]
+gdb_assert { [dict exists $response body instructions] } "instructions in disassemble output"
 
 dap_shutdown
diff --git a/gdb/testsuite/lib/dap-support.exp b/gdb/testsuite/lib/dap-support.exp
index 8c85f90c352..ee55a4acc9c 100644
--- a/gdb/testsuite/lib/dap-support.exp
+++ b/gdb/testsuite/lib/dap-support.exp
@@ -124,7 +124,7 @@ proc _dap_send_request {command {obj {}}} {
     return $result
 }
 
-# Read a JSON response from gdb.  This will return a TON object on
+# Read a JSON response from gdb.  This will return a dict on
 # success, or throw an exception on error.
 proc _dap_read_json {} {
     set length ""
@@ -171,31 +171,31 @@ proc _dap_read_json {} {
 	incr length -$this_len
     }
 
-    return [ton::json2ton $json]
+    set ton [ton::json2ton $json]
+    return [namespace eval ton::2dict $ton]
 }
 
 # Read a sequence of JSON objects from gdb, until a response object is
 # seen.  If the response object has the request sequence number NUM,
 # and is for command CMD, return a list of two elements: the response
 # object and a list of any preceding events, in the order they were
-# emitted.  The objects are in TON format.  If a response object is
-# seen but has the wrong sequence number or command, throw an
-# exception
+# emitted.  The objects are dicts.  If a response object is seen but has
+# the wrong sequence number or command, throw an exception
+
 proc _dap_read_response {cmd num} {
     set result {}
     while 1 {
-	set obj [_dap_read_json]
-	set d [namespace eval ton::2dict $obj]
+	set d [_dap_read_json]
 	if {[dict get $d type] == "response"} {
 	    if {[dict get $d request_seq] != $num} {
 		error "saw wrong request_seq in $obj"
 	    } elseif {[dict get $d command] != $cmd} {
 		error "saw wrong command in $obj"
 	    } else {
-		return [list $obj $result]
+		return [list $d $result]
 	    }
 	} else {
-	    lappend result $obj
+	    lappend result $d
 	}
     }
 }
@@ -210,15 +210,15 @@ proc dap_request_and_response {command {obj {}}} {
 # Like dap_request_and_response, but also checks that the response
 # indicates success.  NAME is used to issue a test result.
 proc dap_check_request_and_response {name command {obj {}}} {
-    set result [dap_request_and_response $command $obj]
-    set d [namespace eval ton::2dict [lindex $result 0]]
-    if {[dict get $d success] != "true"} {
-	verbose "request failure: $result"
+    set response_and_events [dap_request_and_response $command $obj]
+    set response [lindex $response_and_events 0]
+    if {[dict get $response success] != "true"} {
+	verbose "request failure: $response"
 	fail "$name success"
 	return ""
     }
     pass "$name success"
-    return $result
+    return $response_and_events
 }
 
 # Start gdb, send a DAP initialization request and return the
@@ -257,8 +257,7 @@ proc dap_shutdown {{name shutdown}} {
 # Search the event list EVENTS for an output event matching the regexp
 # RX.  Pass the test NAME if found, fail if not.
 proc dap_search_output {name rx events} {
-    foreach event $events {
-	set d [namespace eval ton::2dict $event]
+    foreach d $events {
 	if {[dict get $d type] != "event"
 	    || [dict get $d event] != "output"} {
 	    continue
@@ -271,10 +270,9 @@ proc dap_search_output {name rx events} {
     fail $name
 }
 
-# Check that OBJ (a single TON object) has values that match the
+# Check that D (a dict object) has values that match the
 # key/value pairs given in ARGS.  NAME is used as the test name.
-proc dap_match_values {name obj args} {
-    set d [namespace eval ton::2dict $obj]
+proc dap_match_values {name d args} {
     foreach {key value} $args {
 	if {[eval dict get [list $d] $key] != $value} {
 	    fail "$name (checking $key)"
@@ -290,21 +288,21 @@ proc _dap_read_event {type} {
     while 1 {
 	# We don't do any extra error checking here for the time
 	# being; we'll just get a timeout thrown instead.
-	set obj [_dap_read_json]
-	set d [namespace eval ton::2dict $obj]
+	set d [_dap_read_json]
 	if {[dict get $d type] == "event"
 	    && [dict get $d event] == $type} {
-	    return $obj
+	    return $d
 	}
     }
 }
 
 # Read JSON objects looking for an event whose "event" field is TYPE.
+#
 # NAME is used as the test name; it defaults to TYPE.  Extra arguments
 # are used to check fields of the event; the arguments alternate
 # between a field name (in "dict get" form) and its expected value.
-# Returns the TON object for the chosen event, or empty string on
-# error.
+#
+# Returns the dict for the chosen event, or empty string on error.
 proc dap_read_event {name type args} {
     if {$name == ""} {
 	set name $type
@@ -320,7 +318,7 @@ proc dap_read_event {name type args} {
 # breakpoint is created.  OBJ is an object as returned by
 # dap_check_request_and_response.
 proc dap_get_breakpoint_number {obj} {
-    set d [namespace eval ton::2dict [lindex $obj 0]]
+    set d [lindex $obj 0]
     set bplist [dict get $d body breakpoints]
     return [dict get [lindex $bplist 0] id]
 }
-- 
2.38.1

Re: [PATCH 6/9] gdb/testsuite/dap: pass around dicts instead of ton objects

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Change-Id: I2ca47bea355bf459090bae8680c6a917350b5c3f

Probably could use a commit message.

The main issue with this is that dicts lose type information from the
JSON.  So, if we ever need to check the type of some value sent by gdb,
we won't be able to.

But I suppose we could always just add some new proc that returns the
TON form and go from there.  I don't recall if I've needed any such
tests yet.  Arguably maybe we should write one.

Tom

Re: [PATCH 6/9] gdb/testsuite/dap: pass around dicts instead of ton objects

[Simon Marchi]

On 1/25/23 17:04, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> Change-Id: I2ca47bea355bf459090bae8680c6a917350b5c3f
> 
> Probably could use a commit message.

Oops, that was a mistake.  I always put at least a little something in
the commit messages, except for the extremely obvious commits.

> The main issue with this is that dicts lose type information from the
> JSON.  So, if we ever need to check the type of some value sent by gdb,
> we won't be able to.

Ah, interesting.

> But I suppose we could always just add some new proc that returns the
> TON form and go from there.  I don't recall if I've needed any such
> tests yet.  Arguably maybe we should write one.

The idea of my change was that we currently did `namespace eval
ton::2dict` pretty much everywhere.  So I thought it would be nice to
make it easier on callers and to the ton::2dict for them, that makes the
tests less verbose.

I think we can indeed add intermediary functions that return ton objects
as needed.  For instance, have dap_request_and_response_ton can do most
of the work, and have dap_request_and_response be a small wrapper around
it to do the ton::2dict.

Based on this, my commit message would be:

    The DAP helper functions generally return TON objects.  However, callers
    almost all immediately use ton::2dict to convert them to dicts, to
    access their contents.  This commits makes things a bit simpler for them
    by having function return dicts directly instead.

    The downside is that the TON objects contain type information.  For
    instance, a "2" in a TCL dict could have been the integer 2 or the
    string "2" in JSON.  By converting to TCL dicts, we lose that
    information.  If some tests specifically want to check the types of some
    fields, I think we can add intermediary functions that return TON
    objects, without having to complicate other callers who don't care.

Simon

Re: [PATCH 6/9] gdb/testsuite/dap: pass around dicts instead of ton objects

[Tom Tromey]

Simon> Based on this, my commit message would be:
...

Sounds good to me, thanks again.

Tom

[PATCH 7/9] gdb/testsuite/dap: rename dap_read_event to dap_wait_for_event_and_check

[Simon Marchi]

I think that name describes a bit better what the proc does, it is
similar to "wait_for" in tuiterm.exp.

Change-Id: Ie55aa011e6595dd1b5a874db13881ba572ace419
---
 gdb/testsuite/gdb.dap/basic-dap.exp | 10 +++++-----
 gdb/testsuite/lib/dap-support.exp   |  6 +++---
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/gdb/testsuite/gdb.dap/basic-dap.exp b/gdb/testsuite/gdb.dap/basic-dap.exp
index 68d0440f692..795702a5a8f 100644
--- a/gdb/testsuite/gdb.dap/basic-dap.exp
+++ b/gdb/testsuite/gdb.dap/basic-dap.exp
@@ -88,9 +88,9 @@ set bplist [dict get $response body breakpoints]
 set insn_pc [dict get [lindex $bplist 0] instructionReference]
 
 dap_check_request_and_response "start inferior" configurationDone
-dap_read_event "inferior started" thread "body reason" started
+dap_wait_for_event_and_check "inferior started" thread "body reason" started
 
-dap_read_event "stopped at function breakpoint" stopped \
+dap_wait_for_event_and_check "stopped at function breakpoint" stopped \
     "body reason" breakpoint \
     "body hitBreakpointIds" $fn_bpno
 
@@ -100,7 +100,7 @@ dap_match_values "global value in function" [lindex $obj 0] \
     "body result" 23
 
 dap_check_request_and_response step stepIn {o threadId [i 1]}
-dap_read_event "stopped after step" stopped "body reason" step
+dap_wait_for_event_and_check "stopped after step" stopped "body reason" step
 
 set obj [dap_check_request_and_response "evaluate global second time" \
 	     evaluate {o expression [s global_variable]}]
@@ -108,12 +108,12 @@ dap_match_values "global value after step" [lindex $obj 0] \
     "body result" 24
 
 dap_check_request_and_response "continue to address" continue
-dap_read_event "stopped at address breakpoint" stopped \
+dap_wait_for_event_and_check "stopped at address breakpoint" stopped \
     "body reason" breakpoint \
     "body hitBreakpointIds" $insn_bpno
 
 dap_check_request_and_response "continue to line" continue
-dap_read_event "stopped at line breakpoint" stopped \
+dap_wait_for_event_and_check "stopped at line breakpoint" stopped \
     "body reason" breakpoint \
     "body hitBreakpointIds" $line_bpno
 
diff --git a/gdb/testsuite/lib/dap-support.exp b/gdb/testsuite/lib/dap-support.exp
index ee55a4acc9c..179f675b4d8 100644
--- a/gdb/testsuite/lib/dap-support.exp
+++ b/gdb/testsuite/lib/dap-support.exp
@@ -284,7 +284,7 @@ proc dap_match_values {name d args} {
 
 # A helper for dap_read_event that reads events, looking for one
 # matching TYPE.
-proc _dap_read_event {type} {
+proc _dap_wait_for_event {type} {
     while 1 {
 	# We don't do any extra error checking here for the time
 	# being; we'll just get a timeout thrown instead.
@@ -303,12 +303,12 @@ proc _dap_read_event {type} {
 # between a field name (in "dict get" form) and its expected value.
 #
 # Returns the dict for the chosen event, or empty string on error.
-proc dap_read_event {name type args} {
+proc dap_wait_for_event_and_check {name type args} {
     if {$name == ""} {
 	set name $type
     }
 
-    set result [_dap_read_event $type]
+    set result [_dap_wait_for_event $type]
     eval dap_match_values [list $name $result] $args
 
     return $result
-- 
2.38.1

[PATCH 8/9] gdb/testsuite/dap: make dap_wait_for_event_and_check return preceding messages

[Simon Marchi]

In the following patch, I change gdb.dap/basic-dap.exp such that after
waiting for some event, it checks if it received another event
meanwhile.  To help with this, make dap_wait_for_event_and_check and
_dap_dap_wait_for_event return a list with everything received before
the event of interest.  This is similar to what
dap_check_request_and_response returns.

Change-Id: I85c8980203a2dec833937e7552c2196bc137935d
---
 gdb/testsuite/lib/dap-support.exp | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/lib/dap-support.exp b/gdb/testsuite/lib/dap-support.exp
index 179f675b4d8..527e7db5228 100644
--- a/gdb/testsuite/lib/dap-support.exp
+++ b/gdb/testsuite/lib/dap-support.exp
@@ -282,17 +282,27 @@ proc dap_match_values {name d args} {
     pass $name
 }
 
-# A helper for dap_read_event that reads events, looking for one
+# A helper for dap_wait_for_event_and_check that reads events, looking for one
 # matching TYPE.
-proc _dap_wait_for_event {type} {
+#
+# Return a list of two items:
+#
+#  - the matched event
+#  - a list of any JSON objects (events or others) seen before the matched
+#    event.
+proc _dap_wait_for_event { {type ""} } {
+    set preceding [list]
+
     while 1 {
 	# We don't do any extra error checking here for the time
 	# being; we'll just get a timeout thrown instead.
 	set d [_dap_read_json]
 	if {[dict get $d type] == "event"
-	    && [dict get $d event] == $type} {
-	    return $d
+	    && ($type == "" || [dict get $d event] == $type)} {
+	    return [list $d $preceding]
 	}
+
+	lappend preceding $d
     }
 }
 
@@ -302,14 +312,20 @@ proc _dap_wait_for_event {type} {
 # are used to check fields of the event; the arguments alternate
 # between a field name (in "dict get" form) and its expected value.
 #
-# Returns the dict for the chosen event, or empty string on error.
+# Return a list of two items:
+#
+#  - the matched event (regardless of whether it passed the field validation or
+#    not)
+#  - a list of any JSON objects (events or others) seen before the matched
+#    event.
 proc dap_wait_for_event_and_check {name type args} {
     if {$name == ""} {
 	set name $type
     }
 
     set result [_dap_wait_for_event $type]
-    eval dap_match_values [list $name $result] $args
+    set event [lindex $result 0]
+    eval dap_match_values [list $name $event] $args
 
     return $result
 }
-- 
2.38.1

[PATCH 9/9] gdb/testsuite/dap: fix gdb.dap/basic-dap.exp disassembly test for PIE

[Simon Marchi]

Prior to this patch, I get:

    >>> {"seq": 17, "type": "request", "command": "disassemble", "arguments": {"memoryReference": "0x115d", "instructionCount": 1}}
    Content-Length: 147

    {"request_seq": 17, "type": "response", "command": "disassemble", "success": false, "message": "Cannot access memory at address 0x115d", "seq": 41}FAIL: gdb.dap/basic-dap.exp: disassemble one instruction success
    FAIL: gdb.dap/basic-dap.exp: instructions in disassemble output

The problem is that the PC to disassemble is taken from the breakpoint
insertion response, which happens before running.  With a PIE
executable, that PC is unrelocated, but the disassembly request happens
after relocation.

I chose to fix this by watching for a breakpoint changed event giving
the new breakpoint address, and recording the address from there.  I
think this is an interesting way to fix it, because it adds a bit of
test coverage, I don't think these events are checked right now.

Other ways to fix it would be:

 - Get the address by doing a breakpoint insertion after the program is
   started, or some other way.
 - Do the disassembly by symbol instead of by address.
 - Do the disassembly before running the program.

Change-Id: I3c396f796ac4c8b22e7dfd2fa1c5467f7a47e84e
---
 gdb/testsuite/gdb.dap/basic-dap.exp | 33 ++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.dap/basic-dap.exp b/gdb/testsuite/gdb.dap/basic-dap.exp
index 795702a5a8f..5303e943320 100644
--- a/gdb/testsuite/gdb.dap/basic-dap.exp
+++ b/gdb/testsuite/gdb.dap/basic-dap.exp
@@ -90,9 +90,36 @@ set insn_pc [dict get [lindex $bplist 0] instructionReference]
 dap_check_request_and_response "start inferior" configurationDone
 dap_wait_for_event_and_check "inferior started" thread "body reason" started
 
-dap_wait_for_event_and_check "stopped at function breakpoint" stopped \
-    "body reason" breakpoint \
-    "body hitBreakpointIds" $fn_bpno
+# While waiting for the stopped event, we might receive breakpoint changed
+# events indicating some breakpoint addresses were relocated.  Update INSN_PC
+# if necessary.
+lassign [dap_wait_for_event_and_check "stopped at function breakpoint" stopped \
+	    "body reason" breakpoint \
+	    "body hitBreakpointIds" $fn_bpno] unused objs
+foreach obj $objs {
+    if { [dict get $obj "type"] != "event" } {
+	continue
+    }
+
+    if { [dict get $obj "event"] != "breakpoint" } {
+	continue
+    }
+
+    set body [dict get $obj "body"]
+
+    if { [dict get $body "reason"] != "changed" } {
+	continue
+    }
+
+    set breakpoint [dict get $body "breakpoint"]
+    set breakpoint_id [dict get $breakpoint "id"]
+
+    if { $breakpoint_id != $insn_bpno } {
+	continue
+    }
+
+    set insn_pc [dict get $breakpoint "instructionReference"]
+}
 
 set obj [dap_check_request_and_response "evaluate global in function" \
 	     evaluate {o expression [s global_variable]}]
-- 
2.38.1

Re: [PATCH 9/9] gdb/testsuite/dap: fix gdb.dap/basic-dap.exp disassembly test for PIE

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

>>>> {... {"memoryReference": "0x115d"

Simon> The problem is that the PC to disassemble is taken from the breakpoint
Simon> insertion response, which happens before running.  With a PIE
Simon> executable, that PC is unrelocated, but the disassembly request happens
Simon> after relocation.

This is an odd one.  It isn't super clear when memory references are
invalidated.  (The spec doesn't even define what a memory reference is.)

Simon> I chose to fix this by watching for a breakpoint changed event giving
Simon> the new breakpoint address, and recording the address from there.  I
Simon> think this is an interesting way to fix it, because it adds a bit of
Simon> test coverage, I don't think these events are checked right now.

This seems pretty good to me.

Simon>  - Do the disassembly by symbol instead of by address.

I don't think this is possible in DAP.  One of the many holes.

Tom

Re: [PATCH 9/9] gdb/testsuite/dap: fix gdb.dap/basic-dap.exp disassembly test for PIE

[Simon Marchi]

On 1/25/23 17:10, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>>>>> {... {"memoryReference": "0x115d"
> 
> Simon> The problem is that the PC to disassemble is taken from the breakpoint
> Simon> insertion response, which happens before running.  With a PIE
> Simon> executable, that PC is unrelocated, but the disassembly request happens
> Simon> after relocation.
> 
> This is an odd one.  It isn't super clear when memory references are
> invalidated.  (The spec doesn't even define what a memory reference is.)

I'm not sure what you mean.  The same could happen in a regular GDB
test, if you grabbed a function's address, started the program, and
tried to disassemble at that address.  I don't expect GDB to be able to
do anything with the unrelocated address obtained before running.

Since there isn't a DAP event that says when symbols have changed
(AFAIK), I think DAP clients have to assume that any symbol address may
be invalid after resuming the program.

> Simon> I chose to fix this by watching for a breakpoint changed event giving
> Simon> the new breakpoint address, and recording the address from there.  I
> Simon> think this is an interesting way to fix it, because it adds a bit of
> Simon> test coverage, I don't think these events are checked right now.
> 
> This seems pretty good to me.
> 
> Simon>  - Do the disassembly by symbol instead of by address.
> 
> I don't think this is possible in DAP.  One of the many holes.

When I saw this in the DisassembleArguments DAP structure:

  /**
   * Memory reference to the base location containing the instructions to
   * disassemble.
   */
  memoryReference: string;

I assumed that memoryReference could be pretty much anything that
resolves to an address, essentially the same thing you could pass to
GDB's disassemble command.  But that's just my interpretation of it.

Simon

Re: [PATCH 9/9] gdb/testsuite/dap: fix gdb.dap/basic-dap.exp disassembly test for PIE

[Tom Tromey]

>> This is an odd one.  It isn't super clear when memory references are
>> invalidated.  (The spec doesn't even define what a memory reference is.)

Simon> I'm not sure what you mean.

Actually I used the wrong word there.  In this case the term is really
"instruction reference"

Anyway, what I mean is that the DAP spec refers to "instruction
reference" without defining it.  So for example the response to a
setBreakpoints request is essentially an array of Breakpoint, which is
documented as:

  /**
   * A memory reference to where the breakpoint is set.
   */
  instructionReference?: string;

However, what are the semantics of this?  The spec does not say.  So, we
don't know what values might be valid (and pragmatically I think a
client has to assume it must treat them as opaque cookies).  When are
they invalidated, or are they assumed to be valid for the lifetime of
the inferior?

Maybe gdb could send an InvalidatedEvent but even this is pretty vague.

Simon> Since there isn't a DAP event that says when symbols have changed
Simon> (AFAIK), I think DAP clients have to assume that any symbol address may
Simon> be invalid after resuming the program.

I agree, though it's unwritten.

>> I don't think this is possible in DAP.  One of the many holes.

Simon> When I saw this in the DisassembleArguments DAP structure:

Simon>   /**
Simon>    * Memory reference to the base location containing the instructions to
Simon>    * disassemble.
Simon>    */
Simon>   memoryReference: string;

Simon> I assumed that memoryReference could be pretty much anything that
Simon> resolves to an address, essentially the same thing you could pass to
Simon> GDB's disassemble command.  But that's just my interpretation of it.

This would be convenient and perhaps it is what gdb ought to do, but I
don't see any text in the spec supporting this approach, so unless a
client specifically knows it will only talk to gdb, it seems risky for
them to use it.  And of course if you're specifically talking only to
gdb, you might as well use MI, which despite its flaws has solved all
the problems that DAP still has.

Probably what would be better for gdb is something that uses the DAP
transport -- JSON-RPC is just amazingly easy to work with -- but with MI
requests as the high-level protocol.  Or, that would have been better in
some alternate universe anyway.

Anyway I imagine the route forward is to file a bunch of DAP bugs and
try to get them resolved; though I haven't seen much progress on, e.g.,
the multiple location bug.

Tom

Re: [PATCH 9/9] gdb/testsuite/dap: fix gdb.dap/basic-dap.exp disassembly test for PIE

[Simon Marchi]

On 1/26/23 10:08, Tom Tromey wrote:
>>> This is an odd one.  It isn't super clear when memory references are
>>> invalidated.  (The spec doesn't even define what a memory reference is.)
> 
> Simon> I'm not sure what you mean.
> 
> Actually I used the wrong word there.  In this case the term is really
> "instruction reference"
> 
> Anyway, what I mean is that the DAP spec refers to "instruction
> reference" without defining it.  So for example the response to a
> setBreakpoints request is essentially an array of Breakpoint, which is
> documented as:
> 
>   /**
>    * A memory reference to where the breakpoint is set.
>    */
>   instructionReference?: string;
> 
> However, what are the semantics of this?  The spec does not say.  So, we
> don't know what values might be valid (and pragmatically I think a
> client has to assume it must treat them as opaque cookies).  When are
> they invalidated, or are they assumed to be valid for the lifetime of
> the inferior?
> 
> Maybe gdb could send an InvalidatedEvent but even this is pretty vague.

Ok, thanks for the explanation.

Well, the client gets an initial instructionReference for the
breakpoint.  But then GDB sends a "breakpoint changed" event for that
same breakpoint, with a new instructionReference.  That sounds like a
good way to tell the client "your old instructionReference for that
breakpoint is not longer valid".

> 
> Simon> Since there isn't a DAP event that says when symbols have changed
> Simon> (AFAIK), I think DAP clients have to assume that any symbol address may
> Simon> be invalid after resuming the program.
> 
> I agree, though it's unwritten.
> 
>>> I don't think this is possible in DAP.  One of the many holes.
> 
> Simon> When I saw this in the DisassembleArguments DAP structure:
> 
> Simon>   /**
> Simon>    * Memory reference to the base location containing the instructions to
> Simon>    * disassemble.
> Simon>    */
> Simon>   memoryReference: string;
> 
> Simon> I assumed that memoryReference could be pretty much anything that
> Simon> resolves to an address, essentially the same thing you could pass to
> Simon> GDB's disassemble command.  But that's just my interpretation of it.
> 
> This would be convenient and perhaps it is what gdb ought to do, but I
> don't see any text in the spec supporting this approach, so unless a
> client specifically knows it will only talk to gdb, it seems risky for
> them to use it.  And of course if you're specifically talking only to
> gdb, you might as well use MI, which despite its flaws has solved all
> the problems that DAP still has.
> 
> Probably what would be better for gdb is something that uses the DAP
> transport -- JSON-RPC is just amazingly easy to work with -- but with MI
> requests as the high-level protocol.  Or, that would have been better in
> some alternate universe anyway.

I would support changing MI to use JSON-RPC or similar.

At least, if the output could be JSON, rather than the current output
that kinda looks like JSON but isn't, it would be nice.

But even command input would benefit from having a better structure.  I
don't recall them at the moment (because I haven't worked on a frontend
for a long time), but there are some inconsistencies between commands,
how things should be quoted, escaped, etc.

> Anyway I imagine the route forward is to file a bunch of DAP bugs and
> try to get them resolved; though I haven't seen much progress on, e.g.,
> the multiple location bug.

It will happen when somebody at Microsoft needs it, I suppose.

Simon

Re: [PATCH 0/9] Fix gdb.dap/basic-dap.exp for PIE

[Simon Marchi]

Ping.

On 1/6/23 13:57, Simon Marchi via Gdb-patches wrote:
> The last patch of this series fixes a FAIL I get in
> gdb.dap/basic-dap.exp.  The rest of the patches are small improvements I
> made to lib/dap-support.exp that helped me get there.
> 
> Simon Marchi (9):
>   gdb/testsuite/dap: use gdb_assert in gdb.dap/basic-dap.exp
>   gdb/testsuite/dap: prefix some procs with _
>   gdb/testsuite/dap: write requests to gdb.log
>   gdb/testsuite/dap: make dap_request_and_response not catch / issue
>     test result
>   gdb/testsuite/dap: remove catch from dap_read_event
>   gdb/testsuite/dap: pass around dicts instead of ton objects
>   gdb/testsuite/dap: rename dap_read_event to
>     dap_wait_for_event_and_check
>   gdb/testsuite/dap: make dap_wait_for_event_and_check return preceding
>     messages
>   gdb/testsuite/dap: fix gdb.dap/basic-dap.exp disassembly test for PIE
> 
>  gdb/testsuite/gdb.dap/basic-dap.exp |  73 ++++++++++-------
>  gdb/testsuite/lib/dap-support.exp   | 121 ++++++++++++++--------------
>  2 files changed, 102 insertions(+), 92 deletions(-)
> 

Re: [PATCH 0/9] Fix gdb.dap/basic-dap.exp for PIE

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Ping.

I sent all my comments.  Nothing major, thank you very much for doing this.

Tom

Re: [PATCH 0/9] Fix gdb.dap/basic-dap.exp for PIE

[Simon Marchi]

On 1/25/23 17:10, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> Ping.
> 
> I sent all my comments.  Nothing major, thank you very much for doing this.
> 
> Tom

Ok, thanks.  I will push the series now.

Simon