[Bug testsuite/31382] New: [gdb/testsuite] 2 errors instead of 1 reported by _dap_read_json

[Bug testsuite/31382] New: [gdb/testsuite] 2 errors instead of 1 reported by _dap_read_json

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31382

            Bug ID: 31382
           Summary: [gdb/testsuite] 2 errors instead of 1 reported by
                    _dap_read_json
           Product: gdb
           Version: HEAD
            Status: NEW
          Severity: normal
          Priority: P2
         Component: testsuite
          Assignee: unassigned at sourceware dot org
          Reporter: vries at gcc dot gnu.org
  Target Milestone: ---

Consider test-case gdb.dap/eof.exp, modified like so:
...
diff --git a/gdb/testsuite/gdb.dap/eof.exp b/gdb/testsuite/gdb.dap/eof.exp
index 9c17725c0d0..26bad3eeb3f 100644
--- a/gdb/testsuite/gdb.dap/eof.exp
+++ b/gdb/testsuite/gdb.dap/eof.exp
@@ -30,6 +30,9 @@ if {[dap_initialize] == ""} {
     return
 }

+_dap_wait_for_event "initialized"
+return
+
 catch "close -i $gdb_spawn_id"
 catch "wait -i $gdb_spawn_id"
 unset gdb_spawn_id
...

That works fine (well, unless we run into PR31306).

Now, if we add another _dap_wait_for_event line like it, we run into a timeout
error followed by an eof error:
...
Running /data/vries/gdb/src/gdb/testsuite/gdb.dap/eof.exp ...
ERROR: timeout reading json header
    while executing
"error "timeout reading json header""
    invoked from within
"expect {
-i exp9 -timeout 10 
        -re "^Content-Length: (\[0-9\]+)\r\n" {
            set length $expect_out(1,string)
            exp_continue
        }
        -re "^(\[^\r\n\]+)\..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" NONE timeout reading json header
ERROR: tcl error sourcing /data/vries/gdb/src/gdb/testsuite/gdb.dap/eof.exp.
ERROR: eof reading json header
    while executing
"error "eof reading json header""
    ("uplevel" body line 2)
    invoked from within
"uplevel $error_sect"
    invoked from within
"remote_expect host 10 {
        -re "^Content-Length: (\[0-9\]+)\r\n" {
            set length $expect_out(1,string)
            exp_continue
        }
        -re "^(\[^\r\n\]+)\r\n" {..."
    ("uplevel" body line 1)
    invoked from within
"uplevel remote_expect host $tmt $expcode"
    invoked from within
"gdb_expect {
        -re "^Content-Length: (\[0-9\]+)\r\n" {
            set length $expect_out(1,string)
            exp_continue
        }
        -re "^(\[^\r\n\]+)\r\n" {
            # Any..."
    (procedure "_dap_read_json" line 3)
    invoked from within
"_dap_read_json"
    (procedure "_dap_wait_for_event" line 7)
    invoked from within
"_dap_wait_for_event "initialized""
    (file "/data/vries/gdb/src/gdb/testsuite/gdb.dap/eof.exp" line 34)
    invoked from within
"source /data/vries/gdb/src/gdb/testsuite/gdb.dap/eof.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source /data/vries/gdb/src/gdb/testsuite/gdb.dap/eof.exp"
    invoked from within
"catch "uplevel #0 source $test_file_name""
...

This is caused by using calls to error in these gdb_expect clauses in
_dap_read_json:
...
        timeout {
            error "timeout reading json header"
        }
        eof {
            error "eof reading json header"
        }
...

Proc gdb_expect uses dejagnu's remote_expect, which has some peculiar semantics
related to errors:
...
# remote_expect works basically the same as standard expect, but it             
# also takes care of getting the file descriptor from the specified             
# host and also calling the timeout/eof/default section if there is an          
# error on the expect call.                                                     
#                                                                               
...

So, the timeout error is caught by gdb_expect, and then printed, after which
the "timeout/eof/default" is called, which triggers the eof error.

I'm not sure how that proc is supposed to work, so this may or may not be
correct behaviour.

Either way, it's questionable whether it's a good idea to use error directly in
gdb_expect clauses.

I ran into this when trying to catch the timeout error:
...
catch {_dap_wait_for_event initialized}
...
which didn't work because the timeout error is caught and printed by
remote_expect.  Instead, we catch the eof error.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug testsuite/31382] [gdb/testsuite] 2 errors instead of 1 reported by _dap_read_json

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31382

--- Comment #1 from Tom de Vries <vries at gcc dot gnu.org> ---
Fixed by:
...
diff --git a/gdb/testsuite/lib/dap-support.exp
b/gdb/testsuite/lib/dap-support.exp
index 979dfa2cd73..e9595a85418 100644
--- a/gdb/testsuite/lib/dap-support.exp
+++ b/gdb/testsuite/lib/dap-support.exp
@@ -137,6 +137,8 @@ proc dap_send_request {command {obj {}}} {
 # "last_ton" will be set to the TON form of the result.
 proc _dap_read_json {} {
     set length ""
+    set seen_timeout 0
+    set seen_eof 0
     gdb_expect {
        -re "^Content-Length: (\[0-9\]+)\r\n" {
            set length $expect_out(1,string)
@@ -150,13 +152,20 @@ proc _dap_read_json {} {
            # Done.
        }
        timeout {
-           error "timeout reading json header"
+           set seen_timeout 1
        }
        eof {
-           error "eof reading json header"
+           set seen_eof 1
        }
     }

+    if {$seen_timeout} {
+       error "timeout reading json header"     
+    }
+    if {$seen_eof} {
+       error "eof reading json header"
+    }
+
     if {$length == ""} {
        error "didn't find content-length"
     }
...
which gives us:
...
ERROR: tcl error sourcing /data/vries/gdb/src/gdb/testsuite/gdb.dap/eof.exp.
ERROR: timeout reading json header
    while executing
"error "timeout reading json header"    "
    (procedure "_dap_read_json" line 26)
    invoked from within
"_dap_read_json"
    (procedure "_dap_wait_for_event" line 7)
    invoked from within
"_dap_wait_for_event "initialized""
    (file "/data/vries/gdb/src/gdb/testsuite/gdb.dap/eof.exp" line 34)
    invoked from within
"source /data/vries/gdb/src/gdb/testsuite/gdb.dap/eof.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source /data/vries/gdb/src/gdb/testsuite/gdb.dap/eof.exp"
    invoked from within
"catch "uplevel #0 source $test_file_name""
...

There's another gdb_expect in _dap_read_json that looks like it needs the same
treatment.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug testsuite/31382] [gdb/testsuite] 2 errors instead of 1 reported by _dap_read_json

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31382

--- Comment #2 from Tom de Vries <vries at gcc dot gnu.org> ---
https://sourceware.org/pipermail/gdb-patches/2024-February/206611.html

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug testsuite/31382] [gdb/testsuite] 2 errors instead of 1 reported by _dap_read_json

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31382

--- Comment #3 from Sourceware Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tom de Vries <vries@sourceware.org>:

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

commit 99eeecc8d276e5af745e48825d66efff693a7678
Author: Tom de Vries <tdevries@suse.de>
Date:   Wed Feb 21 14:25:31 2024 +0100

    [gdb/testsuite] Fix error handling in _dap_read_json

    In _dap_read_json we have a gdb_expect with clauses that generate errors:
    ...
            timeout {
                error "timeout reading json header"
            }
            eof {
                error "eof reading json header"
            }
    ...

    Proc gdb_expect uses dejagnu's remote_expect, which has some peculiar
    semantics related to errors:
    ...
     # remote_expect works basically the same as standard expect, but it
     # also takes care of getting the file descriptor from the specified
     # host and also calling the timeout/eof/default section if there is an
     # error on the expect call.
    .....

    When a timeout triggers, it generates a timeout error, which is reported by
    gdb_expect, after which it runs the timeout/eof/default clauses, which
    generates an eof error, which is reported by runtest.

    I think the intention here is to generate just a one error, a timeout
error.

    Fix this by postponing generating the error until after gdb_expect.

    Tested on x86_64-linux, by:
    - running all the DAP test-cases and observing no regressions, and
    - modifying the gdb.dap/eof.exp test-case to trigger a timeout error, and
      observing only a timeout error.

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

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug testsuite/31382] [gdb/testsuite] 2 errors instead of 1 reported by _dap_read_json

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31382

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |15.1

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug testsuite/31382] [gdb/testsuite] 2 errors instead of 1 reported by _dap_read_json

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31382

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #4 from Tom de Vries <vries at gcc dot gnu.org> ---
Fixed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.