diff mbox series

Fix DAP stackTrace through frames without debuginfo

Message ID 20230215194847.3805619-1-tromey@adacore.com
State New
Headers
Series Fix DAP stackTrace through frames without debuginfo |

Commit Message

Tom Tromey Feb. 15, 2023, 7:48 p.m. UTC 
  The DAP stackTrace implementation did not fully account for frames
without debuginfo.  Attemping this would yield a result like:

{"request_seq": 5, "type": "response", "command": "stackTrace", "success": false, "message": "'NoneType' object has no attribute 'filename'", "seq": 11}

This patch fixes the problem by adding another check for None.
---
 gdb/python/lib/gdb/dap/bt.py         |  4 +--
 gdb/testsuite/gdb.dap/bt-inner.c     | 24 ++++++++++++++
 gdb/testsuite/gdb.dap/bt-main.c      | 29 +++++++++++++++++
 gdb/testsuite/gdb.dap/bt-nodebug.exp | 47 ++++++++++++++++++++++++++++
 4 files changed, 102 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dap/bt-inner.c
 create mode 100644 gdb/testsuite/gdb.dap/bt-main.c
 create mode 100644 gdb/testsuite/gdb.dap/bt-nodebug.exp

Comments

Tom Tromey March 6, 2023, 3:17 p.m. UTC | #1 
>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> The DAP stackTrace implementation did not fully account for frames
Tom> without debuginfo.  Attemping this would yield a result like:

Tom> {"request_seq": 5, "type": "response", "command": "stackTrace", "success": false, "message": "'NoneType' object has no attribute 'filename'", "seq": 11}

Tom> This patch fixes the problem by adding another check for None.

I'm checking this in now.

Tom
Tom de Vries March 9, 2023, 8:38 a.m. UTC | #2 
On 3/6/23 16:17, Tom Tromey via Gdb-patches wrote:
>>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> The DAP stackTrace implementation did not fully account for frames
> Tom> without debuginfo.  Attemping this would yield a result like:
> 
> Tom> {"request_seq": 5, "type": "response", "command": "stackTrace", "success": false, "message": "'NoneType' object has no attribute 'filename'", "seq": 11}
> 
> Tom> This patch fixes the problem by adding another check for None.
> 
> I'm checking this in now.


Hi,

I'm getting:
...
{"request_seq": 5, "type": "response", "command": "stackTrace", 
"success": false, "message": "unhashable type: 'gdb.Frame'", "seq": 
11}FAIL: gdb.dap/bt-nodebug.exp: backtrace success
...

This is with python 3.6m.

Thanks,
- Tom
READ: <<<{"seq": 2, "type": "request", "command": "launch", "arguments": {"program": "/data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.dap/bt-nodebug/bt-nodebug"}}>>>
WROTE: <<<{"request_seq": 2, "type": "response", "command": "launch", "success": true}>>>
+++ file /data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.dap/bt-nodebug/bt-nodebug
READ: <<<{"seq": 3, "type": "request", "command": "setFunctionBreakpoints", "arguments": {"breakpoints": [{"name": "function_breakpoint_here"}]}}>>>
>>> Reading symbols from /data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.dap/bt-nodebug/bt-nodebug...

WROTE: <<<{"type": "event", "event": "breakpoint", "body": {"reason": "new", "breakpoint": {"id": 1, "verified": true, "source": {"name": "bt-main.c", "path": "/data/vries/gdb/binutils-gdb.git/gdb/testsuite/gdb.dap/bt-main.c", "sourceReference": 0}, "line": 23, "instructionReference": "0x4004ab"}}}>>>
WROTE: <<<{"request_seq": 3, "type": "response", "command": "setFunctionBreakpoints", "body": {"breakpoints": [{"id": 1, "verified": true, "source": {"name": "bt-main.c", "path": "/data/vries/gdb/binutils-gdb.git/gdb/testsuite/gdb.dap/bt-main.c", "sourceReference": 0}, "line": 23, "instructionReference": "0x4004ab"}]}, "success": true}>>>
WROTE: <<<{"type": "event", "event": "output", "body": {"category": "stdout", "output": "Breakpoint 1 at 0x4004ab: file /data/vries/gdb/src/gdb/testsuite/gdb.dap/bt-main.c, line 23.\n"}}>>>
READ: <<<{"seq": 4, "type": "request", "command": "configurationDone"}>>>
WROTE: <<<{"request_seq": 4, "type": "response", "command": "configurationDone", "success": true}>>>
+++ run
WROTE: <<<{"type": "event", "event": "thread", "body": {"reason": "started", "threadId": 1}}>>>
_suppress_cont case
WROTE: <<<{"type": "event", "event": "breakpoint", "body": {"reason": "changed", "breakpoint": {"id": 1, "verified": true, "source": {"name": "bt-main.c", "path": "/data/vries/gdb/binutils-gdb.git/gdb/testsuite/gdb.dap/bt-main.c", "sourceReference": 0}, "line": 23, "instructionReference": "0x4004ab"}}}>>>
entering _on_stop: <gdb.BreakpointEvent object at 0x7fead45eb648>
WROTE: <<<{"type": "event", "event": "stopped", "body": {"threadId": 1, "allThreadsStopped": true, "hitBreakpointIds": [1], "reason": "breakpoint"}}>>>
>>> Starting program: /data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.dap/bt-nodebug/bt-nodebug 
[setting tty state failed in terminal_inferior: Inappropriate ioctl for device]

READ: <<<{"seq": 5, "type": "request", "command": "stackTrace", "arguments": {"threadId": 1}}>>>
+++ thread 1
>>> [Switching to thread 1 (process 21092)]
#0  function_breakpoint_here () at /data/vries/gdb/src/gdb/testsuite/gdb.dap/bt-main.c:23
23	}

Traceback (most recent call last):
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/server.py", line 75, in _handle_command
    body = _commands[params["command"]](**args)
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/bt.py", line 93, in stacktrace
    return send_gdb_with_response(lambda: _backtrace(threadId, levels, startFrame))
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/startup.py", line 192, in send_gdb_with_response
    raise val
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/startup.py", line 184, in message
    val = fn()
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/bt.py", line 93, in <lambda>
    return send_gdb_with_response(lambda: _backtrace(threadId, levels, startFrame))
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/startup.py", line 78, in ensure_gdb_thread
    return func(*args, **kwargs)
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/bt.py", line 62, in _backtrace
    "id": frame_id(current_frame),
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/startup.py", line 78, in ensure_gdb_thread
    return func(*args, **kwargs)
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/frames.py", line 46, in frame_id
    if pair not in _frame_ids:
TypeError: unhashable type: 'gdb.Frame'
WROTE: <<<{"request_seq": 5, "type": "response", "command": "stackTrace", "success": false, "message": "unhashable type: 'gdb.Frame'"}>>>
READ: <<<{"seq": 6, "type": "request", "command": "disconnect"}>>>
WROTE: <<<{"request_seq": 6, "type": "response", "command": "disconnect", "success": true}>>>
+++ quit
WROTE: <<<{"type": "event", "event": "exited", "body": {"exitCode": 0}}>>>
Tom Tromey March 9, 2023, 2:24 p.m. UTC | #3 
Tom> I'm getting:
Tom> ...
Tom> {"request_seq": 5, "type": "response", "command": "stackTrace",
Tom> "success": false, "message": "unhashable type: 'gdb.Frame'", "seq":
Tom> 11}FAIL: gdb.dap/bt-nodebug.exp: backtrace success
Tom> ...

Tom> This is with python 3.6m.

Thanks for the report.

I guess a more recent Python somehow allows this.
Anyway I think I can fix it by implementing tp_hash for this type.

Tom
Tom Tromey March 9, 2023, 2:52 p.m. UTC | #4 
Tom> I'm getting:
Tom> ...
Tom> {"request_seq": 5, "type": "response", "command": "stackTrace",
Tom> "success": false, "message": "unhashable type: 'gdb.Frame'", "seq":
Tom> 11}FAIL: gdb.dap/bt-nodebug.exp: backtrace success
Tom> ...

Tom> This is with python 3.6m.

Can you try the appended?

Tom

commit cc1fbba7d9027927db2af10724c276e1e7c5ad7e
Author: Tom Tromey <tromey@adacore.com>
Date:   Thu Mar 9 07:47:29 2023 -0700

    Implement hash function for gdb.Frame
    
    Tom de Vries pointed out that one DAP test failed on Python 3.6
    because gdb.Frame is not hashable.  This patch adds a hash function to
    gdb.Frame.

diff --git a/gdb/frame-id.h b/gdb/frame-id.h
index 8ddf7d11408..53abbe203bf 100644
--- a/gdb/frame-id.h
+++ b/gdb/frame-id.h
@@ -112,6 +112,9 @@ struct frame_id
   /* Return a string representation of this frame id.  */
   std::string to_string () const;
 
+  /* A hash function for this frame_id.  */
+  hashval_t hash () const;
+
   /* Returns true when this frame_id and R identify the same
      frame.  */
   bool operator== (const frame_id &r) const;
diff --git a/gdb/frame.c b/gdb/frame.c
index 9b867b6cd9c..478bd881b63 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -228,26 +228,12 @@ frame_addr_hash (const void *ap)
 {
   const frame_info *frame = (const frame_info *) ap;
   const struct frame_id f_id = frame->this_id.value;
-  hashval_t hash = 0;
 
   gdb_assert (f_id.stack_status != FID_STACK_INVALID
 	      || f_id.code_addr_p
 	      || f_id.special_addr_p);
 
-  if (f_id.stack_status == FID_STACK_VALID)
-    hash = iterative_hash (&f_id.stack_addr,
-			   sizeof (f_id.stack_addr), hash);
-  if (f_id.code_addr_p)
-    hash = iterative_hash (&f_id.code_addr,
-			   sizeof (f_id.code_addr), hash);
-  if (f_id.special_addr_p)
-    hash = iterative_hash (&f_id.special_addr,
-			   sizeof (f_id.special_addr), hash);
-
-  char user_created_p = f_id.user_created_p;
-  hash = iterative_hash (&user_created_p, sizeof (user_created_p), hash);
-
-  return hash;
+  return f_id.hash ();
 }
 
 /* Internal equality function for the hash table.  This function
@@ -796,6 +782,29 @@ frame_id_artificial_p (frame_id l)
   return l.artificial_depth != 0;
 }
 
+/* See frame-id.h.  */
+
+hashval_t
+frame_id::hash () const
+{
+  hashval_t hash = 0;
+
+  if (stack_status == FID_STACK_VALID)
+    hash = iterative_hash (&stack_addr,
+			   sizeof (stack_addr), hash);
+  if (code_addr_p)
+    hash = iterative_hash (&code_addr,
+			   sizeof (code_addr), hash);
+  if (special_addr_p)
+    hash = iterative_hash (&special_addr,
+			   sizeof (special_addr), hash);
+
+  char user_created_p = user_created_p;
+  hash = iterative_hash (&user_created_p, sizeof (user_created_p), hash);
+
+  return hash;
+}
+
 bool
 frame_id::operator== (const frame_id &r) const
 {
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index ecd55d2e568..099f39ac44e 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -687,6 +687,23 @@ gdbpy_frame_stop_reason_string (PyObject *self, PyObject *args)
   return PyUnicode_Decode (str, strlen (str), host_charset (), NULL);
 }
 
+/* Implement the hash method.  */
+
+static Py_hash_t
+frapy_hash (PyObject *self)
+{
+  frame_object *self_frame = (frame_object *) self;
+
+  hashval_t hash = self_frame->frame_id.hash ();
+  hash <<= 1;
+  hash |= self_frame->frame_id_is_next;
+
+  Py_hash_t result = (Py_hash_t) hash;
+  if (result == -1)
+    result = 0;
+  return result;
+}
+
 /* Implements the equality comparison for Frame objects.
    All other comparison operators will throw a TypeError Python exception,
    as they aren't valid for frames.  */
@@ -816,7 +833,7 @@ PyTypeObject frame_object_type = {
   0,				  /* tp_as_number */
   0,				  /* tp_as_sequence */
   0,				  /* tp_as_mapping */
-  0,				  /* tp_hash  */
+  frapy_hash,			  /* tp_hash  */
   0,				  /* tp_call */
   frapy_str,			  /* tp_str */
   0,				  /* tp_getattro */
Tom de Vries March 9, 2023, 4:10 p.m. UTC | #5 
On 3/9/23 15:52, Tom Tromey wrote:
> Tom> I'm getting:
> Tom> ...
> Tom> {"request_seq": 5, "type": "response", "command": "stackTrace",
> Tom> "success": false, "message": "unhashable type: 'gdb.Frame'", "seq":
> Tom> 11}FAIL: gdb.dap/bt-nodebug.exp: backtrace success
> Tom> ...
> 
> Tom> This is with python 3.6m.
> 
> Can you try the appended?
> 

Sure.  Still a FAIL, but a different error.  Log attached.

Thanks,
- Tom
READ: <<<{"seq": 2, "type": "request", "command": "launch", "arguments": {"program": "/data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.dap/bt-nodebug/bt-nodebug"}}>>>
WROTE: <<<{"request_seq": 2, "type": "response", "command": "launch", "success": true}>>>
+++ file /data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.dap/bt-nodebug/bt-nodebug
READ: <<<{"seq": 3, "type": "request", "command": "setFunctionBreakpoints", "arguments": {"breakpoints": [{"name": "function_breakpoint_here"}]}}>>>
>>> Reading symbols from /data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.dap/bt-nodebug/bt-nodebug...

WROTE: <<<{"type": "event", "event": "breakpoint", "body": {"reason": "new", "breakpoint": {"id": 1, "verified": true, "source": {"name": "bt-main.c", "path": "/data/vries/gdb/binutils-gdb.git/gdb/testsuite/gdb.dap/bt-main.c", "sourceReference": 0}, "line": 23, "instructionReference": "0x4004ab"}}}>>>
WROTE: <<<{"type": "event", "event": "output", "body": {"category": "stdout", "output": "Breakpoint 1 at 0x4004ab: file /data/vries/gdb/src/gdb/testsuite/gdb.dap/bt-main.c, line 23.\n"}}>>>
WROTE: <<<{"request_seq": 3, "type": "response", "command": "setFunctionBreakpoints", "body": {"breakpoints": [{"id": 1, "verified": true, "source": {"name": "bt-main.c", "path": "/data/vries/gdb/binutils-gdb.git/gdb/testsuite/gdb.dap/bt-main.c", "sourceReference": 0}, "line": 23, "instructionReference": "0x4004ab"}]}, "success": true}>>>
READ: <<<{"seq": 4, "type": "request", "command": "configurationDone"}>>>
WROTE: <<<{"request_seq": 4, "type": "response", "command": "configurationDone", "success": true}>>>
+++ run
WROTE: <<<{"type": "event", "event": "thread", "body": {"reason": "started", "threadId": 1}}>>>
_suppress_cont case
WROTE: <<<{"type": "event", "event": "breakpoint", "body": {"reason": "changed", "breakpoint": {"id": 1, "verified": true, "source": {"name": "bt-main.c", "path": "/data/vries/gdb/binutils-gdb.git/gdb/testsuite/gdb.dap/bt-main.c", "sourceReference": 0}, "line": 23, "instructionReference": "0x4004ab"}}}>>>
entering _on_stop: <gdb.BreakpointEvent object at 0x7f040459d648>
WROTE: <<<{"type": "event", "event": "stopped", "body": {"threadId": 1, "allThreadsStopped": true, "hitBreakpointIds": [1], "reason": "breakpoint"}}>>>
>>> Starting program: /data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.dap/bt-nodebug/bt-nodebug 
[setting tty state failed in terminal_inferior: Inappropriate ioctl for device]

READ: <<<{"seq": 5, "type": "request", "command": "stackTrace", "arguments": {"threadId": 1}}>>>
+++ thread 1
>>> [Switching to thread 1 (process 31948)]
#0  function_breakpoint_here () at /data/vries/gdb/src/gdb/testsuite/gdb.dap/bt-main.c:23
23	}

Traceback (most recent call last):
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/server.py", line 75, in _handle_command
    body = _commands[params["command"]](**args)
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/bt.py", line 93, in stacktrace
    return send_gdb_with_response(lambda: _backtrace(threadId, levels, startFrame))
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/startup.py", line 192, in send_gdb_with_response
    raise val
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/startup.py", line 184, in message
    val = fn()
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/bt.py", line 93, in <lambda>
    return send_gdb_with_response(lambda: _backtrace(threadId, levels, startFrame))
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/startup.py", line 78, in ensure_gdb_thread
    return func(*args, **kwargs)
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/bt.py", line 62, in _backtrace
    "id": frame_id(current_frame),
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/startup.py", line 78, in ensure_gdb_thread
    return func(*args, **kwargs)
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/frames.py", line 50, in frame_id
    return _frame_ids[pair]
KeyError: (1, <built-in method level of gdb.Frame object at 0x7f0404204970>)
WROTE: <<<{"request_seq": 5, "type": "response", "command": "stackTrace", "success": false, "message": "(1, <built-in method level of gdb.Frame object at 0x7f0404204970>)"}>>>
READ: <<<{"seq": 6, "type": "request", "command": "disconnect"}>>>
WROTE: <<<{"request_seq": 6, "type": "response", "command": "disconnect", "success": true}>>>
+++ quit
WROTE: <<<{"type": "event", "event": "exited", "body": {"exitCode": 0}}>>>
Tom Tromey March 9, 2023, 6:45 p.m. UTC | #6 
Tom> Sure.  Still a FAIL, but a different error.  Log attached.

Ok, I finally figured it out (I think) and it's a latent bug in
frames.py.  I have no idea how/why this ever worked.

Tom

commit 7190893e3563f7353a6ebbf130e7aa6a145c18a1
Author: Tom Tromey <tromey@adacore.com>
Date:   Thu Mar 9 07:47:29 2023 -0700

    Implement hash function for gdb.Frame
    
    Tom de Vries pointed out that one DAP test failed on Python 3.6
    because gdb.Frame is not hashable.  This error was mildly confusing,
    because I think the real issue was this:
    
    -    pair = (gdb.selected_thread().global_num, frame.level)
    
    Here, 'frame.level' is a bound method, not just a simple value.
    
    This patch adds a hash function to gdb.Frame, then fixes frames.py to
    use it directly.

diff --git a/gdb/frame-id.h b/gdb/frame-id.h
index 8ddf7d11408..53abbe203bf 100644
--- a/gdb/frame-id.h
+++ b/gdb/frame-id.h
@@ -112,6 +112,9 @@ struct frame_id
   /* Return a string representation of this frame id.  */
   std::string to_string () const;
 
+  /* A hash function for this frame_id.  */
+  hashval_t hash () const;
+
   /* Returns true when this frame_id and R identify the same
      frame.  */
   bool operator== (const frame_id &r) const;
diff --git a/gdb/frame.c b/gdb/frame.c
index 9b867b6cd9c..478bd881b63 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -228,26 +228,12 @@ frame_addr_hash (const void *ap)
 {
   const frame_info *frame = (const frame_info *) ap;
   const struct frame_id f_id = frame->this_id.value;
-  hashval_t hash = 0;
 
   gdb_assert (f_id.stack_status != FID_STACK_INVALID
 	      || f_id.code_addr_p
 	      || f_id.special_addr_p);
 
-  if (f_id.stack_status == FID_STACK_VALID)
-    hash = iterative_hash (&f_id.stack_addr,
-			   sizeof (f_id.stack_addr), hash);
-  if (f_id.code_addr_p)
-    hash = iterative_hash (&f_id.code_addr,
-			   sizeof (f_id.code_addr), hash);
-  if (f_id.special_addr_p)
-    hash = iterative_hash (&f_id.special_addr,
-			   sizeof (f_id.special_addr), hash);
-
-  char user_created_p = f_id.user_created_p;
-  hash = iterative_hash (&user_created_p, sizeof (user_created_p), hash);
-
-  return hash;
+  return f_id.hash ();
 }
 
 /* Internal equality function for the hash table.  This function
@@ -796,6 +782,29 @@ frame_id_artificial_p (frame_id l)
   return l.artificial_depth != 0;
 }
 
+/* See frame-id.h.  */
+
+hashval_t
+frame_id::hash () const
+{
+  hashval_t hash = 0;
+
+  if (stack_status == FID_STACK_VALID)
+    hash = iterative_hash (&stack_addr,
+			   sizeof (stack_addr), hash);
+  if (code_addr_p)
+    hash = iterative_hash (&code_addr,
+			   sizeof (code_addr), hash);
+  if (special_addr_p)
+    hash = iterative_hash (&special_addr,
+			   sizeof (special_addr), hash);
+
+  char user_created_p = user_created_p;
+  hash = iterative_hash (&user_created_p, sizeof (user_created_p), hash);
+
+  return hash;
+}
+
 bool
 frame_id::operator== (const frame_id &r) const
 {
diff --git a/gdb/python/lib/gdb/dap/frames.py b/gdb/python/lib/gdb/dap/frames.py
index 337bbedae0f..4b554f3d920 100644
--- a/gdb/python/lib/gdb/dap/frames.py
+++ b/gdb/python/lib/gdb/dap/frames.py
@@ -18,8 +18,8 @@ import gdb
 from .startup import in_gdb_thread
 
 
-# Map from frame (thread,level) pair to frame ID numbers.  Note we
-# can't use the frame itself here as it is not hashable.
+# Map from frame pair to the frame ID number that is passed back to
+# the client.
 _frame_ids = {}
 
 # Map from frame ID number back to frames.
@@ -42,12 +42,11 @@ gdb.events.cont.connect(_clear_frame_ids)
 def frame_id(frame):
     """Return the frame identifier for FRAME."""
     global _frame_ids, _id_to_frame
-    pair = (gdb.selected_thread().global_num, frame.level)
-    if pair not in _frame_ids:
+    if frame not in _frame_ids:
         id = len(_frame_ids)
-        _frame_ids[pair] = id
+        _frame_ids[frame] = id
         _id_to_frame[id] = frame
-    return _frame_ids[pair]
+    return _frame_ids[frame]
 
 
 @in_gdb_thread
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index ecd55d2e568..099f39ac44e 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -687,6 +687,23 @@ gdbpy_frame_stop_reason_string (PyObject *self, PyObject *args)
   return PyUnicode_Decode (str, strlen (str), host_charset (), NULL);
 }
 
+/* Implement the hash method.  */
+
+static Py_hash_t
+frapy_hash (PyObject *self)
+{
+  frame_object *self_frame = (frame_object *) self;
+
+  hashval_t hash = self_frame->frame_id.hash ();
+  hash <<= 1;
+  hash |= self_frame->frame_id_is_next;
+
+  Py_hash_t result = (Py_hash_t) hash;
+  if (result == -1)
+    result = 0;
+  return result;
+}
+
 /* Implements the equality comparison for Frame objects.
    All other comparison operators will throw a TypeError Python exception,
    as they aren't valid for frames.  */
@@ -816,7 +833,7 @@ PyTypeObject frame_object_type = {
   0,				  /* tp_as_number */
   0,				  /* tp_as_sequence */
   0,				  /* tp_as_mapping */
-  0,				  /* tp_hash  */
+  frapy_hash,			  /* tp_hash  */
   0,				  /* tp_call */
   frapy_str,			  /* tp_str */
   0,				  /* tp_getattro */
Tom de Vries March 9, 2023, 8:35 p.m. UTC | #7 
On 3/9/23 19:45, Tom Tromey wrote:
> Tom> Sure.  Still a FAIL, but a different error.  Log attached.
> 
> Ok, I finally figured it out (I think) and it's a latent bug in
> frames.py.  I have no idea how/why this ever worked.
> 

Sorry, yet another error.

Thanks,
- Tom
READ: <<<{"seq": 2, "type": "request", "command": "launch", "arguments": {"program": "/data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.dap/bt-nodebug/bt-nodebug"}}>>>
WROTE: <<<{"request_seq": 2, "type": "response", "command": "launch", "success": true}>>>
+++ file /data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.dap/bt-nodebug/bt-nodebug
READ: <<<{"seq": 3, "type": "request", "command": "setFunctionBreakpoints", "arguments": {"breakpoints": [{"name": "function_breakpoint_here"}]}}>>>
>>> Reading symbols from /data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.dap/bt-nodebug/bt-nodebug...

WROTE: <<<{"type": "event", "event": "breakpoint", "body": {"reason": "new", "breakpoint": {"id": 1, "verified": true, "source": {"name": "bt-main.c", "path": "/data/vries/gdb/binutils-gdb.git/gdb/testsuite/gdb.dap/bt-main.c", "sourceReference": 0}, "line": 23, "instructionReference": "0x4004ab"}}}>>>
WROTE: <<<{"request_seq": 3, "type": "response", "command": "setFunctionBreakpoints", "body": {"breakpoints": [{"id": 1, "verified": true, "source": {"name": "bt-main.c", "path": "/data/vries/gdb/binutils-gdb.git/gdb/testsuite/gdb.dap/bt-main.c", "sourceReference": 0}, "line": 23, "instructionReference": "0x4004ab"}]}, "success": true}>>>
WROTE: <<<{"type": "event", "event": "output", "body": {"category": "stdout", "output": "Breakpoint 1 at 0x4004ab: file /data/vries/gdb/src/gdb/testsuite/gdb.dap/bt-main.c, line 23.\n"}}>>>
READ: <<<{"seq": 4, "type": "request", "command": "configurationDone"}>>>
WROTE: <<<{"request_seq": 4, "type": "response", "command": "configurationDone", "success": true}>>>
+++ run
WROTE: <<<{"type": "event", "event": "thread", "body": {"reason": "started", "threadId": 1}}>>>
_suppress_cont case
WROTE: <<<{"type": "event", "event": "breakpoint", "body": {"reason": "changed", "breakpoint": {"id": 1, "verified": true, "source": {"name": "bt-main.c", "path": "/data/vries/gdb/binutils-gdb.git/gdb/testsuite/gdb.dap/bt-main.c", "sourceReference": 0}, "line": 23, "instructionReference": "0x4004ab"}}}>>>
entering _on_stop: <gdb.BreakpointEvent object at 0x7f61b03e0648>
WROTE: <<<{"type": "event", "event": "stopped", "body": {"threadId": 1, "allThreadsStopped": true, "hitBreakpointIds": [1], "reason": "breakpoint"}}>>>
>>> Starting program: /data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.dap/bt-nodebug/bt-nodebug 
[setting tty state failed in terminal_inferior: Inappropriate ioctl for device]

READ: <<<{"seq": 5, "type": "request", "command": "stackTrace", "arguments": {"threadId": 1}}>>>
+++ thread 1
>>> [Switching to thread 1 (process 30982)]
#0  function_breakpoint_here () at /data/vries/gdb/src/gdb/testsuite/gdb.dap/bt-main.c:23
23	}

Traceback (most recent call last):
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/server.py", line 75, in _handle_command
    body = _commands[params["command"]](**args)
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/bt.py", line 93, in stacktrace
    return send_gdb_with_response(lambda: _backtrace(threadId, levels, startFrame))
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/startup.py", line 192, in send_gdb_with_response
    raise val
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/startup.py", line 184, in message
    val = fn()
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/bt.py", line 93, in <lambda>
    return send_gdb_with_response(lambda: _backtrace(threadId, levels, startFrame))
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/startup.py", line 78, in ensure_gdb_thread
    return func(*args, **kwargs)
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/bt.py", line 62, in _backtrace
    "id": frame_id(current_frame),
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/startup.py", line 78, in ensure_gdb_thread
    return func(*args, **kwargs)
  File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/frames.py", line 49, in frame_id
    return _frame_ids[frame]
KeyError: <gdb.Frame object at 0x7f61b0047bb0>
WROTE: <<<{"request_seq": 5, "type": "response", "command": "stackTrace", "success": false, "message": "<gdb.Frame object at 0x7f61b0047bb0>"}>>>
READ: <<<{"seq": 6, "type": "request", "command": "disconnect"}>>>
WROTE: <<<{"request_seq": 6, "type": "response", "command": "disconnect", "success": true}>>>
+++ quit
WROTE: <<<{"type": "event", "event": "exited", "body": {"exitCode": 0}}>>>
Tom Tromey March 13, 2023, 6:46 p.m. UTC | #8 
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Sorry, yet another error.

I do not understand this one.

Tom>   File "/data/vries/gdb/leap-15-4/build/gdb/data-directory/python/gdb/dap/frames.py", line 49, in frame_id
Tom>     return _frame_ids[frame]
Tom> KeyError: <gdb.Frame object at 0x7f61b0047bb0>

I don't really know what's going wrong here.  I wonder if the new hash
function could be incorrect.  Like, if the contents of the frame id
change, causing the hash to change, then this would presumably break the
Python hash table.

Instead of a hash I can change this to use a list I suppose.

Tom
Tom Tromey March 14, 2023, 1:08 p.m. UTC | #9 
Tom> Sorry, yet another error.

Could you try this one instead?

Tom

commit 28705d727f9c90a77a8b7f78b60ac97f010abf48
Author: Tom Tromey <tromey@adacore.com>
Date:   Tue Mar 14 07:05:13 2023 -0600

    Fix DAP frame bug with older versions of Python
    
    Tom de Vries pointed out that one DAP test failed on Python 3.6
    because gdb.Frame is not hashable.
    
    This patch fixes the problem by using a list to hold the frames.  This
    is less efficient but there normally won't be that many frames.

diff --git a/gdb/python/lib/gdb/dap/frames.py b/gdb/python/lib/gdb/dap/frames.py
index 337bbedae0f..08209d0b361 100644
--- a/gdb/python/lib/gdb/dap/frames.py
+++ b/gdb/python/lib/gdb/dap/frames.py
@@ -18,20 +18,17 @@ import gdb
 from .startup import in_gdb_thread
 
 
-# Map from frame (thread,level) pair to frame ID numbers.  Note we
-# can't use the frame itself here as it is not hashable.
-_frame_ids = {}
-
-# Map from frame ID number back to frames.
-_id_to_frame = {}
+# A list of all the frames we've reported.  A frame's index in the
+# list is its ID.  We don't use a hash here because frames are not
+# hashable.
+_all_frames = []
 
 
 # Clear all the frame IDs.
 @in_gdb_thread
 def _clear_frame_ids(evt):
-    global _frame_ids, _id_to_frame
-    _frame_ids = {}
-    _id_to_frame = {}
+    global _all_frames
+    _all_frames = []
 
 
 # Clear the frame ID map whenever the inferior runs.
@@ -41,17 +38,17 @@ gdb.events.cont.connect(_clear_frame_ids)
 @in_gdb_thread
 def frame_id(frame):
     """Return the frame identifier for FRAME."""
-    global _frame_ids, _id_to_frame
-    pair = (gdb.selected_thread().global_num, frame.level)
-    if pair not in _frame_ids:
-        id = len(_frame_ids)
-        _frame_ids[pair] = id
-        _id_to_frame[id] = frame
-    return _frame_ids[pair]
+    global _all_frames
+    for i in range(0, len(_all_frames)):
+        if _all_frames[i] == frame:
+            return i
+    result = len(_all_frames)
+    _all_frames.append(frame)
+    return result
 
 
 @in_gdb_thread
 def frame_for_id(id):
     """Given a frame identifier ID, return the corresponding frame."""
-    global _id_to_frame
-    return _id_to_frame[id]
+    global _all_frames
+    return _all_frames[id]
Tom de Vries March 14, 2023, 1:28 p.m. UTC | #10 
On 3/14/23 14:08, Tom Tromey wrote:
> Tom> Sorry, yet another error.
> 
> Could you try this one instead?
> 

This fixes the failure for me, and I've tested both gdb.dap test-cases, 
so ...

Tested-by: Tom de Vries <tdevries@suse.de>

Thanks,
- Tom

> Tom
> 
> commit 28705d727f9c90a77a8b7f78b60ac97f010abf48
> Author: Tom Tromey <tromey@adacore.com>
> Date:   Tue Mar 14 07:05:13 2023 -0600
> 
>      Fix DAP frame bug with older versions of Python
>      
>      Tom de Vries pointed out that one DAP test failed on Python 3.6
>      because gdb.Frame is not hashable.
>      
>      This patch fixes the problem by using a list to hold the frames.  This
>      is less efficient but there normally won't be that many frames.
> 
> diff --git a/gdb/python/lib/gdb/dap/frames.py b/gdb/python/lib/gdb/dap/frames.py
> index 337bbedae0f..08209d0b361 100644
> --- a/gdb/python/lib/gdb/dap/frames.py
> +++ b/gdb/python/lib/gdb/dap/frames.py
> @@ -18,20 +18,17 @@ import gdb
>   from .startup import in_gdb_thread
>   
>   
> -# Map from frame (thread,level) pair to frame ID numbers.  Note we
> -# can't use the frame itself here as it is not hashable.
> -_frame_ids = {}
> -
> -# Map from frame ID number back to frames.
> -_id_to_frame = {}
> +# A list of all the frames we've reported.  A frame's index in the
> +# list is its ID.  We don't use a hash here because frames are not
> +# hashable.
> +_all_frames = []
>   
>   
>   # Clear all the frame IDs.
>   @in_gdb_thread
>   def _clear_frame_ids(evt):
> -    global _frame_ids, _id_to_frame
> -    _frame_ids = {}
> -    _id_to_frame = {}
> +    global _all_frames
> +    _all_frames = []
>   
>   
>   # Clear the frame ID map whenever the inferior runs.
> @@ -41,17 +38,17 @@ gdb.events.cont.connect(_clear_frame_ids)
>   @in_gdb_thread
>   def frame_id(frame):
>       """Return the frame identifier for FRAME."""
> -    global _frame_ids, _id_to_frame
> -    pair = (gdb.selected_thread().global_num, frame.level)
> -    if pair not in _frame_ids:
> -        id = len(_frame_ids)
> -        _frame_ids[pair] = id
> -        _id_to_frame[id] = frame
> -    return _frame_ids[pair]
> +    global _all_frames
> +    for i in range(0, len(_all_frames)):
> +        if _all_frames[i] == frame:
> +            return i
> +    result = len(_all_frames)
> +    _all_frames.append(frame)
> +    return result
>   
>   
>   @in_gdb_thread
>   def frame_for_id(id):
>       """Given a frame identifier ID, return the corresponding frame."""
> -    global _id_to_frame
> -    return _id_to_frame[id]
> +    global _all_frames
> +    return _all_frames[id]
Tom Tromey March 14, 2023, 2:03 p.m. UTC | #11 
Tom> This fixes the failure for me, and I've tested both gdb.dap
Tom> test-cases, so ...

Tom> Tested-by: Tom de Vries <tdevries@suse.de>

Thanks.  I'm going to check it in.

Tom
diff mbox series

Patch

diff --git a/gdb/python/lib/gdb/dap/bt.py b/gdb/python/lib/gdb/dap/bt.py
index 990ab135b05..01d1aa088e2 100644
--- a/gdb/python/lib/gdb/dap/bt.py
+++ b/gdb/python/lib/gdb/dap/bt.py
@@ -1,4 +1,4 @@ 
-# Copyright 2022 Free Software Foundation, Inc.
+# Copyright 2022, 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
@@ -69,7 +69,7 @@  def _backtrace(thread_id, levels, startFrame):
                 "instructionPointerReference": hex(current_frame.pc()),
             }
             sal = _safe_sal(current_frame)
-            if sal is not None:
+            if sal is not None and sal.symtab is not None:
                 newframe["source"] = {
                     "name": os.path.basename(sal.symtab.filename),
                     "path": sal.symtab.filename,
diff --git a/gdb/testsuite/gdb.dap/bt-inner.c b/gdb/testsuite/gdb.dap/bt-inner.c
new file mode 100644
index 00000000000..f5d48ab128e
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/bt-inner.c
@@ -0,0 +1,24 @@ 
+/* Copyright 2023 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+extern void function_breakpoint_here (void);
+
+void
+no_debug_info (void)
+{
+  function_breakpoint_here ();
+}
diff --git a/gdb/testsuite/gdb.dap/bt-main.c b/gdb/testsuite/gdb.dap/bt-main.c
new file mode 100644
index 00000000000..7664ef79932
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/bt-main.c
@@ -0,0 +1,29 @@ 
+/* Copyright 2023 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+extern void no_debug_info (void);
+
+void
+function_breakpoint_here (void)
+{
+}
+
+int main ()
+{
+  no_debug_info ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.dap/bt-nodebug.exp b/gdb/testsuite/gdb.dap/bt-nodebug.exp
new file mode 100644
index 00000000000..e365f0877d4
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/bt-nodebug.exp
@@ -0,0 +1,47 @@ 
+# 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 DAP 'bt' through a function without debuginfo.
+
+load_lib dap-support.exp
+
+standard_testfile bt-main.c bt-inner.c
+
+if {[build_executable_from_specs $testfile.exp $testfile {} \
+	 $srcfile debug \
+	 $srcfile2 {}] == -1} {
+    return
+}
+
+if {[dap_launch $testfile] == ""} {
+    return
+}
+
+set obj [dap_check_request_and_response "set breakpoint on inner" \
+	     setFunctionBreakpoints \
+	     {o breakpoints [a [o name [s function_breakpoint_here]]]}]
+set fn_bpno [dap_get_breakpoint_number $obj]
+
+dap_check_request_and_response "start inferior" configurationDone
+dap_wait_for_event_and_check "inferior started" thread "body reason" started
+
+lassign [dap_wait_for_event_and_check "stopped at function breakpoint" stopped \
+	    "body reason" breakpoint \
+	    "body hitBreakpointIds" $fn_bpno] unused objs
+
+# The bug was that this request would fail.
+dap_check_request_and_response "backtrace" stackTrace {o threadId [i 1]}
+
+dap_shutdown