bug#39180: 27.0.50; [PATCH] Use expressions as memory location in gdb-mi memory buffer

[Yuan Fu <casouri@gmail.com>]

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



> On Jan 31, 2020, at 5:05 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Sat, 18 Jan 2020 15:54:35 -0500
>> 
>> Currently gdb-mi does allow expressions as memory address, but it translates it to a fixed address. This patch makes gdb to store the expression and re-evaluate on updates. So the address changes as expression’s value changes.
> 
> Thanks.
> 
> Are these 4 patches needed to add the above improvement, or is each
> part of the series independent, and could be applied on its own right?
> 

I merged them into one.

> If the entire series should be applied in a single transaction, please
> make then a single patch, as that makes it easier to review and
> apply.  Otherwise, please explain what is the rationale for each part
> separately, because I don't think I understand it.
> 
>> Before the memory buffer evaluates the expression as address
>> and use the fixed result in each stop. This change stores the
>> expression itself and reevaluates it in each stop for an address.
>> Then displays the value of the memory at that address.
> 
> Two spaces between sentences (here and elsewhere in the patch),
> please.
> 
>> lisp/progmodes/gdb-mi.el (gdb-memory-address-expression): new
>>  (gdb-memory-address): change default value, add docstring
>>  (def-gdb-trigger-and-handler gdb-invalidate-memory,
>>  gdb-memory-set-address): replace ’gdb-memory-address’ with
>>    ’gdb-memory-address-expression’
>>  (gdb-memory-header): Add display for ’gdb-memory-address-expression’,
>>    move the mouse event from address to expression
> 
> Please quote symbols 'like this'.

Both fixed.

> 
>> * lisp/progmodes/gdb-mi.el (gdb-read-memory-custom):
>> Break infinite loop. Change ’error’ to ’user-error’
> 
> I don't understand what infinite loop are you alluding to here, and
> how did it come into existence.

Added some comments to explain.

> 
>> * lisp/progmodes/gdb-mi.el (gdb-memory-header):
>> Protect against nil value
> 
> And what is the problem you are trying to solve here?

Added some comments.

> 
>> * lisp/progmodes/gdb-mi.el (gdb--memory-display-warning): new
>> (gdb-read-memory-custom, gdb-memory-header): Add warning
> 
> And what is this part about?

Added some explanation in commit message and docstring of gdb--memory-display-warning.

And here is the new patch.

Yuan


[-- Attachment #2: memory-fixed.patch --]
[-- Type: application/octet-stream, Size: 6702 bytes --]

From 6be6b8aa1215aeac0cbed7432b980408b1275b24 Mon Sep 17 00:00:00 2001
From: Yuan Fu <casouri@gmail.com>
Date: Sat, 5 Oct 2019 22:42:07 -0400
Subject: [PATCH] Enhance memory buffer in gdb-mi

1. Enhance support for expressions as memory address

Before, the memory buffer evaluates the expression as address and use
the fixed result in each stop.  This change makes gdb store the
expression itself and reevaluates it in each stop for an address.

We also add a warning (a red bold exclamation mark) on the header line
when the content of the page doesn't represent the memory location
user requested for.  That happends when some error occurs and we
display the last successful page.

lisp/progmodes/gdb-mi.el (gdb-memory-address-expression,
gdb--memory-display-warning): New variables.
(gdb-memory-address): Change default value to nil; add docstring.
(def-gdb-trigger-and-handler, gdb-invalidate-memory,
gdb-memory-set-address): Replace 'gdb-memory-address' with
'gdb-memory-address-expression'.
(gdb-memory-header): Add code to display
'gdb-memory-address-expression' on header line.  Move the mouse event
from address to expression.  Add code to display the warning.
(gdb-memory-header): Fix the error from
'propertize' when 'gdb-memory-address-expression' or
'gdb-memory-address' is nil.
(gdb-read-memory-custom): Change 'error' to 'user-error'.  Add code to
display the warning.
---
 lisp/progmodes/gdb-mi.el | 57 +++++++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 15 deletions(-)

diff --git a/lisp/progmodes/gdb-mi.el b/lisp/progmodes/gdb-mi.el
index 60852e4ad6..1384837a04 100644
--- a/lisp/progmodes/gdb-mi.el
+++ b/lisp/progmodes/gdb-mi.el
@@ -105,13 +105,24 @@ tool-bar-map
 (defvar speedbar-initial-expansion-list-name)
 (defvar speedbar-frame)
 
-(defvar	gdb-memory-address "main")
-(defvar	gdb-memory-last-address nil
+(defvar-local gdb-memory-address-expression "main"
+  "This expression is passed to gdb.
+Possible value: main, $rsp, x+3.")
+(defvar-local gdb-memory-address nil
+  "Address of memory display.")
+(defvar-local gdb-memory-last-address nil
   "Last successfully accessed memory address.")
 (defvar	gdb-memory-next-page nil
   "Address of next memory page for program memory buffer.")
 (defvar	gdb-memory-prev-page nil
   "Address of previous memory page for program memory buffer.")
+(defvar-local gdb--memory-display-warning nil
+  "Display warning on memory header if t.
+
+When error occurs when retrieving memory, gdb-mi displays the
+last successful page.  In that case the expression might not
+match the memory displayed.  We want to let the user be aware of
+that, so display a warning exclamation mark in the header line.")
 
 (defvar gdb-thread-number nil
   "Main current thread.
@@ -3444,7 +3455,7 @@ gdb-memory-unit
 (def-gdb-trigger-and-handler
   gdb-invalidate-memory
   (format "-data-read-memory %s %s %d %d %d"
-          gdb-memory-address
+          (gdb-mi-quote gdb-memory-address-expression)
           gdb-memory-format
           gdb-memory-unit
           gdb-memory-rows
@@ -3484,6 +3495,9 @@ gdb-read-memory-custom
          (err-msg (bindat-get-field res 'msg)))
     (if (not err-msg)
         (let ((memory (bindat-get-field res 'memory)))
+          (when gdb-memory-last-address
+            ;; Nil means last retrieve emits error or just started the session.
+            (setq gdb--memory-display-warning nil))
           (setq gdb-memory-address (bindat-get-field res 'addr))
           (setq gdb-memory-next-page (bindat-get-field res 'next-page))
           (setq gdb-memory-prev-page (bindat-get-field res 'prev-page))
@@ -3497,10 +3511,15 @@ gdb-read-memory-custom
                                             gdb-memory-format)))))
             (newline)))
       ;; Show last page instead of empty buffer when out of bounds
-      (progn
-        (let ((gdb-memory-address gdb-memory-last-address))
+      (when gdb-memory-last-address
+        (let ((gdb-memory-address-expression gdb-memory-last-address))
+          ;; If we don't set `gdb-memory-last-address' to nil,
+          ;; `gdb-invalidate-memory' eventually calls
+          ;; `gdb-read-memory-custom', making an infinite loop.
+          (setq gdb-memory-last-address nil
+                gdb--memory-display-warning t)
           (gdb-invalidate-memory 'update)
-          (error err-msg))))))
+          (user-error "Error when retrieving memory: %s Displaying last successful page" err-msg))))))
 
 (defvar gdb-memory-mode-map
   (let ((map (make-sparse-keymap)))
@@ -3534,7 +3553,7 @@ gdb-memory-set-address
   "Set the start memory address."
   (interactive)
   (let ((arg (read-from-minibuffer "Memory address: ")))
-    (setq gdb-memory-address arg))
+    (setq gdb-memory-address-expression arg))
   (gdb-invalidate-memory 'update))
 
 (defmacro def-gdb-set-positive-number (name variable echo-string &optional doc)
@@ -3717,7 +3736,19 @@ gdb-memory-font-lock-keywords
 (defvar gdb-memory-header
   '(:eval
     (concat
-     "Start address["
+     "Start address "
+     ;; If `gdb-memory-address-expression' is nil, `propertize' would error.
+     (propertize (or gdb-memory-address-expression "N/A")
+                 'face font-lock-warning-face
+                 'help-echo "mouse-1: set start address"
+                 'mouse-face 'mode-line-highlight
+                 'local-map (gdb-make-header-line-mouse-map
+                             'mouse-1
+                             #'gdb-memory-set-address-event))
+     (if gdb--memory-display-warning
+         (propertize " !" 'face '(:inherit error :weight bold))
+       "")
+     " ["
      (propertize "-"
                  'face font-lock-warning-face
                  'help-echo "mouse-1: decrement address"
@@ -3734,13 +3765,9 @@ gdb-memory-header
                              'mouse-1
                              #'gdb-memory-show-next-page))
      "]: "
-     (propertize gdb-memory-address
-                 'face font-lock-warning-face
-                 'help-echo "mouse-1: set start address"
-                 'mouse-face 'mode-line-highlight
-                 'local-map (gdb-make-header-line-mouse-map
-                             'mouse-1
-                             #'gdb-memory-set-address-event))
+     ;; If `gdb-memory-address' is nil, `propertize' would error.
+     (propertize (or gdb-memory-address "N/A")
+                 'face font-lock-warning-face)
      "  Rows: "
      (propertize (number-to-string gdb-memory-rows)
                  'face font-lock-warning-face
-- 
2.25.0