[PATCH 0/4] DAP fixups

[PATCH 0/4] DAP fixups

[Gregory Anders]

This is my first contribution to GDB, so I appreciate your patience as I
navigate the contribution process :)

I've been experimenting with GDB's new DAP support and am so far very
happy. As an embedded developer, GDB is pretty much the only game in
town for remote target debugging, so the addition of DAP support was
very exciting.

Through the process of setting everything up between GDB and my DAP
client I ran into a couple of small issues. Some are downright bugs
(with easy fixes), and at least one is a question of semantics (the last
patch in the series).

Gregory Anders (4):
  dap: check for breakpoint source before unpacking
  dap: ignore unused keyword args in step_out
  dap: use breakpoint fullname to resolve source
  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/sources.py    | 16 +++++++++++-----
 3 files changed, 27 insertions(+), 17 deletions(-)

-- 
2.42.0

[PATCH 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 ++++++++++++--------
 1 file changed, 12 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
-- 
2.42.0

[PATCH 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 +-
 1 file changed, 1 insertion(+), 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))
 
-- 
2.42.0

[PATCH 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 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 and is non-zero, 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 could conflict with the _acutal_ existing file at "source.path").

Instead, only set "sourceReference" if the source file path does not
exist.
---

It is unclear exactly what the meaning of "sourceReference" is from the
specification, but at least two items support my interpretation here:

1. The wording in the specification for sourceReference mentions that
   the contents of the source must be retrieved through a 'source'
   request, _even if_ a path is specified. This suggests to me that
   sourceReference is meant to be used in cases where the debugger has
   access to source contents that the client does not (e.g. I could see
   something like this being used for "constructed" or "generated"
   sources, like disassembly).

2. Other debuggers (such as lldb-vscode and codelldb) do not set the
   sourceReference field at all for files which exist on disk. If this
   patch is accepted then GDB will match the behavior of those two.

And at least anecdotally, my testing with nvim-dap (my DAP client of
choice) shows that this behavior better matches the expectation of that
DAP client in particular.

 gdb/python/lib/gdb/dap/sources.py | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 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
 
 
-- 
2.42.0

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

[Tom Tromey]

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

Thank you for the patch.

> 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.

This one could probably use a test case.

I guess the failure mode is setting a breakpoint on a function without
debuginfo?  If so maybe the test is as simple as having
gdb/testsuite/gdb.dap/bt-nodebug.exp set a breakpoint on the
"no_debug_info" function, I suppose just before dap_shutdown.

Anyway I think the patch itself looks fine.

Tom

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

[Tom Tromey]

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

Gregory> Some DAP clients may send additional parameters in the stepOut command
Gregory> (e.g. "granularity") which are not used by GDB, but should nonetheless
Gregory> be accepted without error.

Nice catch, thank you.

I came up with the appended to ensure this remains true in the future.

I may land this one out of sequence -- we can accept some very small
patches without a copyright assignment -- and then the self-check patch
afterward.

Let me know what you think.

Tom

diff --git a/gdb/python/lib/gdb/dap/server.py b/gdb/python/lib/gdb/dap/server.py
index db7893a387b..d84bca5d1fc 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))

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

[Tom Tromey]

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

> 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).

Thank you.

How did you find this?  I'm wondering if a test case is possible.

Tom

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

[Gregory Anders]

On Fri, 01 Sep 2023 13:05 -0600, Tom Tromey wrote:
>This one could probably use a test case.
>
>I guess the failure mode is setting a breakpoint on a function without
>debuginfo?  If so maybe the test is as simple as having
>gdb/testsuite/gdb.dap/bt-nodebug.exp set a breakpoint on the
>"no_debug_info" function, I suppose just before dap_shutdown.

I actually stumbled on this on accident. I was starting GDB with a
.gdbinit file that sets a breakpoint by address (specifically, an
exception handler in the vector table, so something like b *0xf4). That
breakpoint had address info, but no source file or line information, and
would trigger an error which this patch corrects.

I'll look into writing up a test case.

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

[Tom Tromey]

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

Thank you for the patch.

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

On closer reading (this isn't the first time I've gotten the DAP spec
wrong) I think you are right.

The docs for the Source object say:

   * If the value > 0 the contents of the source must be retrieved through the
   * `source` request (even if a path is specified).

... which I think supports your view.

Gregory> +        if not os.path.exists(fullname):
Gregory> +            global _next_source

I suspect we should just remove all sourceReference handling from
sources.py, because if we can't find a full path, gdb isn't going to be
able to come up with the source anyway.

What do you think?

I thought the test suite might need some updates but I don't see any
references to sourceReference there.

Tom

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

[Gregory Anders]

On Fri, 01 Sep 2023 13:42 -0600, Tom Tromey wrote:
>I suspect we should just remove all sourceReference handling from
>sources.py, because if we can't find a full path, gdb isn't going to be
>able to come up with the source anyway.
>
>What do you think?

I agree. I avoided doing this originally to keep the change small, but I
think eliminating sourceReference completely (for the reason you
mentioned) is the better approach. It will keep the code simpler.

>I thought the test suite might need some updates but I don't see any
>references to sourceReference there.

There was one test case that was failing with the v1 patch
(sources.exp). I updated it in v2 (sent a few minutes ago).

Greg

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

[Gregory Anders]

On Fri, 01 Sep 2023 21:43 +0000, Gregory Anders wrote:
>On Fri, 01 Sep 2023 13:42 -0600, Tom Tromey wrote:
>>I suspect we should just remove all sourceReference handling from
>>sources.py, because if we can't find a full path, gdb isn't going to be
>>able to come up with the source anyway.
>>
>>What do you think?
>
>I agree. I avoided doing this originally to keep the change small, but I
>think eliminating sourceReference completely (for the reason you
>mentioned) is the better approach. It will keep the code simpler.
>
>>I thought the test suite might need some updates but I don't see any
>>references to sourceReference there.
>
>There was one test case that was failing with the v1 patch
>(sources.exp). I updated it in v2 (sent a few minutes ago).
>
>Greg

I checked what lldb-vscode does, and AFAICT they are doing the same
thing proposed in this patch: if the source file exists/is valid, they
do not populate sourceReference at all and do not maintain that entry
in any kind of source map.

They do, however, maintain a source map (with sourceReferences) for
things like function disassembly. So it might be worth keeping that
stuff around in GDB in case that feature is ever implemented in the
future.

Your call.

Greg

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

[Gregory Anders]

On Fri, 01 Sep 2023 13:08 -0600, Tom Tromey wrote:
>How did you find this?  I'm wondering if a test case is possible.

I use the 'substitute-path' option, and noticed that the path in
source.path uses the "compiled" source path (before translating with
'substitute-path') while loc.fullname contains the path after
translating.

Any hints or ideas on how to include a test case for that?

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

[Gregory Anders]

On Fri, 01 Sep 2023 13:07 -0600, Tom Tromey wrote:
>I came up with the appended to ensure this remains true in the future.
>
>I may land this one out of sequence -- we can accept some very small
>patches without a copyright assignment -- and then the self-check patch
>afterward.
>
>Let me know what you think.

LGTM, I included your suggestion in the v2 patchset I sent on Friday.

Greg