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

Message ID 20230106185729.42372-7-simon.marchi@efficios.com
State Committed
Commit faee137249c8494e36d0170c0a57319113f54185
Headers
Series Fix gdb.dap/basic-dap.exp for PIE |

Commit Message

Simon Marchi Jan. 6, 2023, 6:57 p.m. UTC
  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(-)
  

Comments

Tom Tromey Jan. 25, 2023, 10:04 p.m. UTC | #1
>>>>> "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
  
Simon Marchi Jan. 26, 2023, 3:29 a.m. UTC | #2
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
  
Tom Tromey Jan. 26, 2023, 2:55 p.m. UTC | #3
Simon> Based on this, my commit message would be:
...

Sounds good to me, thanks again.

Tom
  

Patch

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]
 }