[PATCH v2 0/4] DAP fixups

[PATCH v2 0/4] DAP fixups

[Gregory Anders]

v2 includes test cases for the breakpoint and source location changes as
well as the change proposed by Tom Tromey to enforce **args parameter.

Gregory Anders (4):
  gdb/dap: check for breakpoint source before unpacking
  gdb/dap: ignore unused keyword args in step_out
  gdb/dap: use breakpoint fullname to resolve source
  gdb/dap: only include sourceReference if file path does not exist

 gdb/python/lib/gdb/dap/breakpoint.py | 26 +++++++++++++++-----------
 gdb/python/lib/gdb/dap/next.py       |  2 +-
 gdb/python/lib/gdb/dap/server.py     |  7 +++++++
 gdb/python/lib/gdb/dap/sources.py    | 16 +++++++++++-----
 gdb/testsuite/gdb.dap/bt-nodebug.exp |  7 +++++++
 gdb/testsuite/gdb.dap/sources.exp    |  9 +++++----
 6 files changed, 46 insertions(+), 21 deletions(-)

-- 
2.42.0

[PATCH v2 1/4] gdb/dap: check for breakpoint source before unpacking

[Gregory Anders]

Not all breakpoints have a source location. For example, a breakpoint
set on a raw address will have only the "address" field populated, but
"source" will be None, which leads to a RuntimeError when attempting to
unpack the filename and line number.

Before attempting to unpack the filename and line number from the
breakpoint, ensure that the source information is not None. Also
populate the source and line information separately from the
"instructionReference" field, so that breakpoints that include only an
address are still included.
---
 gdb/python/lib/gdb/dap/breakpoint.py | 20 ++++++++++++--------
 gdb/testsuite/gdb.dap/bt-nodebug.exp |  7 +++++++
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/breakpoint.py b/gdb/python/lib/gdb/dap/breakpoint.py
index 76ff129d..0ebb9597 100644
--- a/gdb/python/lib/gdb/dap/breakpoint.py
+++ b/gdb/python/lib/gdb/dap/breakpoint.py
@@ -106,14 +106,18 @@ def _breakpoint_descriptor(bp):
         # multiple locations.  See
         # https://github.com/microsoft/debug-adapter-protocol/issues/13
         loc = bp.locations[0]
-        (filename, line) = loc.source
-        result.update(
-            {
-                "source": make_source(filename, os.path.basename(filename)),
-                "line": line,
-                "instructionReference": hex(loc.address),
-            }
-        )
+        if loc.source:
+            (filename, line) = loc.source
+            result.update(
+                {
+                    "source": make_source(filename, os.path.basename(filename)),
+                    "line": line,
+                }
+            )
+
+        if loc.address:
+            result["instructionReference"] = hex(loc.address),
+
         path = loc.fullname
         if path is not None:
             result["source"]["path"] = path
diff --git a/gdb/testsuite/gdb.dap/bt-nodebug.exp b/gdb/testsuite/gdb.dap/bt-nodebug.exp
index e9726d2e..abd394ad 100644
--- a/gdb/testsuite/gdb.dap/bt-nodebug.exp
+++ b/gdb/testsuite/gdb.dap/bt-nodebug.exp
@@ -53,4 +53,11 @@ gdb_assert {[llength $frames] == 3} "three frames"
 gdb_assert {[dict get [lindex $frames 1] name] == "no_debug_info"} \
     "name of no-debug frame"
 
+# Breakpoint can be set without source file information
+set obj [dap_check_request_and_response "set breakpoint on no_debug_info" \
+	    setFunctionBreakpoints \
+	    {o breakpoints [a [o name [s no_debug_info]]]}]
+set breakpoints [dict get [lindex $obj 0] body breakpoints]
+gdb_assert {[dict exists [lindex $breakpoints 0] instructionReference]} "breakpoint has instructionReference"
+
 dap_shutdown
-- 
2.42.0

[PATCH v2 2/4] gdb/dap: ignore unused keyword args in step_out

[Gregory Anders]

Some DAP clients may send additional parameters in the stepOut command
(e.g. "granularity") which are not used by GDB, but should nonetheless
be accepted without error.
---
 gdb/python/lib/gdb/dap/next.py   | 2 +-
 gdb/python/lib/gdb/dap/server.py | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/gdb/python/lib/gdb/dap/next.py b/gdb/python/lib/gdb/dap/next.py
index 8b112770..5d49c871 100644
--- a/gdb/python/lib/gdb/dap/next.py
+++ b/gdb/python/lib/gdb/dap/next.py
@@ -69,7 +69,7 @@ def step_in(
 
 
 @request("stepOut")
-def step_out(*, threadId: int, singleThread: bool = False):
+def step_out(*, threadId: int, singleThread: bool = False, **args):
     send_gdb(lambda: _handle_thread_step(threadId, singleThread))
     send_gdb(ExecutionInvoker("finish", StopKinds.STEP))
 
diff --git a/gdb/python/lib/gdb/dap/server.py b/gdb/python/lib/gdb/dap/server.py
index db7893a3..d84bca5d 100644
--- a/gdb/python/lib/gdb/dap/server.py
+++ b/gdb/python/lib/gdb/dap/server.py
@@ -13,6 +13,7 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import inspect
 import json
 import queue
 import sys
@@ -165,6 +166,12 @@ def request(name):
 
     def wrap(func):
         global _commands
+        code = func.__code__
+        # We don't permit requests to have positional arguments.
+        assert code.co_posonlyargcount == 0
+        assert code.co_argcount == 0
+        # A request must have a **args parameter.
+        assert code.co_flags & inspect.CO_VARKEYWORDS
         # All requests must run in the DAP thread.
         # Also type-check the calls.
         func = in_dap_thread(type_check(func))
-- 
2.42.0

[PATCH v2 3/4] gdb/dap: use breakpoint fullname to resolve source

[Gregory Anders]

If the breakpoint has a fullname, use that as the source path when
resolving the breakpoint source information. This is consistent with
other callers of make_source which also use "fullname" if it exists (see
e.g. DAPFrameDecorator which returns the symtab's fullname).
---
 gdb/python/lib/gdb/dap/breakpoint.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/breakpoint.py b/gdb/python/lib/gdb/dap/breakpoint.py
index 0ebb9597..49efff10 100644
--- a/gdb/python/lib/gdb/dap/breakpoint.py
+++ b/gdb/python/lib/gdb/dap/breakpoint.py
@@ -108,6 +108,9 @@ def _breakpoint_descriptor(bp):
         loc = bp.locations[0]
         if loc.source:
             (filename, line) = loc.source
+            if loc.fullname is not None:
+                filename = loc.fullname
+
             result.update(
                 {
                     "source": make_source(filename, os.path.basename(filename)),
@@ -118,9 +121,6 @@ def _breakpoint_descriptor(bp):
         if loc.address:
             result["instructionReference"] = hex(loc.address),
 
-        path = loc.fullname
-        if path is not None:
-            result["source"]["path"] = path
     return result
 
 
-- 
2.42.0

[PATCH v2 4/4] gdb/dap: only include sourceReference if file path does not exist

[Gregory Anders]

According to the DAP specification if the "sourceReference" field is
included in a Source object, then the DAP client _must_ make a "source"
request to the debugger to retrieve file contents, even if the Source
object also includes path information.

If the Source's path field is a valid path that the DAP client is able
to read from the filesystem, having to make another request to the
debugger to get the file contents is wasteful and leads to incorrect
results (DAP clients will try to get the contents from the server and
display those contents as a file with the name in "source.path", but
this will conflict with the _acutal_ existing file at "source.path").

Instead, only set "sourceReference" if the source file path does not
exist.
---
 gdb/python/lib/gdb/dap/sources.py | 16 +++++++++++-----
 gdb/testsuite/gdb.dap/sources.exp |  9 +++++----
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/sources.py b/gdb/python/lib/gdb/dap/sources.py
index 7fa1ae43..00a70701 100644
--- a/gdb/python/lib/gdb/dap/sources.py
+++ b/gdb/python/lib/gdb/dap/sources.py
@@ -13,6 +13,8 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import os
+
 import gdb
 
 from .server import request, capability
@@ -41,16 +43,20 @@ def make_source(fullname, filename):
     if fullname in _source_map:
         result = _source_map[fullname]
     else:
-        global _next_source
         result = {
             "name": filename,
             "path": fullname,
-            "sourceReference": _next_source,
         }
+
+        if not os.path.exists(fullname):
+            global _next_source
+            result["sourceReference"] = _next_source
+
+            global _id_map
+            _id_map[_next_source] = result
+            _next_source += 1
+
         _source_map[fullname] = result
-        global _id_map
-        _id_map[_next_source] = result
-        _next_source += 1
     return result
 
 
diff --git a/gdb/testsuite/gdb.dap/sources.exp b/gdb/testsuite/gdb.dap/sources.exp
index a12c2089..f1b19acc 100644
--- a/gdb/testsuite/gdb.dap/sources.exp
+++ b/gdb/testsuite/gdb.dap/sources.exp
@@ -30,20 +30,21 @@ if {[dap_launch $testfile stop_at_main 1] == ""} {
 }
 
 set obj [dap_check_request_and_response loadedSources loadedSources]
-set ref 0
+set path ""
 foreach src [dict get [lindex $obj 0] body sources] {
     if {[file tail [dict get $src name]] == "sources.c"} {
-	set ref [dict get $src sourceReference]
+	set path [dict get $src path]
     }	
 }
 
-if {$ref == 0} {
+if {$path == ""} {
     fail "sources.c in loadedSources"
 } else {
     pass "sources.c in loadedSources"
 
     set obj [dap_check_request_and_response "get source" source \
-		 [format {o sourceReference [i %d]} $ref]]
+		 [format {o source [o path [s %s]] \
+			    sourceReference [i 0]} $path]]
     set text [dict get [lindex $obj 0] body content]
     gdb_assert {[string first "Distinguishing comment" $text] != -1}
 }
-- 
2.42.0

Re: [PATCH v2 0/4] DAP fixups

[Tom Tromey]

>>>>> "Gregory" == Gregory Anders via Gdb-patches <gdb-patches@sourceware.org> writes:

Gregory> v2 includes test cases for the breakpoint and source location changes as
Gregory> well as the change proposed by Tom Tromey to enforce **args parameter.

Gregory's copyright assignment came through.  I've applied this series
locally and tested it; I am going to push it now.

Gregory, if you are planning to do more work on gdb, let me know
off-list and we can get you set up with write-after-approval access.

Thank you for the patches.

Tom