[PATCH 0/6] Several DAP fixes for VSCode

[PATCH 0/6] Several DAP fixes for VSCode

[Tom Tromey]

Vladimir Makaev tried the gdb DAP implementation with VSCode and found
a few bugs.  This series fixes these.  It also adds a new DAP launch
parameter -- I thought I was going to need this, and while I didn't,
it still seemed like a good idea to add.

I'm not completely happy with this series.  See the "full paths" patch
for some discussion.  Essentially I think the FrameDecorator API is
not very well thought out, and once again this is an area where it
would be very nice if we could add new methods to the spec.

I wonder whether we should.  One difference between pretty-printers
and frame decorators is that my sense is that there are many more of
the former, so the smaller for the latter.

Another idea I had for that patch is to just change the base behavior
of FrameDecorator, based on a global flag -- that would only be set by
DAP.

---
Tom Tromey (6):
      Rename private member of FrameDecorator
      Refactor dap_launch
      Add "cwd" parameter to DAP launch request
      Full paths in DAP stackTrace responses
      Move DAP breakpoint event code to breakpoint.py
      Do not send "new breakpoint" event when breakpoint is set

 gdb/doc/gdb.texinfo                    |   8 +++
 gdb/python/lib/gdb/FrameDecorator.py   | 119 ++++++++++++++++++++-------------
 gdb/python/lib/gdb/dap/breakpoint.py   |  64 +++++++++++++++++-
 gdb/python/lib/gdb/dap/events.py       |  37 ----------
 gdb/python/lib/gdb/dap/launch.py       |  34 +++++-----
 gdb/python/lib/gdb/frames.py           |  14 +++-
 gdb/testsuite/gdb.dap/args-env.exp     |   2 +-
 gdb/testsuite/gdb.dap/basic-dap.exp    |  11 +--
 gdb/testsuite/gdb.dap/cwd.exp          |  40 +++++++++++
 gdb/testsuite/gdb.dap/stop-at-main.exp |   2 +-
 gdb/testsuite/lib/dap-support.exp      |  72 +++++++++++++-------
 11 files changed, 262 insertions(+), 141 deletions(-)
---
base-commit: 695776dc2f43c56dd2ae2f7036fb7cf74e19b46b
change-id: 20230725-dap-bt-path-32e910a3ee36

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/6] Rename private member of FrameDecorator

[Tom Tromey]

In Python, a member name starting with "__" is specially handled to
make it private to the class.  This patch ensures that this is done
for the private method of FrameDecorator.
---
 gdb/python/lib/gdb/FrameDecorator.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/python/lib/gdb/FrameDecorator.py b/gdb/python/lib/gdb/FrameDecorator.py
index 050cb934b7c..c4a7806e434 100644
--- a/gdb/python/lib/gdb/FrameDecorator.py
+++ b/gdb/python/lib/gdb/FrameDecorator.py
@@ -53,7 +53,7 @@ class FrameDecorator(object):
         self._base = base
 
     @staticmethod
-    def _is_limited_frame(frame):
+    def __is_limited_frame(frame):
         """Internal utility to determine if the frame is special or
         limited."""
         sal = frame.find_sal()
@@ -148,7 +148,7 @@ class FrameDecorator(object):
             return self._base.frame_args()
 
         frame = self.inferior_frame()
-        if self._is_limited_frame(frame):
+        if self.__is_limited_frame(frame):
             return None
 
         args = FrameVars(frame)
@@ -164,7 +164,7 @@ class FrameDecorator(object):
             return self._base.frame_locals()
 
         frame = self.inferior_frame()
-        if self._is_limited_frame(frame):
+        if self.__is_limited_frame(frame):
             return None
 
         args = FrameVars(frame)
@@ -179,7 +179,7 @@ class FrameDecorator(object):
             return self._base.line()
 
         frame = self.inferior_frame()
-        if self._is_limited_frame(frame):
+        if self.__is_limited_frame(frame):
             return None
 
         sal = frame.find_sal()

-- 
2.40.1

[PATCH 2/6] Refactor dap_launch

[Tom Tromey]

This patch refactors dap_launch to make it more extensible and also
easier to use.
---
 gdb/testsuite/gdb.dap/args-env.exp     |  2 +-
 gdb/testsuite/gdb.dap/stop-at-main.exp |  2 +-
 gdb/testsuite/lib/dap-support.exp      | 67 +++++++++++++++++++++-------------
 3 files changed, 43 insertions(+), 28 deletions(-)

diff --git a/gdb/testsuite/gdb.dap/args-env.exp b/gdb/testsuite/gdb.dap/args-env.exp
index 96fbb28d9ce..ae6cf2e66a6 100644
--- a/gdb/testsuite/gdb.dap/args-env.exp
+++ b/gdb/testsuite/gdb.dap/args-env.exp
@@ -25,7 +25,7 @@ if {[build_executable ${testfile}.exp $testfile] == -1} {
     return
 }
 
-if {[dap_launch $testfile {a "b c"} {{DEI something}}] == ""} {
+if {[dap_launch $testfile arguments {a "b c"} env {{DEI something}}] == ""} {
     return
 }
 
diff --git a/gdb/testsuite/gdb.dap/stop-at-main.exp b/gdb/testsuite/gdb.dap/stop-at-main.exp
index 80a9b81e152..3f22f4a0154 100644
--- a/gdb/testsuite/gdb.dap/stop-at-main.exp
+++ b/gdb/testsuite/gdb.dap/stop-at-main.exp
@@ -25,7 +25,7 @@ if {[build_executable ${testfile}.exp $testfile $srcfile] == -1} {
     return
 }
 
-if {[dap_launch $testfile {} {} 1] == ""} {
+if {[dap_launch $testfile stop_at_main 1] == ""} {
     return
 }
 
diff --git a/gdb/testsuite/lib/dap-support.exp b/gdb/testsuite/lib/dap-support.exp
index e3750e1d016..4a1a288e7ae 100644
--- a/gdb/testsuite/lib/dap-support.exp
+++ b/gdb/testsuite/lib/dap-support.exp
@@ -239,40 +239,55 @@ proc _dap_initialize {name} {
 # Start gdb, send a DAP initialize request, and then a launch request
 # specifying FILE as the program to use for the inferior.  Returns the
 # empty string on failure, or the response object from the launch
-# request.  If specified, ARGS is a list of command-line arguments,
-# and ENV is a list of pairs of the form {VAR VALUE} that is used to
-# populate the inferior's environment.  After this is called, gdb will
-# be ready to accept breakpoint requests.  If STOP_AT_MAIN is nonzero,
-# pass "stopAtBeginningOfMainSubprogram" to the launch request.
-proc dap_launch {file {args {}} {env {}} {stop_at_main 0}} {
+# request.  If specified, ARGS is a dictionary of key-value pairs,
+# each passed to the launch request.  Valid keys are:
+# * arguments - value is a list of strings passed as command-line
+#   arguments to the inferior
+# * env - value is a list of pairs of the form {VAR VALUE} that is
+#   used to populate the inferior's environment.
+# * stop_at_main - value is ignored, the presence of this means that
+#   "stopAtBeginningOfMainSubprogram" will be passed to the launch
+#   request.
+#
+# After this proc is called, gdb will be ready to accept breakpoint
+# requests.
+proc dap_launch {file {args {}}} {
     if {[_dap_initialize "startup - initialize"] == ""} {
 	return ""
     }
     set params "o program"
     append params " [format {[%s]} [list s [standard_output_file $file]]]"
 
-    if {[llength $args] > 0} {
-	append params " args"
-	set arglist ""
-	foreach arg $args {
-	    append arglist " \[s [list $arg]\]"
-	}
-	append params " \[a $arglist\]"
-    }
+    foreach {key value} $args {
+	switch -exact -- $key {
+	    arguments {
+		append params " args"
+		set arglist ""
+		foreach arg $value {
+		    append arglist " \[s [list $arg]\]"
+		}
+		append params " \[a $arglist\]"
+	    }
 
-    if {[llength $env] > 0} {
-	append params " env"
-	set envlist ""
-	foreach pair $env {
-	    lassign $pair var value
-	    append envlist " $var"
-	    append envlist " [format {[%s]} [list s $value]]"
-	}
-	append params " \[o $envlist\]"
-    }
+	    env {
+		append params " env"
+		set envlist ""
+		foreach pair $value {
+		    lassign $pair var value
+		    append envlist " $var"
+		    append envlist " [format {[%s]} [list s $value]]"
+		}
+		append params " \[o $envlist\]"
+	    }
+
+	    stop_at_main {
+		append params { stopAtBeginningOfMainSubprogram [l true]}
+	    }
 
-    if {$stop_at_main} {
-	append params { stopAtBeginningOfMainSubprogram [l true]}
+	    default {
+		error "unrecognized parameter $key"
+	    }
+	}
     }
 
     return [dap_check_request_and_response "startup - launch" launch $params]

-- 
2.40.1

[PATCH 3/6] Add "cwd" parameter to DAP launch request

[Tom Tromey]

This adds the "cwd" parameter to the DAP launch request.

This came up here:
    https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/issues/90
... and seemed like a good idea.
---
 gdb/doc/gdb.texinfo               |  8 ++++++++
 gdb/python/lib/gdb/dap/launch.py  | 34 ++++++++++++++++-----------------
 gdb/testsuite/gdb.dap/cwd.exp     | 40 +++++++++++++++++++++++++++++++++++++++
 gdb/testsuite/lib/dap-support.exp |  5 +++++
 4 files changed, 70 insertions(+), 17 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 1463f501768..bd11f0a4475 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -39069,6 +39069,14 @@ If provided, this should be an array of strings.  These strings are
 provided as command-line arguments to the inferior, as if by
 @code{set args}.  @xref{Arguments}.
 
+@item cwd
+If provided, this should be a string.  @value{GDBN} will change its
+working directory to this directory, as if by the @code{cd} command
+(@pxref{Working Directory}).  The launched program will inherit this
+as its working directory.  Note that change of directory happens
+before the @code{program} parameter is processed.  This will affect
+the result if @code{program} is a relative filename.
+
 @item env
 If provided, this should be an object.  Each key of the object will be
 used as the name of an environment variable; each value must be a
diff --git a/gdb/python/lib/gdb/dap/launch.py b/gdb/python/lib/gdb/dap/launch.py
index c3c09bc3dd0..d13037fa476 100644
--- a/gdb/python/lib/gdb/dap/launch.py
+++ b/gdb/python/lib/gdb/dap/launch.py
@@ -23,12 +23,22 @@ from .server import request, capability
 from .startup import send_gdb, send_gdb_with_response, in_gdb_thread, exec_and_log
 
 
+# The program being launched, or None.  This should only be access
+# from the DAP thread.
 _program = None
 
 
 @in_gdb_thread
-def _set_args_env(args, env):
+def _launch_setup(program, cwd, args, env, stopAtBeginningOfMainSubprogram):
+    if cwd is not None:
+        exec_and_log("cd " + cwd)
+    if program is not None:
+        exec_and_log("file " + program)
     inf = gdb.selected_inferior()
+    if stopAtBeginningOfMainSubprogram:
+        main = inf.main_name
+        if main is not None:
+            exec_and_log("tbreak " + main)
     inf.arguments = args
     if env is not None:
         inf.clear_env()
@@ -36,14 +46,6 @@ def _set_args_env(args, env):
             inf.set_env(name, value)
 
 
-@in_gdb_thread
-def _break_at_main():
-    inf = gdb.selected_inferior()
-    main = inf.main_name
-    if main is not None:
-        exec_and_log("tbreak " + main)
-
-
 # Any parameters here are necessarily extensions -- DAP requires this
 # from implementations.  Any additions or changes here should be
 # documented in the gdb manual.
@@ -51,19 +53,17 @@ def _break_at_main():
 def launch(
     *,
     program: Optional[str] = None,
+    cwd: Optional[str] = None,
     args: Sequence[str] = (),
     env: Optional[Mapping[str, str]] = None,
     stopAtBeginningOfMainSubprogram: bool = False,
     **extra,
 ):
-    if program is not None:
-        global _program
-        _program = program
-        send_gdb("file " + _program)
-    if stopAtBeginningOfMainSubprogram:
-        send_gdb(_break_at_main)
-    if len(args) > 0 or env is not None:
-        send_gdb(lambda: _set_args_env(args, env))
+    global _program
+    _program = program
+    send_gdb(
+        lambda: _launch_setup(program, cwd, args, env, stopAtBeginningOfMainSubprogram)
+    )
 
 
 @request("attach")
diff --git a/gdb/testsuite/gdb.dap/cwd.exp b/gdb/testsuite/gdb.dap/cwd.exp
new file mode 100644
index 00000000000..394fe5ada83
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/cwd.exp
@@ -0,0 +1,40 @@
+# Copyright 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test the cwd extension.
+
+require allow_dap_tests {!is_remote host}
+
+load_lib dap-support.exp
+
+standard_testfile attach.c
+
+if {[build_executable ${testfile}.exp $testfile $srcfile] == -1} {
+    return
+}
+
+# Starting the inferior will fail if the change of cwd does not work.
+set the_dir [file dirname $testfile]
+set the_file [file tail $testfile]
+if {[dap_launch $the_file cwd $the_dir stop_at_main 1] == ""} {
+    return
+}
+
+dap_check_request_and_response "start inferior" configurationDone
+# We didn't explicitly set a breakpoint, so if we hit one, it worked.
+dap_wait_for_event_and_check "stopped at function breakpoint" stopped \
+    "body reason" breakpoint
+
+dap_shutdown
diff --git a/gdb/testsuite/lib/dap-support.exp b/gdb/testsuite/lib/dap-support.exp
index 4a1a288e7ae..657ad7b29bc 100644
--- a/gdb/testsuite/lib/dap-support.exp
+++ b/gdb/testsuite/lib/dap-support.exp
@@ -248,6 +248,7 @@ proc _dap_initialize {name} {
 # * stop_at_main - value is ignored, the presence of this means that
 #   "stopAtBeginningOfMainSubprogram" will be passed to the launch
 #   request.
+# * cwd - value is the working directory to use.
 #
 # After this proc is called, gdb will be ready to accept breakpoint
 # requests.
@@ -284,6 +285,10 @@ proc dap_launch {file {args {}}} {
 		append params { stopAtBeginningOfMainSubprogram [l true]}
 	    }
 
+	    cwd {
+		append envlist " cwd [format {[%s]} [list s $value]]"
+	    }
+
 	    default {
 		error "unrecognized parameter $key"
 	    }

-- 
2.40.1

[PATCH 4/6] Full paths in DAP stackTrace responses

[Tom Tromey]

Vladimir Makaev noticed that, in some cases, a DAP stackTrace response
would include a relative path name for the "path" component.

This patch changes the frame decorator code to add a new DAP-specific
decorator, and changes the DAP entry point to frame filters to use it.
This decorator prefers the symtab's full name, and does not fall back
to the solib's name.

I'm not entirely happy with this patch, because if a user frame filter
uses FrameDecorator, it may still do the wrong thing.  It would be
better to have frame filters return symtab-like objects instead, or to
have a separate method to return the full path to the source file.

I also tend to think that the solib fallback behavior of
FrameDecorator is a mistake.  If this is ever needed, it seems to me
that it should be a separate method.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30665
---
 gdb/python/lib/gdb/FrameDecorator.py | 111 +++++++++++++++++++++--------------
 gdb/python/lib/gdb/frames.py         |  14 ++++-
 2 files changed, 78 insertions(+), 47 deletions(-)

diff --git a/gdb/python/lib/gdb/FrameDecorator.py b/gdb/python/lib/gdb/FrameDecorator.py
index c4a7806e434..22d5fa4d0a9 100644
--- a/gdb/python/lib/gdb/FrameDecorator.py
+++ b/gdb/python/lib/gdb/FrameDecorator.py
@@ -16,34 +16,8 @@
 import gdb
 
 
-class FrameDecorator(object):
-    """Basic implementation of a Frame Decorator"""
-
-    """ This base frame decorator decorates a frame or another frame
-    decorator, and provides convenience methods.  If this object is
-    wrapping a frame decorator, defer to that wrapped object's method
-    if it has one.  This allows for frame decorators that have
-    sub-classed FrameDecorator object, but also wrap other frame
-    decorators on the same frame to correctly execute.
-
-    E.g
-
-    If the result of frame filters running means we have one gdb.Frame
-    wrapped by multiple frame decorators, all sub-classed from
-    FrameDecorator, the resulting hierarchy will be:
-
-    Decorator1
-      -- (wraps) Decorator2
-        -- (wraps) FrameDecorator
-          -- (wraps) gdb.Frame
-
-    In this case we have two frame decorators, both of which are
-    sub-classed from FrameDecorator.  If Decorator1 just overrides the
-    'function' method, then all of the other methods are carried out
-    by the super-class FrameDecorator.  But Decorator2 may have
-    overriden other methods, so FrameDecorator will look at the
-    'base' parameter and defer to that class's methods.  And so on,
-    down the chain."""
+class _FrameDecoratorBase(object):
+    """Base class of frame decorators."""
 
     # 'base' can refer to a gdb.Frame or another frame decorator.  In
     # the latter case, the child class will have called the super
@@ -122,22 +96,6 @@ class FrameDecorator(object):
         frame = self.inferior_frame()
         return frame.pc()
 
-    def filename(self):
-        """Return the filename associated with this frame, detecting
-        and returning the appropriate library name is this is a shared
-        library."""
-
-        if hasattr(self._base, "filename"):
-            return self._base.filename()
-
-        frame = self.inferior_frame()
-        sal = frame.find_sal()
-        if not sal.symtab or not sal.symtab.filename:
-            pc = frame.pc()
-            return gdb.solib_name(pc)
-        else:
-            return sal.symtab.filename
-
     def frame_args(self):
         """Return an iterable of frame arguments for this frame, if
         any.  The iterable object contains objects conforming with the
@@ -198,6 +156,71 @@ class FrameDecorator(object):
         return self._base
 
 
+class FrameDecorator(_FrameDecoratorBase):
+    """Basic implementation of a Frame Decorator"""
+
+    """ This base frame decorator decorates a frame or another frame
+    decorator, and provides convenience methods.  If this object is
+    wrapping a frame decorator, defer to that wrapped object's method
+    if it has one.  This allows for frame decorators that have
+    sub-classed FrameDecorator object, but also wrap other frame
+    decorators on the same frame to correctly execute.
+
+    E.g
+
+    If the result of frame filters running means we have one gdb.Frame
+    wrapped by multiple frame decorators, all sub-classed from
+    FrameDecorator, the resulting hierarchy will be:
+
+    Decorator1
+      -- (wraps) Decorator2
+        -- (wraps) FrameDecorator
+          -- (wraps) gdb.Frame
+
+    In this case we have two frame decorators, both of which are
+    sub-classed from FrameDecorator.  If Decorator1 just overrides the
+    'function' method, then all of the other methods are carried out
+    by the super-class FrameDecorator.  But Decorator2 may have
+    overriden other methods, so FrameDecorator will look at the
+    'base' parameter and defer to that class's methods.  And so on,
+    down the chain."""
+
+    def filename(self):
+        """Return the filename associated with this frame, detecting
+        and returning the appropriate library name is this is a shared
+        library."""
+
+        if hasattr(self._base, "filename"):
+            return self._base.filename()
+
+        frame = self.inferior_frame()
+        sal = frame.find_sal()
+        if not sal.symtab or not sal.symtab.filename:
+            pc = frame.pc()
+            return gdb.solib_name(pc)
+        else:
+            return sal.symtab.filename
+
+
+class DAPFrameDecorator(_FrameDecoratorBase):
+    """Like FrameDecorator, but has slightly different results
+    for the "filename" method."""
+
+    def filename(self):
+        """Return the filename associated with this frame, detecting
+        and returning the appropriate library name is this is a shared
+        library."""
+
+        if hasattr(self._base, "filename"):
+            return self._base.filename()
+
+        frame = self.inferior_frame()
+        sal = frame.find_sal()
+        if sal.symtab is not None:
+            return sal.symtab.fullname()
+        return None
+
+
 class SymValueWrapper(object):
     """A container class conforming to the Symbol/Value interface
     which holds frame locals or frame arguments."""
diff --git a/gdb/python/lib/gdb/frames.py b/gdb/python/lib/gdb/frames.py
index cff3ba07704..104ff4bb203 100644
--- a/gdb/python/lib/gdb/frames.py
+++ b/gdb/python/lib/gdb/frames.py
@@ -18,7 +18,7 @@
 
 import gdb
 from gdb.FrameIterator import FrameIterator
-from gdb.FrameDecorator import FrameDecorator
+from gdb.FrameDecorator import FrameDecorator, DAPFrameDecorator
 import itertools
 import collections
 
@@ -172,7 +172,11 @@ def _frame_iterator(frame, frame_low, frame_high, always):
 
     # Apply a basic frame decorator to all gdb.Frames.  This unifies
     # the interface.
-    frame_iterator = map(FrameDecorator, frame_iterator)
+    if always:
+        decorator = DAPFrameDecorator
+    else:
+        decorator = FrameDecorator
+    frame_iterator = map(decorator, frame_iterator)
 
     for ff in sorted_list:
         frame_iterator = ff.filter(frame_iterator)
@@ -214,7 +218,11 @@ def frame_iterator(frame, frame_low, frame_high):
     """Helper function that will execute the chain of frame filters.
     Each filter is executed in priority order.  After the execution
     completes, slice the iterator to frame_low - frame_high range.  An
-    iterator is always returned.
+    iterator is always returned.  The iterator will always yield
+    frame decorator objects, but note that these decorators have
+    slightly different semantics from the ordinary ones: they will
+    always return a fully-qualified 'filename' (if possible) and will
+    never substitute the objfile name.
 
     Arguments:
         frame: The initial frame.

-- 
2.40.1

[PATCH 5/6] Move DAP breakpoint event code to breakpoint.py

[Tom Tromey]

A subsequent patch will add the ability to suppress breakpoint events
to DAP.  My first attempt at this ended up with recurse imports, cause
Python failures.  So, this patch moves all the DAP breakpoint event
code to breakpoint.py in preparation for the change.

I've renamed breakpoint_descriptor here as well, because it can now be
private to breakpoint.py.
---
 gdb/python/lib/gdb/dap/breakpoint.py | 42 ++++++++++++++++++++++++++++++++++--
 gdb/python/lib/gdb/dap/events.py     | 37 -------------------------------
 2 files changed, 40 insertions(+), 39 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/breakpoint.py b/gdb/python/lib/gdb/dap/breakpoint.py
index 27745ebfd2c..4a1c98efd87 100644
--- a/gdb/python/lib/gdb/dap/breakpoint.py
+++ b/gdb/python/lib/gdb/dap/breakpoint.py
@@ -25,6 +25,44 @@ from .startup import send_gdb_with_response, in_gdb_thread, log_stack
 from .typecheck import type_check
 
 
+@in_gdb_thread
+def _bp_modified(event):
+    send_event(
+        "breakpoint",
+        {
+            "reason": "changed",
+            "breakpoint": _breakpoint_descriptor(event),
+        },
+    )
+
+
+@in_gdb_thread
+def _bp_created(event):
+    send_event(
+        "breakpoint",
+        {
+            "reason": "new",
+            "breakpoint": _breakpoint_descriptor(event),
+        },
+    )
+
+
+@in_gdb_thread
+def _bp_deleted(event):
+    send_event(
+        "breakpoint",
+        {
+            "reason": "removed",
+            "breakpoint": _breakpoint_descriptor(event),
+        },
+    )
+
+
+gdb.events.breakpoint_created.connect(_bp_created)
+gdb.events.breakpoint_modified.connect(_bp_modified)
+gdb.events.breakpoint_deleted.connect(_bp_deleted)
+
+
 # Map from the breakpoint "kind" (like "function") to a second map, of
 # breakpoints of that type.  The second map uses the breakpoint spec
 # as a key, and the gdb.Breakpoint itself as a value.  This is used to
@@ -34,7 +72,7 @@ breakpoint_map = {}
 
 
 @in_gdb_thread
-def breakpoint_descriptor(bp):
+def _breakpoint_descriptor(bp):
     "Return the Breakpoint object descriptor given a gdb Breakpoint."
     result = {
         "id": bp.number,
@@ -115,7 +153,7 @@ def _set_breakpoints_callback(kind, specs, creator):
 
             # Reaching this spot means success.
             breakpoint_map[kind][keyspec] = bp
-            result.append(breakpoint_descriptor(bp))
+            result.append(_breakpoint_descriptor(bp))
         # Exceptions other than gdb.error are possible here.
         except Exception as e:
             log_stack()
diff --git a/gdb/python/lib/gdb/dap/events.py b/gdb/python/lib/gdb/dap/events.py
index c1631442746..50ab038e91d 100644
--- a/gdb/python/lib/gdb/dap/events.py
+++ b/gdb/python/lib/gdb/dap/events.py
@@ -18,7 +18,6 @@ import gdb
 
 from .server import send_event
 from .startup import in_gdb_thread, Invoker, log
-from .breakpoint import breakpoint_descriptor
 from .modules import is_module, make_module
 
 
@@ -35,39 +34,6 @@ def _on_exit(event):
     )
 
 
-@in_gdb_thread
-def _bp_modified(event):
-    send_event(
-        "breakpoint",
-        {
-            "reason": "changed",
-            "breakpoint": breakpoint_descriptor(event),
-        },
-    )
-
-
-@in_gdb_thread
-def _bp_created(event):
-    send_event(
-        "breakpoint",
-        {
-            "reason": "new",
-            "breakpoint": breakpoint_descriptor(event),
-        },
-    )
-
-
-@in_gdb_thread
-def _bp_deleted(event):
-    send_event(
-        "breakpoint",
-        {
-            "reason": "removed",
-            "breakpoint": breakpoint_descriptor(event),
-        },
-    )
-
-
 @in_gdb_thread
 def _new_thread(event):
     send_event(
@@ -169,9 +135,6 @@ def _on_stop(event):
 
 gdb.events.stop.connect(_on_stop)
 gdb.events.exited.connect(_on_exit)
-gdb.events.breakpoint_created.connect(_bp_created)
-gdb.events.breakpoint_modified.connect(_bp_modified)
-gdb.events.breakpoint_deleted.connect(_bp_deleted)
 gdb.events.new_thread.connect(_new_thread)
 gdb.events.cont.connect(_cont)
 gdb.events.new_objfile.connect(_new_objfile)

-- 
2.40.1

[PATCH 6/6] Do not send "new breakpoint" event when breakpoint is set

[Tom Tromey]

When the DAP client sets a breakpoint, gdb currently sends a "new
breakpoint" event.  However, Vladimir Makaev discovered that this
causes VSCode to think there are two breakpoints.

This patch changes gdb to suppress the event in this case.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30678
---
 gdb/python/lib/gdb/dap/breakpoint.py | 36 ++++++++++++++++++++++++++++--------
 gdb/testsuite/gdb.dap/basic-dap.exp  | 11 ++++++-----
 2 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/breakpoint.py b/gdb/python/lib/gdb/dap/breakpoint.py
index 4a1c98efd87..e612c512a89 100644
--- a/gdb/python/lib/gdb/dap/breakpoint.py
+++ b/gdb/python/lib/gdb/dap/breakpoint.py
@@ -17,6 +17,8 @@ import gdb
 import os
 import re
 
+from contextlib import contextmanager
+
 # These are deprecated in 3.9, but required in older versions.
 from typing import Optional, Sequence
 
@@ -36,15 +38,32 @@ def _bp_modified(event):
     )
 
 
+# True when suppressing new breakpoint events.
+_suppress_bp = False
+
+
+@contextmanager
+def suppress_new_breakpoint_event():
+    """Return a new context manager that suppresses new breakpoint events."""
+    global _suppress_bp
+    _suppress_bp = True
+    try:
+        yield None
+    finally:
+        _suppress_bp = False
+
+
 @in_gdb_thread
 def _bp_created(event):
-    send_event(
-        "breakpoint",
-        {
-            "reason": "new",
-            "breakpoint": _breakpoint_descriptor(event),
-        },
-    )
+    global _suppress_bp
+    if not _suppress_bp:
+        send_event(
+            "breakpoint",
+            {
+                "reason": "new",
+                "breakpoint": _breakpoint_descriptor(event),
+            },
+        )
 
 
 @in_gdb_thread
@@ -141,7 +160,8 @@ def _set_breakpoints_callback(kind, specs, creator):
             if keyspec in saved_map:
                 bp = saved_map.pop(keyspec)
             else:
-                bp = creator(**spec)
+                with suppress_new_breakpoint_event():
+                    bp = creator(**spec)
 
             bp.condition = condition
             if hit_condition is None:
diff --git a/gdb/testsuite/gdb.dap/basic-dap.exp b/gdb/testsuite/gdb.dap/basic-dap.exp
index ef3c535f6a2..c4a1698beda 100644
--- a/gdb/testsuite/gdb.dap/basic-dap.exp
+++ b/gdb/testsuite/gdb.dap/basic-dap.exp
@@ -54,7 +54,7 @@ set obj [dap_check_request_and_response "set breakpoint by line number" \
 set line_bpno [dap_get_breakpoint_number $obj]
 
 # Check the new breakpoint event.
-set ok 0
+set ok 1
 foreach d [lindex $obj 1] {
     if {[dict get $d type] != "event"
 	|| [dict get $d event] != "breakpoint"} {
@@ -62,13 +62,14 @@ foreach d [lindex $obj 1] {
     }
     if {[dict get $d body reason] == "new"
 	&& [dict get $d body breakpoint verified] == "true"} {
-	set ok 1
-	pass "check new breakpoint event"
+	set ok 0
 	break
     }
 }
-if {!$ok} {
-    fail "check new breakpoint event"
+if {$ok} {
+    pass "check lack of new breakpoint event"
+} else {
+    fail "check lack of new breakpoint event"
 }
 
 # Check that there are breakpoint locations on each line between FIRST

-- 
2.40.1

Re: [PATCH 3/6] Add "cwd" parameter to DAP launch request

[Eli Zaretskii]

> Date: Tue, 25 Jul 2023 07:49:40 -0600
> From: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org>
> 
> This adds the "cwd" parameter to the DAP launch request.
> 
> This came up here:
>     https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/issues/90
> ... and seemed like a good idea.
> ---
>  gdb/doc/gdb.texinfo               |  8 ++++++++
>  gdb/python/lib/gdb/dap/launch.py  | 34 ++++++++++++++++-----------------
>  gdb/testsuite/gdb.dap/cwd.exp     | 40 +++++++++++++++++++++++++++++++++++++++
>  gdb/testsuite/lib/dap-support.exp |  5 +++++
>  4 files changed, 70 insertions(+), 17 deletions(-)

OK for the documentation part, thanks.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 1/6] Rename private member of FrameDecorator

[Lancelot SIX]

On Tue, Jul 25, 2023 at 07:49:38AM -0600, Tom Tromey via Gdb-patches wrote:
> In Python, a member name starting with "__" is specially handled to
> make it private to the class.  This patch ensures that this is done
> for the private method of FrameDecorator.

Hi,

My understanding is that the "__" prefix is more a syntactic sugar to
avoid name clashes with subclasses than making it strictly private[1].

In this case, the method still exists as
FrameDecorator._FrameDecorator__is_limited_frame, which can be accessed
from outside of FrameDecorator.  I agree it can be less intuitive to
access this method from a subclass as one would need to call
self._FrameDecorator__is_limited_frame, which is probably what you are
looking for.

I do not object to this change, I am just noting that nothing is really
private in Python, and it seems to me that the commit message promises
otherwise.

Best,
Lancelot.

[1] https://docs.python.org/3/tutorial/classes.html#private-variables

> ---
>  gdb/python/lib/gdb/FrameDecorator.py | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/python/lib/gdb/FrameDecorator.py b/gdb/python/lib/gdb/FrameDecorator.py
> index 050cb934b7c..c4a7806e434 100644
> --- a/gdb/python/lib/gdb/FrameDecorator.py
> +++ b/gdb/python/lib/gdb/FrameDecorator.py
> @@ -53,7 +53,7 @@ class FrameDecorator(object):
>          self._base = base
>  
>      @staticmethod
> -    def _is_limited_frame(frame):
> +    def __is_limited_frame(frame):
>          """Internal utility to determine if the frame is special or
>          limited."""
>          sal = frame.find_sal()
> @@ -148,7 +148,7 @@ class FrameDecorator(object):
>              return self._base.frame_args()
>  
>          frame = self.inferior_frame()
> -        if self._is_limited_frame(frame):
> +        if self.__is_limited_frame(frame):
>              return None
>  
>          args = FrameVars(frame)
> @@ -164,7 +164,7 @@ class FrameDecorator(object):
>              return self._base.frame_locals()
>  
>          frame = self.inferior_frame()
> -        if self._is_limited_frame(frame):
> +        if self.__is_limited_frame(frame):
>              return None
>  
>          args = FrameVars(frame)
> @@ -179,7 +179,7 @@ class FrameDecorator(object):
>              return self._base.line()
>  
>          frame = self.inferior_frame()
> -        if self._is_limited_frame(frame):
> +        if self.__is_limited_frame(frame):
>              return None
>  
>          sal = frame.find_sal()
> 
> -- 
> 2.40.1
> 

Re: [PATCH 4/6] Full paths in DAP stackTrace responses

[Lancelot SIX]

Hi Tom,

> +class FrameDecorator(_FrameDecoratorBase):
> +    """Basic implementation of a Frame Decorator"""
> +

I can see that this pre-existed your patch, but is it expected to have
two docstrings instead of one with empty lines to separate paragraphs?

> +    """ This base frame decorator decorates a frame or another frame
> +    decorator, and provides convenience methods.  If this object is
> +    wrapping a frame decorator, defer to that wrapped object's method
> +    if it has one.  This allows for frame decorators that have
> +    sub-classed FrameDecorator object, but also wrap other frame
> +    decorators on the same frame to correctly execute.
> +

> @@ -172,7 +172,11 @@ def _frame_iterator(frame, frame_low, frame_high, always):
>  
>      # Apply a basic frame decorator to all gdb.Frames.  This unifies
>      # the interface.
> -    frame_iterator = map(FrameDecorator, frame_iterator)
> +    if always:
> +        decorator = DAPFrameDecorator
> +    else:
> +        decorator = FrameDecorator
> +    frame_iterator = map(decorator, frame_iterator)

The purpose of always (always return an iterator) seems orthogonal to
the way `filename()` behaves.  I find this confusing.  Should the
parameter be renamed / a new parameter added?

Best,
Lancelot.

Re: [PATCH 1/6] Rename private member of FrameDecorator

[Tom Tromey]

>>>>> "Lancelot" == Lancelot SIX <lsix@lancelotsix.com> writes:

Lancelot> I do not object to this change, I am just noting that nothing is really
Lancelot> private in Python, and it seems to me that the commit message promises
Lancelot> otherwise.

I've reworded it to:

    In Python, a member name starting with "__" is specially handled to
    make it "more private" to the class -- it isn't truly private, but it
    is renamed to make it less likely to be reused by mistake.

Tom

Re: [PATCH 1/6] Rename private member of FrameDecorator

[Lancelot SIX]

On Mon, Jul 31, 2023 at 08:30:05AM -0600, Tom Tromey wrote:
> >>>>> "Lancelot" == Lancelot SIX <lsix@lancelotsix.com> writes:
> 
> Lancelot> I do not object to this change, I am just noting that nothing is really
> Lancelot> private in Python, and it seems to me that the commit message promises
> Lancelot> otherwise.
> 
> I've reworded it to:
> 
>     In Python, a member name starting with "__" is specially handled to
>     make it "more private" to the class -- it isn't truly private, but it
>     is renamed to make it less likely to be reused by mistake.
> 
> Tom

Hi,

FWIW, this looks good to me.

Best,
Lancelot.

Re: [PATCH 4/6] Full paths in DAP stackTrace responses

[Tom Tromey]

>> +class FrameDecorator(_FrameDecoratorBase):
>> +    """Basic implementation of a Frame Decorator"""
>> +

Lancelot> I can see that this pre-existed your patch, but is it expected to have
Lancelot> two docstrings instead of one with empty lines to separate paragraphs?

Nope, I've fixed it.

>> @@ -172,7 +172,11 @@ def _frame_iterator(frame, frame_low, frame_high, always):
>> 
>> # Apply a basic frame decorator to all gdb.Frames.  This unifies
>> # the interface.
>> -    frame_iterator = map(FrameDecorator, frame_iterator)
>> +    if always:
>> +        decorator = DAPFrameDecorator
>> +    else:
>> +        decorator = FrameDecorator
>> +    frame_iterator = map(decorator, frame_iterator)

Lancelot> The purpose of always (always return an iterator) seems orthogonal to
Lancelot> the way `filename()` behaves.  I find this confusing.  Should the
Lancelot> parameter be renamed / a new parameter added?

I renamed it to "dap_semantics".

Tom