diff mbox series

[3/4] Avoid exception from attach in DAP

Message ID 20231212-dap-no-test-exceptions-v1-3-af0e33f10093@adacore.com
State New
Headers
Series Check for rogue DAP exceptions |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Tom Tromey Dec. 12, 2023, 5:44 p.m. UTC 
  I noticed that the DAP attach test case (and similarly
remoted-dap.exp) had a rogue exception stack trace in the log.  It
turns out that an attach will generate a stop that does not have a
reason.

This patch fixes the problem in the _on_stop event listener by making
it a bit more careful when examining the event reason.  It also adds
some machinery so that attach stops can be suppressed, which I think
is the right thing to do.
---
 gdb/python/lib/gdb/dap/events.py | 43 +++++++++++++++++++++++++++++++---------
 gdb/python/lib/gdb/dap/launch.py |  3 ++-
 2 files changed, 36 insertions(+), 10 deletions(-)

Comments

Kévin Le Gouguec Dec. 13, 2023, 4:36 p.m. UTC | #1 
Tom Tromey <tromey@adacore.com> writes:

> +    if "reason" not in event.details:
> +        # This can only really happen via a "repl" evaluation of
> +        # something like "attach".  In this case just emit a generic
> +        # stop.
> +        obj["reason"] = "stopped"
>      else:
> -        obj["reason"] = stop_reason_map[reason]
> +        reason = event.details["reason"]
> +        if (
> +                _expected_pause
> +                and reason == "signal-received"
> +                and event.details["signal-name"] in ("SIGINT", "SIGSTOP")
> +        ):
> +            obj["reason"] = "pause"
> +        else:
> +            global stop_reason_map
> +            obj["reason"] = stop_reason_map[reason]
>      _expected_pause = False
>      send_event("stopped", obj)

Nit: I'd find

    if "reason" not in event.details:
        # ...
        obj["reason"] = "stopped"
    elif (
        _expected_pause
        and event.details["reason"] == "signal-received"
        and event.details["signal-name"] in ("SIGINT", "SIGSTOP")
    ):
        obj["reason"] = "pause"
    else:
        global stop_reason_map
        obj["reason"] = stop_reason_map[event.details["reason"]]

slightly more legible (less nesting; easier to understand that the
purpose of each branch is to set obj["reason"]), even if it means
repeating event.details["reason"].  Really not a huge deal though, good
to go as-is for me.

Reviewed-By: Kévin Le Gouguec <legouguec@adacore.com>
Tom Tromey Dec. 14, 2023, 6:52 p.m. UTC | #2 
>>>>> "Kévin" == Kévin Le Gouguec <legouguec@adacore.com> writes:

Kévin> Nit: I'd find
...
Kévin> slightly more legible (less nesting; easier to understand that the
Kévin> purpose of each branch is to set obj["reason"]), even if it means
Kévin> repeating event.details["reason"].  Really not a huge deal though, good
Kévin> to go as-is for me.

I like it, I've made this change here.

Tom
diff mbox series

Patch

diff --git a/gdb/python/lib/gdb/dap/events.py b/gdb/python/lib/gdb/dap/events.py
index cbefe90e4ca..82fe99632d5 100644
--- a/gdb/python/lib/gdb/dap/events.py
+++ b/gdb/python/lib/gdb/dap/events.py
@@ -147,6 +147,16 @@  def _cont(event):
         )
 
 
+_suppress_stop = False
+
+
+@in_gdb_thread
+def suppress_stop():
+    """Indicate that the next stop should not emit an event."""
+    global _suppress_stop
+    _suppress_stop = True
+
+
 _expected_pause = False
 
 
@@ -198,6 +208,13 @@  stop_reason_map = {
 def _on_stop(event):
     global inferior_running
     inferior_running = False
+
+    global _suppress_stop
+    if _suppress_stop:
+        _suppress_stop = False
+        log("suppressing stop in _on_stop")
+        return
+
     log("entering _on_stop: " + repr(event))
     log("   details: " + repr(event.details))
     obj = {
@@ -206,17 +223,25 @@  def _on_stop(event):
     }
     if isinstance(event, gdb.BreakpointEvent):
         obj["hitBreakpointIds"] = [x.number for x in event.breakpoints]
-    global stop_reason_map
-    reason = event.details["reason"]
     global _expected_pause
-    if (
-        _expected_pause
-        and reason == "signal-received"
-        and event.details["signal-name"] in ("SIGINT", "SIGSTOP")
-    ):
-        obj["reason"] = "pause"
+    # Some stop events still do not emit details.  For example,
+    # 'attach' causes a reason-less stop.
+    if "reason" not in event.details:
+        # This can only really happen via a "repl" evaluation of
+        # something like "attach".  In this case just emit a generic
+        # stop.
+        obj["reason"] = "stopped"
     else:
-        obj["reason"] = stop_reason_map[reason]
+        reason = event.details["reason"]
+        if (
+                _expected_pause
+                and reason == "signal-received"
+                and event.details["signal-name"] in ("SIGINT", "SIGSTOP")
+        ):
+            obj["reason"] = "pause"
+        else:
+            global stop_reason_map
+            obj["reason"] = stop_reason_map[reason]
     _expected_pause = False
     send_event("stopped", obj)
 
diff --git a/gdb/python/lib/gdb/dap/launch.py b/gdb/python/lib/gdb/dap/launch.py
index a8adb125707..a20009c190d 100644
--- a/gdb/python/lib/gdb/dap/launch.py
+++ b/gdb/python/lib/gdb/dap/launch.py
@@ -18,7 +18,7 @@  import gdb
 # These are deprecated in 3.9, but required in older versions.
 from typing import Mapping, Optional, Sequence
 
-from .events import exec_and_expect_stop, expect_process
+from .events import exec_and_expect_stop, expect_process, suppress_stop
 from .server import request, capability
 from .startup import exec_and_log, DAPException
 
@@ -71,6 +71,7 @@  def attach(*, pid: Optional[int] = None, target: Optional[str] = None, **args):
     else:
         raise DAPException("attach requires either 'pid' or 'target'")
     expect_process("attach")
+    suppress_stop()
     exec_and_log(cmd)