[PATCH v2] gdb/dap - Add support for additional target types

[PATCH v2] gdb/dap - Add support for additional target types

[Simon Farre]

After discussion with Tom and Andrew we decided that the best approach
is the simple one - make the "target" field contain a string that
performs the desired command.

So for instance "extended-remote 127.0.0.1:50505" or "remote foohost"
etc.
---
 gdb/python/lib/gdb/dap/launch.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/python/lib/gdb/dap/launch.py b/gdb/python/lib/gdb/dap/launch.py
index c3c09bc3dd0..7eae8a1916c 100644
--- a/gdb/python/lib/gdb/dap/launch.py
+++ b/gdb/python/lib/gdb/dap/launch.py
@@ -74,7 +74,7 @@ def attach(*, pid: Optional[int] = None, target: Optional[str] = None, **args):
     if pid is not None:
         cmd = "attach " + str(pid)
     elif target is not None:
-        cmd = "target remote " + target
+        cmd = "target " + target
     else:
         raise Exception("attach requires either 'pid' or 'target'")
     # Use send_gdb_with_response to ensure we get an error if the
-- 
2.40.1

Re: [PATCH v2] gdb/dap - Add support for additional target types

[Tom Tromey]

>>>>> "Simon" == Simon Farre via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> After discussion with Tom and Andrew we decided that the best approach
Simon> is the simple one - make the "target" field contain a string that
Simon> performs the desired command.

Simon> So for instance "extended-remote 127.0.0.1:50505" or "remote foohost"
Simon> etc.

I think this will require a patch to the test suite, either in
dap_target_remote or in its caller in remote-dap.exp.

It also needs an update to the documentation.

Tom

Re: [PATCH v2] gdb/dap - Add support for additional target types

[Simon Farre]

> I know this change was focused on remote/extended-remote, but just to
> backup what Tom is suggesting, 'target sim' takes an arbitrary set of
> options, which can vary depending on which simulator is being invoked,
> there's no one single set of arguments.  Thus just passing any argument
> string through from Python to GDB will be best for that target.
>
> Thanks,
> Andrew
>
I see. Yeah, I think that settles it then. I'll update the patch just 
"streamline" through the target string, as it were. This is also closer 
to the initial implementation I had tested on my machine.

Thanks!

Simon

Re: [PATCH v2] gdb/dap - Add support for additional target types

[Andrew Burgess]

Simon Farre via Gdb-patches <gdb-patches@sourceware.org> writes:

>> I don't really understand this part of the implementation.
>>
>> However, I was wondering if we really want to bother with all this.
>> Perhaps instead we should just have the clients pass any old "target"
>> argument as a string and have gdb invoke '"target " + client_string'.
>> I guess I don't see a whole lot of value in trying to separate out the
>> various parameters somehow.
>
> I think you're right actually. I looked over the other `target` commands 
> and it seems as though they all
>
> take just 1 parameter, which makes this implementation 
> superfluous/over-designed. I think the
>
> simple string approach, where the user passes `remote <addr>` or 
> `extended-remote <addr>` is better.

I know this change was focused on remote/extended-remote, but just to
backup what Tom is suggesting, 'target sim' takes an arbitrary set of
options, which can vary depending on which simulator is being invoked,
there's no one single set of arguments.  Thus just passing any argument
string through from Python to GDB will be best for that target.

Thanks,
Andrew

Re: [PATCH v2] gdb/dap - Add support for additional target types

[Simon Farre]

[-- Attachment #1: Type: text/plain, Size: 686 bytes --]


> I don't really understand this part of the implementation.
>
> However, I was wondering if we really want to bother with all this.
> Perhaps instead we should just have the clients pass any old "target"
> argument as a string and have gdb invoke '"target " + client_string'.
> I guess I don't see a whole lot of value in trying to separate out the
> various parameters somehow.

I think you're right actually. I looked over the other `target` commands 
and it seems as though they all

take just 1 parameter, which makes this implementation 
superfluous/over-designed. I think the

simple string approach, where the user passes `remote <addr>` or 
`extended-remote <addr>` is better.

Re: [PATCH v2] gdb/dap - Add support for additional target types

[Tom Tromey]

>>>>> "Simon" == Simon Farre via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Adds support for extended-remote configuration. Initial work
Simon> was really just adding 2-3 lines that checked for extended-remote
Simon> but I figured this design makes it trivial to add the remaining target
Simon> types and this design also is self-documenting.

Extensions are documented in the manual, so this should patch that as
well.

Simon> +# To add targets in the future: add required parameters set to their type
Simon> +target_type_requirements = {}
Simon> +target_type_requirements["remote"] = {"address"}
Simon> +target_type_requirements["extended-remote"] = {"address"}

I don't really understand this part of the implementation.

However, I was wondering if we really want to bother with all this.
Perhaps instead we should just have the clients pass any old "target"
argument as a string and have gdb invoke '"target " + client_string'.
I guess I don't see a whole lot of value in trying to separate out the
various parameters somehow.

Simon> +@request("attach")
Simon> +def attach(
Simon> +    *,
Simon> +    pid: Optional[int] = None,
Simon> +    target: Optional[dict] = None,
Simon> +    **args
Simon> +):
Simon> +    # Ensure configurationDone does not try to run (in launch.py)
Simon> +    global _program
Simon> +    _program = None

I don't think this will work properly.
However if we go with the "simple string" approach then the code would
be as simple, or simpler, and could just remain where it is.

Tom

[PATCH v2] gdb/dap - Add support for additional target types

[Simon Farre]

v2.
Forgot to add new file to Makefile

Adds support for extended-remote configuration. Initial work
was really just adding 2-3 lines that checked for extended-remote
but I figured this design makes it trivial to add the remaining target
types and this design also is self-documenting.

For remaining types, add the type to target requirements and the
required field(s) for it and in parse_target add branch for what result
it should be parsed into.
---
 gdb/data-directory/Makefile.in     |  1 +
 gdb/python/lib/gdb/dap/__init__.py |  1 +
 gdb/python/lib/gdb/dap/attach.py   | 72 ++++++++++++++++++++++++++++++
 gdb/python/lib/gdb/dap/launch.py   | 20 +--------
 4 files changed, 75 insertions(+), 19 deletions(-)
 create mode 100644 gdb/python/lib/gdb/dap/attach.py

diff --git a/gdb/data-directory/Makefile.in b/gdb/data-directory/Makefile.in
index a3775a4a666..0b204a93ff7 100644
--- a/gdb/data-directory/Makefile.in
+++ b/gdb/data-directory/Makefile.in
@@ -95,6 +95,7 @@ PYTHON_FILE_LIST = \
 	gdb/dap/frames.py \
 	gdb/dap/__init__.py \
 	gdb/dap/io.py \
+	gdb/dap/attach.py \
 	gdb/dap/launch.py \
 	gdb/dap/locations.py \
 	gdb/dap/memory.py \
diff --git a/gdb/python/lib/gdb/dap/__init__.py b/gdb/python/lib/gdb/dap/__init__.py
index f3dd3ff7ea8..9e8a8566e56 100644
--- a/gdb/python/lib/gdb/dap/__init__.py
+++ b/gdb/python/lib/gdb/dap/__init__.py
@@ -25,6 +25,7 @@ from . import bt
 from . import disassemble
 from . import evaluate
 from . import launch
+from . import attach
 from . import locations
 from . import memory
 from . import next
diff --git a/gdb/python/lib/gdb/dap/attach.py b/gdb/python/lib/gdb/dap/attach.py
new file mode 100644
index 00000000000..6f36b4fbe3f
--- /dev/null
+++ b/gdb/python/lib/gdb/dap/attach.py
@@ -0,0 +1,72 @@
+# 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/>.
+
+import gdb
+
+# These are deprecated in 3.9, but required in older versions.
+from typing import Optional
+
+from .server import request
+from .startup import send_gdb_with_response
+
+
+# To add targets in the future: add required parameters set to their type
+target_type_requirements = {}
+target_type_requirements["remote"] = {"address"}
+target_type_requirements["extended-remote"] = {"address"}
+
+def check_target_requirements(target: dict[str]):
+    global target_type_requirements
+    type = target.get("type")
+    if type is None or target_type_requirements.get(type) is None:
+        supported = list(target_type_requirements.keys())
+        raise Exception("'type' field incorrect or missing. Supported types {}"
+                        .format(supported))
+
+    # If requirements is a subset of target; all requirements met
+    if not (target_type_requirements[type] <= target.keys()):
+        missing = target_type_requirements[type] - target.keys()
+        raise Exception("Required fields missing: {}".format(list(missing)))
+
+def parse_target(target: dict[str]) -> str:
+    check_target_requirements(target)
+    type = target["type"]
+    if type == "extended-remote" or type == "remote":
+        addr = target["address"]
+        return "target {} {}".format(type, addr)
+
+    # We should never reach this; check_target_requirements make sure of that.
+    raise gdb.GdbError("Attach configuration parsing failed")
+
+@request("attach")
+def attach(
+    *,
+    pid: Optional[int] = None,
+    target: Optional[dict] = None,
+    **args
+):
+    # Ensure configurationDone does not try to run (in launch.py)
+    global _program
+    _program = None
+    if pid is not None:
+        cmd = "attach " + str(pid)
+    elif target is not None:
+        cmd = parse_target(target)
+    else:
+        raise Exception("attach requires either 'pid' or 'target' fields")
+    # Use send_gdb_with_response to ensure we get an error if the
+    # attach fails.
+    send_gdb_with_response(cmd)
+    return None
diff --git a/gdb/python/lib/gdb/dap/launch.py b/gdb/python/lib/gdb/dap/launch.py
index c3c09bc3dd0..031bd2dcde5 100644
--- a/gdb/python/lib/gdb/dap/launch.py
+++ b/gdb/python/lib/gdb/dap/launch.py
@@ -20,12 +20,11 @@ from typing import Mapping, Optional, Sequence
 
 from .events import ExecutionInvoker
 from .server import request, capability
-from .startup import send_gdb, send_gdb_with_response, in_gdb_thread, exec_and_log
+from .startup import send_gdb, in_gdb_thread, exec_and_log
 
 
 _program = None
 
-
 @in_gdb_thread
 def _set_args_env(args, env):
     inf = gdb.selected_inferior()
@@ -66,23 +65,6 @@ def launch(
         send_gdb(lambda: _set_args_env(args, env))
 
 
-@request("attach")
-def attach(*, pid: Optional[int] = None, target: Optional[str] = None, **args):
-    # Ensure configurationDone does not try to run.
-    global _program
-    _program = None
-    if pid is not None:
-        cmd = "attach " + str(pid)
-    elif target is not None:
-        cmd = "target remote " + target
-    else:
-        raise Exception("attach requires either 'pid' or 'target'")
-    # Use send_gdb_with_response to ensure we get an error if the
-    # attach fails.
-    send_gdb_with_response(cmd)
-    return None
-
-
 @capability("supportsConfigurationDoneRequest")
 @request("configurationDone")
 def config_done(**args):
-- 
2.40.1