Bug 31354 - [gdb/dap] FAIL: gdb.dap/pause.exp: python command failed
Summary: [gdb/dap] FAIL: gdb.dap/pause.exp: python command failed
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: dap (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: 15.1
Assignee: Tom Tromey
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-02-08 08:41 UTC by Tom de Vries
Modified: 2024-02-27 16:57 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tom de Vries 2024-02-08 08:41:18 UTC
I found a problem in the gdb.dap/pause.exp test-case.

It generates a python file, but fails to use it.  This fixes it:
...
diff --git a/gdb/testsuite/gdb.dap/pause.exp b/gdb/testsuite/gdb.dap/pause.exp
index e1e0d957fc6..c1b12fc373c 100644
--- a/gdb/testsuite/gdb.dap/pause.exp
+++ b/gdb/testsuite/gdb.dap/pause.exp
@@ -130,7 +130,7 @@ dap_read_response cancel $cancel_id
 # does not continue the inferior) can be canceled.
 #
 
-write_file py "while True:\n  pass"
+set gdbfile [write_file py "while True:\n  pass"]
 set cont_id [dap_send_request evaluate \
                 [format {o expression [s "source %s"] context [s repl]} \
                      $gdbfile]]
...

Then we run into:
...
Running /home/vries/gdb/src/gdb/testsuite/gdb.dap/pause.exp ...
FAIL: gdb.dap/pause.exp: python command failed
WARNING: While evaluating expression in gdb_assert: key "message" not known in dictionary

                === gdb Summary ===

# of expected passes            18
# of unexpected failures        1
# of unresolved testcases       1
...

In more detail:
...
>>> {"seq": 12, "type": "request", "command": "evaluate", "arguments": {"expression": "source /home/vries/gdb/build/gdb/testsuite/outputs/gdb.dap/pause/pause.py", "context": "repl"}}
>>> {"seq": 13, "type": "request", "command": "cancel", "arguments": {"requestId": 12}}
Content-Length: 327^M
^M
{"request_seq": 12, "type": "response", "command": "evaluate", "body": {"result": "Traceback (most recent call last):\n  File \"/home/vries/gdb/build/gdb/testsuite/outputs/gdb.dap/pause/pause.py\", line 1, in <module>\n    while True:\n          ^^^^\nKeyboardInterrupt\n", "variablesReference": 0}, "success": true, "seq": 28}Content-Length: 88^M
^M
{"request_seq": 13, "type": "response", "command": "cancel", "success": true, "seq": 29}FAIL: gdb.dap/pause.exp: python command failed
WARNING: While evaluating expression in gdb_assert: key "message" not known in dictionary
UNRESOLVED: gdb.dap/pause.exp: python command canceled
...
Comment 1 Tom de Vries 2024-02-08 12:47:59 UTC
So, AFAICT the following happens:
- dap calls gdb.interrupt to cancel a request
- in gdbpy_interrupt, the quit flag is set
- at the end of gdbpy_interrupt, the no_python_sigint destructor is called,
  which calls restore_active_language, during which check_quit_flag is called,
  followed by set_quit_flag
- this calls gdbpy_set_quit_flag, which calls PyErr_SetInterrupt
  (I'm not sure if that's legal at this point.  It's not necessary to hold the GIL,
  but we're also in a PyEval_SaveThread context which has set thread state to null).
- the interrupt is caught while executing the python script using PyRun_SimpleFile,
  throwing a KeyboardInterrupt exception
- the KeyboardInterrupt propagates up to PyRun_SimpleFile, where the exception is
  dumped on the output 
- PyRun_SimpleFile returns -1 because of the KeyboardInterrupt exception, but
  that's silently ignored by python_run_simple_file

The KeyboardInterrupt is intended to propagate all the way up to the python runnable, which would capture the exception and hand it back to the dap thread, but instead it's captured by PyRun_SimpleFile.
Comment 2 Tom Tromey 2024-02-15 18:55:01 UTC
PyRun_SimpleFile doesn't let the caller access the error anyway.
I think we need to have gdb call something else here.
Comment 4 Sourceware Commits 2024-02-27 16:56:41 UTC
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

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

commit a207f6b3a384897be1dab081a0a9a206593029de
Author: Tom Tromey <tromey@adacore.com>
Date:   Thu Feb 15 13:14:43 2024 -0700

    Rewrite "python" command exception handling
    
    The "python" command (and the Python implementation of the gdb
    "source" command) does not handle Python exceptions in the same way as
    other gdb-facing Python code.  In particular, exceptions are turned
    into a generic error rather than being routed through
    gdbpy_handle_exception, which takes care of converting to 'quit' as
    appropriate.
    
    I think this was done this way because PyRun_SimpleFile and friends do
    not propagate the Python exception -- they simply indicate that one
    occurred.
    
    This patch reimplements these functions to respect the general gdb
    convention here.  As a bonus, some Windows-specific code can be
    removed, as can the _execute_file function.
    
    The bulk of this change is tweaking the test suite to match the new
    way that exceptions are displayed.  These changes are largely
    uninteresting.  However, it's worth pointing out the py-error.exp
    change.  Here, the failure changes because the test changes the host
    charset to something that isn't supported by Python.  This then
    results in a weird error in the new setup.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31354
    Acked-By: Tom de Vries <tdevries@suse.de>
    Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Comment 5 Sourceware Commits 2024-02-27 16:56:46 UTC
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

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

commit cfe51255b892962c25166cc0afd8911caf9e1e56
Author: Tom Tromey <tromey@adacore.com>
Date:   Fri Feb 16 10:39:46 2024 -0700

    Use the .py file in gdb.dap/pause.exp
    
    Tom de Vries pointed out that the gdb.dap/pause.exp test writes a
    Python file but then does not use it.  This patch corrects the
    oversight.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31354
    Reviewed-By: Tom de Vries <tdevries@suse.de>
Comment 6 Tom Tromey 2024-02-27 16:57:47 UTC
Fixed.