bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Mattias Engdegård]

When GDB sends a string containing octally-escaped characters, it will be something like

 "abc\377def"

which is first massaged into JSON and then parsed as such, but since JSON doesn't recognise octal escapes, the result is

 "abc377def"

which is wrong. This assumes gdb-mi-decode-strings is nil, which it is by default; otherwise the octal escapes are decoded by a fragile preprocessing stage (gdb-mi-decode) which itself has known problems as noted in a comment.

Frankly, this business with going via JSON after several ad-hoc text-based transforms isn't very principled.

While the bug could be 'solved' by adding yet another regexp hack to gdb-mi-decode or gdb-mi-jsonify-buffer, I suggest we write a GDB/MI parser in Lisp directly, ditching the gdb-mi-decode preprocessing and JSON form entirely, solving this and related bugs once and for all. Many transforms can then be done on the S-expression result after parsing, which should be more efficient and less error-prone.

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 23 Oct 2020 13:50:24 +0200
> 
> While the bug could be 'solved' by adding yet another regexp hack to gdb-mi-decode or gdb-mi-jsonify-buffer, I suggest we write a GDB/MI parser in Lisp directly, ditching the gdb-mi-decode preprocessing and JSON form entirely, solving this and related bugs once and for all. Many transforms can then be done on the S-expression result after parsing, which should be more efficient and less error-prone.

I'm okay with writing a GDB/MI parser, but I'm not sure I understand
how would that help to solve this particular conundrum.  AFAIR,
there's a genuine ambiguity there regarding non-ASCII characters
reported from GDB.  Could you tell how will this be solved by a
different parser?

P.S. Btw: gdb-mi.el already has a BNF parser for GDB/MI.

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Mattias Engdegård]

23 okt. 2020 kl. 14.01 skrev Eli Zaretskii <eliz@gnu.org>:

> I'm okay with writing a GDB/MI parser, but I'm not sure I understand
> how would that help to solve this particular conundrum.  AFAIR,
> there's a genuine ambiguity there regarding non-ASCII characters
> reported from GDB.

Would you mind explaining the ambiguity? Do you mean what coding system should be used for "\303\266" -- whether it should be interpreted as a string of those two bytes, the string "ö", the string "ö", or something else?
This bug is not about the encoding; it's about not interpreting the string as "303266".

>  Could you tell how will this be solved by a
> different parser?

Again, I'm not sure what you mean. The bug arises because we feed incorrectly translated data into a JSON parser. If we parse the string ourselves instead of going via JSON, that particular problem goes away.

> P.S. Btw: gdb-mi.el already has a BNF parser for GDB/MI.

It doesn't parse the lower parts of the grammar -- 'result', 'value' and so on. JSON is used for that.

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 23 Oct 2020 14:41:02 +0200
> Cc: 44173@debbugs.gnu.org
> 
> 23 okt. 2020 kl. 14.01 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > I'm okay with writing a GDB/MI parser, but I'm not sure I understand
> > how would that help to solve this particular conundrum.  AFAIR,
> > there's a genuine ambiguity there regarding non-ASCII characters
> > reported from GDB.
> 
> Would you mind explaining the ambiguity? Do you mean what coding system should be used for "\303\266" -- whether it should be interpreted as a string of those two bytes, the string "ö", the string "ö", or something else?

My memory is imperfect, but luckily I was wise enough to summarize the
problems in a comment to gdb-mi-decode, which you mentioned.  Let me
now quote it:

  ;; FIXME: This is fragile: it relies on the assumption that all the
  ;; non-ASCII strings output by GDB, including names of the source
  ;; files, values of string variables in the inferior, etc., are all
  ;; encoded in the same encoding.  It also assumes that the \nnn
  ;; sequences are not split between chunks of output of the GDB process
  ;; due to buffering, and arrive together.  Finally, if some string
  ;; included literal \nnn strings (as opposed to non-ASCII characters
  ;; converted by GDB/MI to octal escapes), this decoding will mangle
  ;; those strings.  When/if GDB acquires the ability to not
  ;; escape-protect non-ASCII characters in its MI output, this kludge
  ;; should be removed.

The basic ambiguity, AFAIR, is what is described last here: a string
reported bu GDB could include literal \nnn sequences, which are not
non-ASCII characters that GDB/MI converts to octal escapes.  The
information which was which is lost once we receive the GDB/MI output.

> This bug is not about the encoding; it's about not interpreting the string as "303266".

AFAIU, this bug's root cause is the way we solved the ambiguity, which
basically assumes one of the possible interpretations should be
preferred to another, because it is more popular/useful.

Let me turn the table and ask you how did you get that string you show
in the original report?  What kind of application were you debugging,
and what did that string mean in that application?

> >  Could you tell how will this be solved by a
> > different parser?
> 
> Again, I'm not sure what you mean. The bug arises because we feed incorrectly translated data into a JSON parser. If we parse the string ourselves instead of going via JSON, that particular problem goes away.

And what will then happen to non-ASCII strings and file names reported
by GDB?  How will our parser solve that?

> > P.S. Btw: gdb-mi.el already has a BNF parser for GDB/MI.
> 
> It doesn't parse the lower parts of the grammar -- 'result', 'value' and so on. JSON is used for that.

Do you intend to extend the existing parser or write a new one from
scratch?

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Mattias Engdegård]

23 okt. 2020 kl. 15.19 skrev Eli Zaretskii <eliz@gnu.org>:

> The basic ambiguity, AFAIR, is what is described last here: a string
> reported bu GDB could include literal \nnn sequences, which are not
> non-ASCII characters that GDB/MI converts to octal escapes.  The
> information which was which is lost once we receive the GDB/MI output.

So you mean that GDB would produce the value "\303" that does not stand for a string containing the single byte octal 303? When does this occur?

> AFAIU, this bug's root cause is the way we solved the ambiguity, which
> basically assumes one of the possible interpretations should be
> preferred to another, because it is more popular/useful.

Then we disagree. The code doesn't do the right thing if gdb-mi-decode-string is nil, unless you by 'ambiguity' mean that GDB sometimes inserts a spurious backslash that should be ignored. When gdb-mi-decode-string is non-nil, it is sometimes wrong as well.

> Let me turn the table and ask you how did you get that string you show
> in the original report?

A program in the C language containing the local declaration

  char *s = "\303\266";

produces nonsense in the 'Locals' window when debugged. It doesn't matter what the string means; I would have been happy with gdb/emacs interpreting it as utf-8, latin-1 or just raw bytes presented in octal or hex.

> And what will then happen to non-ASCII strings and file names reported
> by GDB?  How will our parser solve that?

The parser can either leave the strings as undecoded unibyte strings -- that is, "\303\266" would be a 2-char string -- or decode them according to gdb-mi-decode-strings, in which case it might become a 1-char multibyte string. In the former case, the code receiving the parse tree could decide what to do with the strings and how to display them, perhaps on a case-by-case basis.

> Do you intend to extend the existing parser or write a new one from
> scratch?

Extending the existing one appears sensible, just replacing the JSON tour.

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 23 Oct 2020 16:21:55 +0200
> Cc: 44173@debbugs.gnu.org
> 
> 23 okt. 2020 kl. 15.19 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > The basic ambiguity, AFAIR, is what is described last here: a string
> > reported bu GDB could include literal \nnn sequences, which are not
> > non-ASCII characters that GDB/MI converts to octal escapes.  The
> > information which was which is lost once we receive the GDB/MI output.
> 
> So you mean that GDB would produce the value "\303" that does not
> stand for a string containing the single byte octal 303?

Yes.

> When does this occur?

There's nothing special in the text "\303" that says it must be an
octal escape.  They are just 4 ASCII characters.

Moreover, even if it is an escape, it is not clear how to interpret
it: as a raw byte or as a character encoded in some 8-bit encoding.
As you probably know, GDB has settings that control how strings it
reports are encoded, so the same string can be reported in different
forms.

> > AFAIU, this bug's root cause is the way we solved the ambiguity, which
> > basically assumes one of the possible interpretations should be
> > preferred to another, because it is more popular/useful.
> 
> Then we disagree. The code doesn't do the right thing if gdb-mi-decode-string is nil, unless you by 'ambiguity' mean that GDB sometimes inserts a spurious backslash that should be ignored. When gdb-mi-decode-string is non-nil, it is sometimes wrong as well.

The ambiguity is whether gdb-mi-decode-strings should be nil or
non-nil.  We have it nil by default because non-ASCII strings and file
names are rare, but when you are debugging a program that uses such
strings, you had better set it non-nil.  And when some of your
program's source file use non-ASCII characters, you _must_ set it
non-nil, otherwise "M-x gdb" will not find the source files it needs
to visit as you step through the program.

> > Let me turn the table and ask you how did you get that string you show
> > in the original report?
> 
> A program in the C language containing the local declaration
> 
>   char *s = "\303\266";
> 
> produces nonsense in the 'Locals' window when debugged. It doesn't matter what the string means; I would have been happy with gdb/emacs interpreting it as utf-8, latin-1 or just raw bytes presented in octal or hex.

That doesn't really answer my question, though, about the use case
that causes such a string to be in the program.  Without a use case, I
could tell you to set gdb-mi-decode-strings non-nil and be done with
it.

> > And what will then happen to non-ASCII strings and file names reported
> > by GDB?  How will our parser solve that?
> 
> The parser can either leave the strings as undecoded unibyte strings -- that is, "\303\266" would be a 2-char string -- or decode them according to gdb-mi-decode-strings, in which case it might become a 1-char multibyte string. In the former case, the code receiving the parse tree could decide what to do with the strings and how to display them, perhaps on a case-by-case basis.

Well, I know that several possible ways exist, but each one of them
loses in some situations.  You say "the code receiving the parse tree
could decide", but will that code have information to make that
decision correctly?  And if you must decide in the parser, how would
you suggest to make the decision to avoid making incorrect decisions?

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Mattias Engdegård]

23 okt. 2020 kl. 16.44 skrev Eli Zaretskii <eliz@gnu.org>:

> There's nothing special in the text "\303" that says it must be an
> octal escape.  They are just 4 ASCII characters.

The grammar uses the name 'c-string', so it is reasonable to assume that most of the lexical conventions of C strings are obeyed.

Perhaps you mean that \ooo can occur outside c-string productions? If so, please say where you have seen it, or have evidence of it being produced.

The possibilities are limited. For example, stream records may use an unquoted (newline-terminated) string in place of a c-string, but I haven't seen any evidence of this in practice and it appears that gdb-mi.el does not handle that case either.

The only other possibility in the grammar would be inside 'variable' productions (field keys) which are unquoted, but those only come from a small set of fixed names.

To be clear: the exact encoding of non-ASCII bytes (whether present literally or as octal escapes in c-string tokens) is unclear, and I do not attempt to solve that problem here and now. This is about more fundamental parsing and lexing problems.

Namely: handling octal escapes in gdb-gdbmi-marker-filter is doing it at the wrong level. Moreover, this substitution, when performed, is not correct since it ignores the context; the 4-byte (excluding quotes) string "\\377" then appears to the user as the 2-byte string "\\377", where the '\377' sequence is painted in a distinct colour and really is the raw byte 0xff, and thus not a valid C character escape sequence.

Finally, the JSON mess is evidently the wrong way to go since it does not take care of strings properly -- and heavens know what else, since gdb-jsonify-buffer works at the wrong level (a pattern here) by doing regexp replacement on the whole text prior to parsing.

> That doesn't really answer my question, though, about the use case
> that causes such a string to be in the program.  Without a use case, I
> could tell you to set gdb-mi-decode-strings non-nil and be done with
> it.

Why a string is in a program is irrelevant; the user does not necessarily know that. If Emacs says that a string contains six decimal digits when it really just contains two (nonzero) bytes, then that is a lie no matter what.

> Well, I know that several possible ways exist, but each one of them
> loses in some situations.  You say "the code receiving the parse tree
> could decide", but will that code have information to make that
> decision correctly?  And if you must decide in the parser, how would
> you suggest to make the decision to avoid making incorrect decisions?

Again that problem is outside the scope of this bug but I think we can agree that it is easier, or at least no more difficult, to make a correct decision knowing the context of the string than not.

Let's see what a value/result parser can do and work from there.

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 23 Oct 2020 19:31:47 +0200
> Cc: 44173@debbugs.gnu.org
> 
> Let's see what a value/result parser can do and work from there.

Whatever.  I don't understand your answers and don't see how they
resolve the issues I raised.  I guess we will have to see who is right
when the code is in place.  Too bad.

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Mattias Engdegård]

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

23 okt. 2020 kl. 20.20 skrev Eli Zaretskii <eliz@gnu.org>:

> I don't understand your answers and don't see how they
> resolve the issues I raised.

Sorry if I've been communicating badly (but it takes two to do it).
I honestly thought I did address your concerns but must have misunderstood you.
Please tell me what you believe I have not explained properly, and I promise I'll do my best to answer it without referring to any previous message.

Meanwhile, here is a proof of concept which may clarify what I failed to put in words. It actually runs both the old and new value parsers on data sent by GDB, and logs an error message if discrepancies are found. They seem to work identically unless there are strings with octal escapes, which are handled correctly by the new parser. (Of course, a proper patch would not retain the old parser.)

If gdb-mi-decode-strings is non-nil, then file names, string contents etc are properly decoded as UTF-8 as expected, without any of the bugginess of the current code. Otherwise raw bytes are shown as octal escapes, which also fixes the original bug.


[-- Attachment #2: gdb-mi.diff --]
[-- Type: application/octet-stream, Size: 6251 bytes --]

diff --git a/lisp/progmodes/gdb-mi.el b/lisp/progmodes/gdb-mi.el
index 9e8af84a60..867e13b38a 100644
--- a/lisp/progmodes/gdb-mi.el
+++ b/lisp/progmodes/gdb-mi.el
@@ -2516,8 +2516,9 @@ gud-gdbmi-marker-filter
   "Filter GDB/MI output."
 
   ;; If required, decode non-ASCII text encoded with octal escapes.
-  (or (null gdb-mi-decode-strings)
-      (setq string (gdb-mi-decode string)))
+;; FIXME: Now done in gdb-mi--parse-c-string; this can go away
+;;  (or (null gdb-mi-decode-strings)
+;;      (setq string (gdb-mi-decode string)))
 
   ;; Record transactions if logging is enabled.
   (when gdb-enable-debug
@@ -2833,7 +2834,7 @@ gdb-jsonify-buffer
     (goto-char (point-max))
     (insert "}")))
 
-(defun gdb-json-read-buffer (&optional fix-key fix-list)
+(defun gdb-json-read-buffer-old (&optional fix-key fix-list)
   "Prepare and parse GDB/MI output in current buffer with `json-read'.
 
 FIX-KEY and FIX-LIST work as in `gdb-jsonify-buffer'."
@@ -2843,6 +2844,140 @@ gdb-json-read-buffer
     (let ((json-array-type 'list))
       (json-read))))
 
+;; Parse result records: this process converts key=value to the cons pair
+;; (variable . value) where variable is a symbol.  Lists and tuples become
+;; Lisp lists.
+
+(defun gdb-mi--parse-tuple-or-list (end-char)
+  "Parse a GDB/MI tuple or list, both returned as a Lisp list.
+END-CHAR is the ending delimiter; will stop at end-of-buffer otherwise."
+  (let ((items nil))
+    (while (not (or (eobp)
+                    (eq (following-char) end-char)))
+      (let ((item (gdb-mi--parse-result-or-value)))
+        (push item items)
+        (when (eq (following-char) ?,)
+          (forward-char))))
+    (when (eq (following-char) end-char)
+      (forward-char))
+    (nreverse items)))
+
+(defun gdb-mi--parse-c-string ()
+  (let ((start (point))
+        (pieces nil)
+        (octals-used nil))
+    (while (and (re-search-forward (rx (or ?\\ ?\")))
+                (not (eq (preceding-char) ?\")))
+      (push (buffer-substring start (1- (point))) pieces)
+      (cond
+       ((looking-at (rx (** 1 3 (any "0-7")))) ;FIXME: optimise
+        (push (unibyte-string (string-to-number (match-string 0) 8)) pieces)
+        (setq octals-used t)
+        (goto-char (match-end 0)))
+       ((looking-at (rx (any "ntrvfab\"\\")))
+        (push (cdr (assq (following-char)
+                         '((?n . "\n")
+                           (?t . "\t")
+                           (?r . "\r")
+                           (?v . "\v")
+                           (?f . "\f")
+                           (?a . "\a")
+                           (?b . "\b")
+                           (?\" . "\"")
+                           (?\\ . "\\"))))
+              pieces)
+        (forward-char))
+       (t
+        (error "Unrecognised escape char: %c" (following-char))))
+      (setq start (point)))
+    (push (buffer-substring start (1- (point))) pieces)
+    (let ((s (apply #'concat (nreverse pieces))))
+      (if (and octals-used gdb-mi-decode-strings)
+          (let ((coding
+                 (if (coding-system-p gdb-mi-decode-strings)
+                     gdb-mi-decode-strings
+                   ;; FIXME: try making this cheaper
+                   (buffer-local-value
+                    'buffer-file-coding-system
+                    (gdb-get-buffer-create 'gdb-partial-output-buffer)))))
+            (decode-coding-string s coding))
+        s))))
+
+(defun gdb-mi--parse-value ()
+  "Parse a value."
+  (cond
+   ((eq (following-char) ?\{)
+    (forward-char)
+    (gdb-mi--parse-tuple-or-list ?\}))
+   ((eq (following-char) ?\[)
+    (forward-char)
+    (gdb-mi--parse-tuple-or-list ?\]))
+   ((eq (following-char) ?\")
+    (forward-char)
+    (gdb-mi--parse-c-string))
+   (t (error "Bad start of result or value: %c" (following-char)))))
+    
+(defun gdb-mi--parse-result-or-value ()
+  "Parse a result (key=value) or value."
+  (if (looking-at (rx (group (+ (any "a-zA-Z" ?_ ?-))) "="))
+      (progn
+        (goto-char (match-end 0))
+        (let* ((variable (intern (match-string 1)))
+               (value (gdb-mi--parse-value)))
+          (cons variable value)))
+    (gdb-mi--parse-value)))
+
+(defun gdb-mi--parse-results ()
+  "Parse zero or more result productions as a list."
+  (gdb-mi--parse-tuple-or-list nil))
+
+(defun gdb-mi--fix-key (key value)
+  "Convert any result (key-value pair) in VALUE whose key is KEY to its value."
+  (cond
+   ((atom value) value)
+   ((symbolp (car value))
+    (if (eq (car value) key)
+        (cdr value)
+      (cons (car value) (gdb-mi--fix-key key (cdr value)))))
+   (t (mapcar (lambda (x) (gdb-mi--fix-key key x)) value))))
+
+(defun gdb-mi--extend-fullname (remote value)
+  "Prepend REMOTE to any result string with `fullname' as the key."
+  (cond
+   ((atom value) value)
+   ((symbolp (car value))
+    (if (and (eq (car value) 'fullname)
+             (stringp (cdr value)))
+        (cons 'fullname (concat remote (cdr value)))
+      (cons (car value) (gdb-mi--extend-fullname remote (cdr value)))))
+   (t (mapcar (lambda (x) (gdb-mi--extend-fullname remote x)) value))))
+
+(defun gdb-json-read-buffer-new (&optional fix-key _fix-list)
+  (goto-char (point-min))
+  (let ((results (gdb-mi--parse-results)))
+    (let ((remote (file-remote-p default-directory)))
+      (when remote
+        (setq results (gdb-mi--extend-fullname remote results))))
+    (when fix-key
+      ;; FIXME: fix-key should be a symbol.
+      (setq results (gdb-mi--fix-key (intern fix-key) results)))
+    ;; FIXME: fix-list is irrelevant since tuples and lists in the
+    ;; input both yield Lisp lists.
+    results))
+
+(defun gdb-json-read-buffer (&optional fix-key fix-list)
+  (save-excursion
+    (let* ((input (buffer-string))
+           (new (gdb-json-read-buffer-new fix-key fix-list))
+           (_ (cl-assert (equal (buffer-string) input)))
+           (old (gdb-json-read-buffer-old fix-key fix-list)))
+      (unless (equal old new)
+        (message "ERROR: input=%S" input)
+        (message "OLD: %S" old)
+        (message "NEW: %S" new))
+      new)))
+
+
 (defun gdb-json-string (string &optional fix-key fix-list)
   "Prepare and parse STRING containing GDB/MI output with `json-read'.
 

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 24 Oct 2020 18:21:53 +0200
> Cc: 44173@debbugs.gnu.org
> 
> If gdb-mi-decode-strings is non-nil, then file names, string contents etc are properly decoded as UTF-8 as expected

Not UTF-8, but the value of gdb-mi-decode-strings, if it's a
coding-system, right?

> without any of the bugginess of the current code. Otherwise raw bytes are shown as octal escapes, which also fixes the original bug.

I hoped/thought you intended to solve this issue as well, but if the
situation is no worse than it was before, it's fine to leave it at
that.  However, please retain at least part of the comment regarding
gdb-mi-decode-strings and the ambiguity related to its use, I think
it's important that people know that.

And I hope you've verified that this does still fix the problem in
bug#21572, which this variable and the related code tries to fix?

> +       (t
> +        (error "Unrecognised escape char: %c" (following-char))))

How about leaving the text unchanged instead of signaling an error
(and thus preventing the entire data from getting to the higher
levels)?

Thanks.

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Mattias Engdegård]

24 okt. 2020 kl. 19.23 skrev Eli Zaretskii <eliz@gnu.org>:

>> If gdb-mi-decode-strings is non-nil, then file names, string contents etc are properly decoded as UTF-8 as expected
> 
> Not UTF-8, but the value of gdb-mi-decode-strings, if it's a
> coding-system, right?

Right.

> I hoped/thought you intended to solve this issue as well, but if the
> situation is no worse than it was before, it's fine to leave it at
> that.  However, please retain at least part of the comment regarding
> gdb-mi-decode-strings and the ambiguity related to its use, I think
> it's important that people know that.

Yes, the valid parts of the comment will be kept.
I'm not sure what a solution to the remaining problems would look like, but it would probably involve splitting gdb-mi-decode-strings in separate variables for file names and program values. On the other hand, given that the world is converging to UTF-8, it may be a disappearing problem?

In any case, should we want to decode strings differently depending on their structural position in the answer, I believe that it would be better done in the field accessors instead of the parser. For example,

  (bindat-get-field breakpoint 'fullname)

might become something like

  (gdb-mi--get-string-field breakpoint 'fullname 'filename)

which would tell the accessor how to decode the field.

In the short term I suggest changing the default value of gdb-mi-decode-strings to 't' as this gives the behaviour most commonly expected by the user. However, it is not critical, and in any case orthogonal to the issue at hand. What do you think?

> And I hope you've verified that this does still fix the problem in
> bug#21572, which this variable and the related code tries to fix?

Yes -- I tried debugging programs whose source file names contain Unicode chars and they were shown correctly (with gdb-mi-decode-strings = t).

>> +       (t
>> +        (error "Unrecognised escape char: %c" (following-char))))
> 
> How about leaving the text unchanged instead of signaling an error
> (and thus preventing the entire data from getting to the higher
> levels)?

Maybe, but I really dislike hiding bugs by being overly tolerant. It is precisely this tolerant nature of 'json-read' that caused this bug in the first place. (I'm not sure whether this is compliant with RFC 8259, by the way.)
I think it's fine to signal errors if the syntax isn't what we expect; after all, that is what the JSON parser does in other cases.

Thanks for the helpful comments. I'll prepare a proper patch.

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 24 Oct 2020 20:27:13 +0200
> Cc: 44173@debbugs.gnu.org
> 
> > I hoped/thought you intended to solve this issue as well, but if the
> > situation is no worse than it was before, it's fine to leave it at
> > that.  However, please retain at least part of the comment regarding
> > gdb-mi-decode-strings and the ambiguity related to its use, I think
> > it's important that people know that.
> 
> Yes, the valid parts of the comment will be kept.
> I'm not sure what a solution to the remaining problems would look like, but it would probably involve splitting gdb-mi-decode-strings in separate variables for file names and program values.

How do we know, at that level, whether a string is a file name or not?
And even if we succeed in knowing that about source file names of the
debuggee, we have no hope of knowing whether some string in the
debuggee is a file name or just a string.

> On the other hand, given that the world is converging to UTF-8, it may be a disappearing problem?

The rate of the convergence is severely exaggerated.  And GDB can be
used to debug all kinds of targets, which is why it has settings for
the host and the target charsets.  As long as GDB doesn't convert
everything to UTF-8, I don't see how gdb-mi.el could do that.

> In any case, should we want to decode strings differently depending on their structural position in the answer, I believe that it would be better done in the field accessors instead of the parser. For example,
> 
>   (bindat-get-field breakpoint 'fullname)
> 
> might become something like
> 
>   (gdb-mi--get-string-field breakpoint 'fullname 'filename)
> 
> which would tell the accessor how to decode the field.

See above: this still doesn't solve the problem of knowing the correct
encoding, even in specific fields of specific responses.

> In the short term I suggest changing the default value of gdb-mi-decode-strings to 't' as this gives the behaviour most commonly expected by the user. However, it is not critical, and in any case orthogonal to the issue at hand. What do you think?

I'm not at all sure this is what users want, since non-ASCII file
names in debugging are quite rare.  But I don't mind changing the
default value.

> >> +       (t
> >> +        (error "Unrecognised escape char: %c" (following-char))))
> > 
> > How about leaving the text unchanged instead of signaling an error
> > (and thus preventing the entire data from getting to the higher
> > levels)?
> 
> Maybe, but I really dislike hiding bugs by being overly tolerant.

We could display a warning.  But interrupting (and possibly ruining) a
debugging session because of our bug is very harsh: the user is
certainly not guilty.

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Mattias Engdegård]

24 okt. 2020 kl. 20.44 skrev Eli Zaretskii <eliz@gnu.org>:

> How do we know, at that level, whether a string is a file name or not?
> And even if we succeed in knowing that about source file names of the
> debuggee, we have no hope of knowing whether some string in the
> debuggee is a file name or just a string.

I think we can distinguish source file names by what field they occur in (which is why I thought that doing it in the accessor might do), but you are certainly right about strings in the program being debugged.

> The rate of the convergence is severely exaggerated.  And GDB can be
> used to debug all kinds of targets, which is why it has settings for
> the host and the target charsets.  As long as GDB doesn't convert
> everything to UTF-8, I don't see how gdb-mi.el could do that.

I don't disagree. What we can do is to have defaults that cover the most likely case and let users with less common setups change those.

> See above: this still doesn't solve the problem of knowing the correct
> encoding, even in specific fields of specific responses.

That's true, and I don't have a good answer. Perhaps we somehow can get GDB (or the OS, if it is the file system) to inform us. But as you noted, even GDB doesn't know what encoding the debugged program uses. It could very well be multiple encodings at once.

A serious debugger interface would probably push this decision to the data presentation layer and allow the user to specify how he wants to view the contents of a particular string, in the same way that users select radix for viewing integers. Not sure how this would be done in gdb-mi.

>> In the short term I suggest changing the default value of gdb-mi-decode-strings to 't' as this gives the behaviour most commonly expected by the user. However, it is not critical, and in any case orthogonal to the issue at hand. What do you think?
> 
> I'm not at all sure this is what users want, since non-ASCII file
> names in debugging are quite rare.  But I don't mind changing the
> default value.

Let's do this separately then. It seems unlikely to cause trouble.

>> Maybe, but I really dislike hiding bugs by being overly tolerant.
> 
> We could display a warning.  But interrupting (and possibly ruining) a
> debugging session because of our bug is very harsh: the user is
> certainly not guilty.

I definitely have some sympathy for that point of view, but then again, warnings that don't impede the user's progress tend to go unfixed, so being 'nice' to the user isn't necessarily in his or her best interest.

But we could try (warn ...), which I suppose you were thinking about? It's fairly visible.

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Mattias Engdegård]

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

Here is an updated patch, rebased after some basic code cleaning in gdb-mi.el.
OK for master?


[-- Attachment #2: 0001-Parse-GDB-MI-results-directly-instead-of-going-via-J.patch --]
[-- Type: application/octet-stream, Size: 27835 bytes --]

From 92b6c84f6554d3b266d239bc9da3ee9a07053e95 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sun, 25 Oct 2020 12:40:36 +0100
Subject: [PATCH] Parse GDB/MI results directly instead of going via JSON
 (bug#44173)

Translating GDB/MI into JSON is an unnecessary and fragile detour
that made it hard to deal with octal escapes in strings correctly.
Parse GDB/MI directly instead.

* lisp/progmodes/gdb-mi.el (gdb-mi-decode-strings): Adjust doc string.
(gdb-mi-decode, gud-gdbmi-marker-filter): Remove gdb-mi-decode.
(gdb-jsonify-buffer): Remove.
(gdb-mi--parse-tuple-or-list, gdb-mi--parse-c-string)
(gdb-mi--parse-value, gdb-mi--parse-result-or-value)
(gdb-mi--parse-results, gdb-mi--fix-key, gdb-mi--extend-fullname)
(gdb-mi--c-string-from-string): New functions.
(gdb-json-read-buffer, gdb-json-string, gdb-json-partial-output):
Rename to gdb-mi--read-buffer, gdb-mi--from-string and
gdb-mi--partial-output respectively.  Remove useless FIX-LIST
argument.  FIX-KEY is now a symbol, not a string. All callers updated.
(gdb-tooltip-print, gdbmi-bnf-log-stream-output, gdb-internals)
(gdb-console, gdb-done-or-error, gdb-get-source-file-list)
(gdb-get-prompt, gdb-get-source-file):
Use gdb-mi--c-string-from-string instead of 'read'.
* test/lisp/progmodes/gdb-mi-tests.el: New file.
---
 lisp/progmodes/gdb-mi.el            | 318 +++++++++++++++-------------
 test/lisp/progmodes/gdb-mi-tests.el |  44 ++++
 2 files changed, 220 insertions(+), 142 deletions(-)
 create mode 100644 test/lisp/progmodes/gdb-mi-tests.el

diff --git a/lisp/progmodes/gdb-mi.el b/lisp/progmodes/gdb-mi.el
index f0262c2315..77dc392cb8 100644
--- a/lisp/progmodes/gdb-mi.el
+++ b/lisp/progmodes/gdb-mi.el
@@ -89,7 +89,6 @@
 ;;; Code:
 
 (require 'gud)
-(require 'json)
 (require 'cl-lib)
 (require 'cl-seq)
 (eval-when-compile (require 'pcase))
@@ -166,7 +165,7 @@ gdb-threads-list
   "Associative list of threads provided by \"-thread-info\" MI command.
 
 Keys are thread numbers (in strings) and values are structures as
-returned from -thread-info by `gdb-json-partial-output'.  Updated in
+returned from -thread-info by `gdb-mi--partial-output'.  Updated in
 `gdb-thread-list-handler-custom'.")
 
 (defvar gdb-running-threads-count nil
@@ -185,7 +184,7 @@ gdb-breakpoints-list
   "Associative list of breakpoints provided by \"-break-list\" MI command.
 
 Keys are breakpoint numbers (in string) and values are structures
-as returned from \"-break-list\" by `gdb-json-partial-output'
+as returned from \"-break-list\" by `gdb-mi--partial-output'
 \(\"body\" field is used). Updated in
 `gdb-breakpoints-list-handler-custom'.")
 
@@ -1122,11 +1121,11 @@ gdb-tooltip-print
                                  "\\)")
                          nil t)
       (tooltip-show
-       (concat expr " = " (read (match-string 1)))
+       (concat expr " = " (gdb-mi--c-string-from-string (match-string 1)))
        (or gud-tooltip-echo-area
 	   (not (display-graphic-p)))))
      ((re-search-forward  "msg=\\(\".+\"\\)$" nil t)
-      (tooltip-show (read (match-string 1))
+      (tooltip-show (gdb-mi--c-string-from-string (match-string 1))
        (or gud-tooltip-echo-area
 	   (not (display-graphic-p))))))))
 
@@ -1266,7 +1265,7 @@ gdb-mi--field
   (cdr (assq field value)))
 
 (defun gdb-var-create-handler (expr)
-  (let* ((result (gdb-json-partial-output)))
+  (let* ((result (gdb-mi--partial-output)))
     (if (not (gdb-mi--field result 'msg))
         (let ((var
 	       (list (gdb-mi--field result 'name)
@@ -1307,7 +1306,7 @@ gdb-var-list-children
 
 (defun gdb-var-list-children-handler (varnum)
   (let* ((var-list nil)
-	 (output (gdb-json-partial-output 'child))
+	 (output (gdb-mi--partial-output 'child))
 	 (children (gdb-mi--field output 'children)))
     (catch 'child-already-watched
       (dolist (var gdb-var-list)
@@ -1382,7 +1381,7 @@ gdb-var-update
              'gdb-var-update))
 
 (defun gdb-var-update-handler ()
-  (let ((changelist (gdb-mi--field (gdb-json-partial-output) 'changelist)))
+  (let ((changelist (gdb-mi--field (gdb-mi--partial-output) 'changelist)))
     (dolist (var gdb-var-list)
       (setcar (nthcdr 5 var) nil))
     (let ((temp-var-list gdb-var-list))
@@ -2304,7 +2303,8 @@ gdbmi-bnf-log-stream-output
   ;; Suppress "No registers."  GDB 6.8 and earlier
   ;; duplicates MI error message on internal stream.
   ;; Don't print to GUD buffer.
-  (if (not (string-equal (read c-string) "No registers.\n"))
+  (if (not (string-equal (gdb-mi--c-string-from-string c-string)
+                         "No registers.\n"))
       (gdb-internals c-string)))
 
 
@@ -2426,7 +2426,7 @@ gdbmi-bnf-incomplete-record-result
       is-complete)))
 
 
-; The following grammar rules are not yet implemented by this GDBMI-BNF parser.
+; The following grammar rules are not parsed directly by this GDBMI-BNF parser.
 ; The handling of those rules is currently done by the handlers registered
 ; in gdbmi-bnf-result-state-configs
 ;
@@ -2448,19 +2448,17 @@ gdbmi-bnf-incomplete-record-result
 ; list ==>
 ;      "[]" | "[" value ( "," value )* "]" | "[" result ( "," result )* "]"
 
+;; FIXME: This is fragile: it relies on the assumption that all the
+;; non-ASCII strings output by GDB, including names of the source
+;; files, values of string variables in the inferior, etc., are all
+;; encoded in the same encoding.
+
 (defcustom gdb-mi-decode-strings nil
   "When non-nil, decode octal escapes in GDB output into non-ASCII text.
 
 If the value is a coding-system, use that coding-system to decode
 the bytes reconstructed from octal escapes.  Any other non-nil value
-means to decode using the coding-system set for the GDB process.
-
-Warning: setting this non-nil might mangle strings reported by GDB
-that have literal substrings which match the \\nnn octal escape
-patterns, where nnn is an octal number between 200 and 377.  So
-we only recommend to set this variable non-nil if the program you
-are debugging really reports non-ASCII text, or some of its source
-file names include non-ASCII characters."
+means to decode using the coding-system set for the GDB process."
   :type '(choice
           (const :tag "Don't decode" nil)
           (const :tag "Decode using default coding-system" t)
@@ -2468,47 +2466,9 @@ gdb-mi-decode-strings
   :group 'gdb
   :version "25.1")
 
-;; The idea of the following function was suggested
-;; by Kenichi Handa <handa@gnu.org>.
-;;
-;; FIXME: This is fragile: it relies on the assumption that all the
-;; non-ASCII strings output by GDB, including names of the source
-;; files, values of string variables in the inferior, etc., are all
-;; encoded in the same encoding.  It also assumes that the \nnn
-;; sequences are not split between chunks of output of the GDB process
-;; due to buffering, and arrive together.  Finally, if some string
-;; included literal \nnn strings (as opposed to non-ASCII characters
-;; converted by GDB/MI to octal escapes), this decoding will mangle
-;; those strings.  When/if GDB acquires the ability to not
-;; escape-protect non-ASCII characters in its MI output, this kludge
-;; should be removed.
-(defun gdb-mi-decode (string)
-  "Decode octal escapes in MI output STRING into multibyte text."
-  (let ((coding
-         (if (coding-system-p gdb-mi-decode-strings)
-             gdb-mi-decode-strings
-           (with-current-buffer
-               (gdb-get-buffer-create 'gdb-partial-output-buffer)
-             buffer-file-coding-system))))
-    (with-temp-buffer
-      (set-buffer-multibyte nil)
-      (prin1 string (current-buffer))
-      (goto-char (point-min))
-      ;; prin1 quotes the octal escapes as well, which interferes with
-      ;; their interpretation by 'read' below.  Remove the extra
-      ;; backslashes to countermand that.
-      (while (re-search-forward "\\\\\\(\\\\[2-3][0-7][0-7]\\)" nil t)
-        (replace-match "\\1" nil nil))
-      (goto-char (point-min))
-      (decode-coding-string (read (current-buffer)) coding))))
-
 (defun gud-gdbmi-marker-filter (string)
   "Filter GDB/MI output."
 
-  ;; If required, decode non-ASCII text encoded with octal escapes.
-  (or (null gdb-mi-decode-strings)
-      (setq string (gdb-mi-decode string)))
-
   ;; Record transactions if logging is enabled.
   (when gdb-enable-debug
     (push (cons 'recv string) gdb-debug-log)
@@ -2555,7 +2515,7 @@ gdb-thread-created
 (defun gdb-thread-exited (_token output-field)
   "Handle =thread-exited async record.
 Unset `gdb-thread-number' if current thread exited and update threads list."
-  (let* ((thread-id (gdb-mi--field (gdb-json-string output-field) 'id)))
+  (let* ((thread-id (gdb-mi--field (gdb-mi--from-string output-field) 'id)))
     (if (string= gdb-thread-number thread-id)
         (gdb-setq-thread-number nil))
     ;; When we continue current thread and it quickly exits,
@@ -2569,7 +2529,7 @@ gdb-thread-selected
   "Handler for =thread-selected MI output record.
 
 Sets `gdb-thread-number' to new id."
-  (let* ((result (gdb-json-string output-field))
+  (let* ((result (gdb-mi--from-string output-field))
          (thread-id (gdb-mi--field result 'id)))
     (gdb-setq-thread-number thread-id)
     ;; Typing `thread N' in GUD buffer makes GDB emit `^done' followed
@@ -2585,7 +2545,7 @@ gdb-thread-selected
 
 (defun gdb-running (_token output-field)
   (let* ((thread-id
-          (gdb-mi--field (gdb-json-string output-field) 'thread-id)))
+          (gdb-mi--field (gdb-mi--from-string output-field) 'thread-id)))
     ;; We reset gdb-frame-number to nil if current thread has gone
     ;; running. This can't be done in gdb-thread-list-handler-custom
     ;; because we need correct gdb-frame-number by the time
@@ -2614,7 +2574,7 @@ gdb-stopped
   "Given the contents of *stopped MI async record, select new
 current thread and update GDB buffers."
   ;; Reason is available with target-async only
-  (let* ((result (gdb-json-string output-field))
+  (let* ((result (gdb-mi--from-string output-field))
          (reason (gdb-mi--field result 'reason))
          (thread-id (gdb-mi--field result 'thread-id))
          (retval (gdb-mi--field result 'return-value))
@@ -2694,7 +2654,7 @@ gdb-internals
 	 (if (string= output-field "\"\\n\"")
 	     ""
 	   (let ((error-message
-		  (read output-field)))
+		  (gdb-mi--c-string-from-string output-field)))
 	     (put-text-property
 	      0 (length error-message)
 	      'face font-lock-warning-face
@@ -2705,7 +2665,8 @@ gdb-internals
 ;; (frontend MI commands should not print to this stream)
 (defun gdb-console (output-field)
   (setq gdb-filter-output
-	(gdb-concat-output gdb-filter-output (read output-field))))
+	(gdb-concat-output gdb-filter-output
+                           (gdb-mi--c-string-from-string output-field))))
 
 (defun gdb-done (token-number output-field is-complete)
   (gdb-done-or-error token-number 'done output-field is-complete))
@@ -2722,7 +2683,8 @@ gdb-done-or-error
 	;; MI error - send to minibuffer
 	(when (eq type 'error)
           ;; Skip "msg=" from `output-field'
-          (message "%s" (read (substring output-field 4)))
+          (message "%s" (gdb-mi--c-string-from-string
+                         (substring output-field 4)))
           ;; Don't send to the console twice.  (If it is a console error
           ;; it is also in the console stream.)
           (setq output-field nil)))
@@ -2770,83 +2732,154 @@ gdb-clear-partial-output
   (with-current-buffer (gdb-get-buffer-create 'gdb-partial-output-buffer)
     (erase-buffer)))
 
-(defun gdb-jsonify-buffer (&optional fix-key fix-list)
-  "Prepare GDB/MI output in current buffer for parsing with `json-read'.
-
-Field names are wrapped in double quotes and equal signs are
-replaced with semicolons.
-
-If FIX-KEY is non-nil, strip all \"FIX-KEY=\" occurrences from
-partial output.  This is used to get rid of useless keys in lists
-in MI messages, e.g.: [key=.., key=..].  -stack-list-frames and
--break-info are examples of MI commands which issue such
-responses.
-
-If FIX-LIST is non-nil, \"FIX-LIST={..}\" is replaced with
-\"FIX-LIST=[..]\" prior to parsing.  This is used to fix broken
--break-info output when it contains breakpoint script field
-incompatible with GDB/MI output syntax.
+;; Parse GDB/MI result records: this process converts
+;;  list      [...]      ->  list
+;;  tuple     {...}      ->  list
+;;  result    KEY=VALUE  ->  (KEY . VALUE) where KEY is a symbol
+;;  c-string  "..."      ->  string
+
+(defun gdb-mi--parse-tuple-or-list (end-char)
+  "Parse a tuple or list, either returned as a Lisp list.
+END-CHAR is the ending delimiter; will stop at end-of-buffer otherwise."
+  (let ((items nil))
+    (while (not (or (eobp)
+                    (eq (following-char) end-char)))
+      (let ((item (gdb-mi--parse-result-or-value)))
+        (push item items)
+        (when (eq (following-char) ?,)
+          (forward-char))))
+    (when (eq (following-char) end-char)
+      (forward-char))
+    (nreverse items)))
+
+(defun gdb-mi--parse-c-string ()
+  "Parse a c-string."
+  (let ((start (point))
+        (pieces nil)
+        (octals-used nil))
+    (while (and (re-search-forward (rx (or ?\\ ?\")))
+                (not (eq (preceding-char) ?\")))
+      (push (buffer-substring start (1- (point))) pieces)
+      (cond
+       ((looking-at (rx (any "0-7") (? (any "0-7") (? (any "0-7")))))
+        (push (unibyte-string (string-to-number (match-string 0) 8)) pieces)
+        (setq octals-used t)
+        (goto-char (match-end 0)))
+       ((looking-at (rx (any "ntrvfab\"\\")))
+        (push (cdr (assq (following-char)
+                         '((?n . "\n")
+                           (?t . "\t")
+                           (?r . "\r")
+                           (?v . "\v")
+                           (?f . "\f")
+                           (?a . "\a")
+                           (?b . "\b")
+                           (?\" . "\"")
+                           (?\\ . "\\"))))
+              pieces)
+        (forward-char))
+       (t
+        (warn "Unrecognised escape char: %c" (following-char))))
+      (setq start (point)))
+    (push (buffer-substring start (1- (point))) pieces)
+    (let ((s (apply #'concat (nreverse pieces))))
+      (if (and octals-used gdb-mi-decode-strings)
+          (let ((coding
+                 (if (coding-system-p gdb-mi-decode-strings)
+                     gdb-mi-decode-strings
+                   (buffer-local-value
+                    'buffer-file-coding-system
+                    ;; FIXME: This is somewhat expensive.
+                    (gdb-get-buffer-create 'gdb-partial-output-buffer)))))
+            (decode-coding-string s coding))
+        s))))
+
+;; FIXME: Ideally this function should not be needed.
+(defun gdb-mi--c-string-from-string (string)
+  "Parse a c-string from (the beginning of) STRING."
+  (with-temp-buffer
+    (insert string)
+    (goto-char (1+ (point-min)))        ; Skip leading double quote.
+    (gdb-mi--parse-c-string)))
 
-If `default-directory' is remote, full file names are adapted accordingly."
-  (save-excursion
+(defun gdb-mi--parse-value ()
+  "Parse a value."
+  (cond
+   ((eq (following-char) ?\{)
+    (forward-char)
+    (gdb-mi--parse-tuple-or-list ?\}))
+   ((eq (following-char) ?\[)
+    (forward-char)
+    (gdb-mi--parse-tuple-or-list ?\]))
+   ((eq (following-char) ?\")
+    (forward-char)
+    (gdb-mi--parse-c-string))
+   (t (error "Bad start of result or value: %c" (following-char)))))
+
+(defun gdb-mi--parse-result-or-value ()
+  "Parse a result (key=value) or value."
+  (if (looking-at (rx (group (+ (any "a-zA-Z" ?_ ?-))) "="))
+      (progn
+        (goto-char (match-end 0))
+        (let* ((variable (intern (match-string 1)))
+               (value (gdb-mi--parse-value)))
+          (cons variable value)))
+    (gdb-mi--parse-value)))
+
+(defun gdb-mi--parse-results ()
+  "Parse zero or more result productions as a list."
+  (gdb-mi--parse-tuple-or-list nil))
+
+(defun gdb-mi--fix-key (key value)
+  "Convert any result (key-value pair) in VALUE whose key is KEY to its value."
+  (cond
+   ((atom value) value)
+   ((symbolp (car value))
+    (if (eq (car value) key)
+        (cdr value)
+      (cons (car value) (gdb-mi--fix-key key (cdr value)))))
+   (t (mapcar (lambda (x) (gdb-mi--fix-key key x)) value))))
+
+(defun gdb-mi--extend-fullname (remote value)
+  "Prepend REMOTE to any result string with `fullname' as the key in VALUE."
+  (cond
+   ((atom value) value)
+   ((symbolp (car value))
+    (if (and (eq (car value) 'fullname)
+             (stringp (cdr value)))
+        (cons 'fullname (concat remote (cdr value)))
+      (cons (car value) (gdb-mi--extend-fullname remote (cdr value)))))
+   (t (mapcar (lambda (x) (gdb-mi--extend-fullname remote x)) value))))
+
+(defun gdb-mi--read-buffer (fix-key)
+  "Parse the current buffer as a list of result productions.
+If FIX-KEY is a non-nil symbol, convert all FIX-KEY=VALUE results into VALUE.
+This is used to get rid of useless keys in lists in MI messages;
+eg, [key=.., key=..].  -stack-list-frames and -break-info are
+examples of MI commands which issue such responses."
+  (goto-char (point-min))
+  (let ((results (gdb-mi--parse-results)))
     (let ((remote (file-remote-p default-directory)))
       (when remote
-        (goto-char (point-min))
-        (while (re-search-forward "[\\[,]fullname=\"\\(.+\\)\"" nil t)
-          (replace-match (concat remote "\\1") nil nil nil 1))))
-    (goto-char (point-min))
+        (setq results (gdb-mi--extend-fullname remote results))))
     (when fix-key
-      (save-excursion
-        (while (re-search-forward (concat "[\\[,]\\(" fix-key "=\\)") nil t)
-          (replace-match "" nil nil nil 1))))
-    (when fix-list
-      (save-excursion
-        ;; Find positions of braces which enclose broken list
-        (while (re-search-forward (concat fix-list "={\"") nil t)
-          (let ((p1 (goto-char (- (point) 2)))
-                (p2 (progn (forward-sexp)
-                           (1- (point)))))
-            ;; Replace braces with brackets
-            (save-excursion
-              (goto-char p1)
-              (delete-char 1)
-              (insert "[")
-              (goto-char p2)
-              (delete-char 1)
-              (insert "]"))))))
-    (goto-char (point-min))
-    (insert "{")
-    (let ((re (concat "\\([[:alnum:]_-]+\\)=")))
-      (while (re-search-forward re nil t)
-        (replace-match "\"\\1\":" nil nil)
-        (if (eq (char-after) ?\") (forward-sexp) (forward-char))))
-    (goto-char (point-max))
-    (insert "}")))
-
-(defun gdb-json-read-buffer (&optional fix-key fix-list)
-  "Prepare and parse GDB/MI output in current buffer with `json-read'.
-
-FIX-KEY and FIX-LIST work as in `gdb-jsonify-buffer'."
-  (gdb-jsonify-buffer fix-key fix-list)
-  (save-excursion
-    (goto-char (point-min))
-    (let ((json-array-type 'list))
-      (json-read))))
+      (setq results (gdb-mi--fix-key fix-key results)))
+    results))
 
-(defun gdb-json-string (string &optional fix-key fix-list)
-  "Prepare and parse STRING containing GDB/MI output with `json-read'.
+(defun gdb-mi--from-string (string &optional fix-key)
+  "Prepare and parse STRING containing GDB/MI output.
 
-FIX-KEY and FIX-LIST work as in `gdb-jsonify-buffer'."
+FIX-KEY works as in `gdb-mi--read-buffer'."
   (with-temp-buffer
     (insert string)
-    (gdb-json-read-buffer fix-key fix-list)))
+    (gdb-mi--read-buffer fix-key)))
 
-(defun gdb-json-partial-output (&optional fix-key fix-list)
-  "Prepare and parse gdb-partial-output-buffer with `json-read'.
+(defun gdb-mi--partial-output (&optional fix-key)
+  "Prepare and parse gdb-partial-output-buffer.
 
-FIX-KEY and FIX-KEY work as in `gdb-jsonify-buffer'."
+FIX-KEY works as in `gdb-mi--read-buffer'."
   (with-current-buffer (gdb-get-buffer-create 'gdb-partial-output-buffer)
-    (gdb-json-read-buffer fix-key fix-list)))
+    (gdb-mi--read-buffer fix-key)))
 
 (defun gdb-line-posns (line)
   "Return a pair of LINE beginning and end positions."
@@ -3015,7 +3048,7 @@ def-gdb-trigger-and-handler
 
 (defun gdb-breakpoints-list-handler-custom ()
   (let ((breakpoints-list (gdb-mi--field
-                           (gdb-mi--field (gdb-json-partial-output 'bkpt)
+                           (gdb-mi--field (gdb-mi--partial-output 'bkpt)
                                           'BreakpointTable)
                            'body))
         (table (make-gdb-table)))
@@ -3338,7 +3371,7 @@ gdb-threads-mode
   'gdb-invalidate-threads)
 
 (defun gdb-thread-list-handler-custom ()
-  (let ((threads-list (gdb-mi--field (gdb-json-partial-output) 'threads))
+  (let ((threads-list (gdb-mi--field (gdb-mi--partial-output) 'threads))
         (table (make-gdb-table))
         (marked-line nil))
     (setq gdb-threads-list nil)
@@ -3579,7 +3612,7 @@ gdb-memory-column-width
       (error "Unknown format"))))
 
 (defun gdb-read-memory-custom ()
-  (let* ((res (gdb-json-partial-output))
+  (let* ((res (gdb-mi--partial-output))
          (err-msg (gdb-mi--field res 'msg)))
     (if (not err-msg)
         (let ((memory (gdb-mi--field res 'memory)))
@@ -3988,7 +4021,7 @@ gdb-disassembly-mode
   'gdb-invalidate-disassembly)
 
 (defun gdb-disassembly-handler-custom ()
-  (let* ((instructions (gdb-mi--field (gdb-json-partial-output) 'asm_insns))
+  (let* ((instructions (gdb-mi--field (gdb-mi--partial-output) 'asm_insns))
          (address (gdb-mi--field (gdb-current-buffer-frame) 'addr))
          (table (make-gdb-table))
          (marked-line nil))
@@ -4129,7 +4162,7 @@ gdb-frame-location
       (if res (concat " of " res) ""))))
 
 (defun gdb-stack-list-frames-custom ()
-  (let ((stack (gdb-mi--field (gdb-json-partial-output 'frame) 'stack))
+  (let ((stack (gdb-mi--field (gdb-mi--partial-output 'frame) 'stack))
         (table (make-gdb-table)))
     (set-marker gdb-stack-position nil)
     (dolist (frame stack)
@@ -4257,7 +4290,7 @@ gdb-edit-locals-value
 ;; Don't display values of arrays or structures.
 ;; These can be expanded using gud-watch.
 (defun gdb-locals-handler-custom ()
-  (let ((locals-list (gdb-mi--field (gdb-json-partial-output) 'locals))
+  (let ((locals-list (gdb-mi--field (gdb-mi--partial-output) 'locals))
         (table (make-gdb-table)))
     (dolist (local locals-list)
       (let ((name (gdb-mi--field local 'name))
@@ -4354,7 +4387,7 @@ gdb-frame-locals-buffer
 (defun gdb-registers-handler-custom ()
   (when gdb-register-names
     (let ((register-values
-           (gdb-mi--field (gdb-json-partial-output) 'register-values))
+           (gdb-mi--field (gdb-mi--partial-output) 'register-values))
           (table (make-gdb-table)))
       (dolist (register register-values)
         (let* ((register-number (gdb-mi--field register 'number))
@@ -4444,7 +4477,7 @@ gdb-get-changed-registers
 (defun gdb-changed-registers-handler ()
   (setq gdb-changed-registers nil)
   (dolist (register-number
-           (gdb-mi--field (gdb-json-partial-output) 'changed-registers))
+           (gdb-mi--field (gdb-mi--partial-output) 'changed-registers))
     (push register-number gdb-changed-registers)))
 
 (defun gdb-register-names-handler ()
@@ -4452,7 +4485,7 @@ gdb-register-names-handler
   ;; only once (in gdb-init-1)
   (setq gdb-register-names nil)
   (dolist (register-name
-           (gdb-mi--field (gdb-json-partial-output) 'register-names))
+           (gdb-mi--field (gdb-mi--partial-output) 'register-names))
     (push register-name gdb-register-names))
   (setq gdb-register-names (reverse gdb-register-names)))
 \f
@@ -4463,7 +4496,8 @@ gdb-get-source-file-list
 is set in them."
   (goto-char (point-min))
   (while (re-search-forward gdb-source-file-regexp nil t)
-    (push (read (match-string 1)) gdb-source-file-list))
+    (push (gdb-mi--c-string-from-string (match-string 1))
+          gdb-source-file-list))
   (dolist (buffer (buffer-list))
     (with-current-buffer buffer
       (when (member buffer-file-name gdb-source-file-list)
@@ -4479,7 +4513,7 @@ gdb-get-main-selected-frame
 (defun gdb-frame-handler ()
   "Set `gdb-selected-frame' and `gdb-selected-file' to show
 overlay arrow in source buffer."
-  (let ((frame (gdb-mi--field (gdb-json-partial-output) 'frame)))
+  (let ((frame (gdb-mi--field (gdb-mi--partial-output) 'frame)))
     (when frame
       (setq gdb-selected-frame (gdb-mi--field frame 'func))
       (setq gdb-selected-file (gdb-mi--field frame 'fullname))
@@ -4510,7 +4544,7 @@ gdb-get-prompt
   (goto-char (point-min))
   (setq gdb-prompt-name nil)
   (re-search-forward gdb-prompt-name-regexp nil t)
-  (setq gdb-prompt-name (read (match-string 1)))
+  (setq gdb-prompt-name (gdb-mi--c-string-from-string (match-string 1)))
   ;; Insert first prompt.
   (setq gdb-filter-output (concat gdb-filter-output gdb-prompt-name)))
 
@@ -4959,7 +4993,7 @@ gdb-get-source-file
   ;; This function is called only once on startup.
   (goto-char (point-min))
   (if (re-search-forward gdb-source-file-regexp nil t)
-      (setq gdb-main-file (read (match-string 1))))
+      (setq gdb-main-file (gdb-mi--c-string-from-string (match-string 1))))
   (if gdb-many-windows
       (gdb-setup-windows)
     (gdb-get-buffer-create 'gdb-breakpoints-buffer)
diff --git a/test/lisp/progmodes/gdb-mi-tests.el b/test/lisp/progmodes/gdb-mi-tests.el
new file mode 100644
index 0000000000..79493a571b
--- /dev/null
+++ b/test/lisp/progmodes/gdb-mi-tests.el
@@ -0,0 +1,44 @@
+;;; gdb-mi-tests.el --- tests for gdb-mi.el    -*- lexical-binding: t -*-
+
+;; Copyright (C) 2020 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs 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.
+
+;; GNU Emacs 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 GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+(require 'ert)
+(require 'gdb-mi)
+
+(ert-deftest gdb-mi-parse-value ()
+  ;; Test the GDB/MI result/value parser.
+  (should (equal
+           (gdb-mi--from-string
+            "alpha=\"ab\\ncd\",beta=[\"x\",{gamma=\"y\",delta=[]}]")
+           '((alpha . "ab\ncd")
+             (beta . ("x" ((gamma . "y") (delta . ())))))))
+  (should (equal
+           (gdb-mi--from-string
+            "alpha=\"ab\\ncd\",beta=[\"x\",{gamma=\"y\",delta=[]}]"
+            'gamma)
+           '((alpha . "ab\ncd")
+             (beta . ("x" ("y" (delta . ())))))))
+
+  (should (equal (gdb-mi--from-string "alpha=\"a\\303\\245b\"")
+                 `((alpha . ,(string-to-multibyte "a\303\245b")))))
+  (let ((gdb-mi-decode-strings 'utf-8))
+    (should (equal (gdb-mi--from-string "alpha=\"a\\303\\245b\"")
+                   '((alpha . "aåb")))))
+  )
+
+(provide 'gdb-mi-tests)
-- 
2.26.2

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Tue, 27 Oct 2020 19:16:55 +0100
> Cc: 44173@debbugs.gnu.org
> 
> Here is an updated patch, rebased after some basic code cleaning in gdb-mi.el.
> OK for master?

Yes, thanks.

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Mattias Engdegård]

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

31 okt. 2020 kl. 09.22 skrev Eli Zaretskii <eliz@gnu.org>:

> Yes, thanks.

Thank you, pushed.

The attached patch changes the default value of gdb-mi-decode-strings from nil to t.
I think we agree that it's probably a good idea, but leave the patch here in case there are comments on the documentation.


[-- Attachment #2: 0001-Change-the-default-value-of-gdb-mi-decode-strings-to.patch --]
[-- Type: application/octet-stream, Size: 2950 bytes --]

From 0b94246450d524a748dba28aefc2611c53ddf30c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sat, 31 Oct 2020 14:44:58 +0100
Subject: [PATCH] Change the default value of gdb-mi-decode-strings to t
 (bug#44173)

This is likely to be a more commonly wanted default value today.

* lisp/progmodes/gdb-mi.el (gdb-mi-decode-strings): Change default.
* doc/emacs/building.texi (Source Buffers): Update manual.
* etc/NEWS: Announce.
---
 doc/emacs/building.texi  | 13 ++++++-------
 etc/NEWS                 |  5 +++++
 lisp/progmodes/gdb-mi.el |  2 +-
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/doc/emacs/building.texi b/doc/emacs/building.texi
index 3e09f24322..23e22f46fb 100644
--- a/doc/emacs/building.texi
+++ b/doc/emacs/building.texi
@@ -1092,13 +1092,12 @@ Source Buffers
 more detail.
 
 @vindex gdb-mi-decode-strings
-  If the file names of the source files are shown with octal escapes,
-set the variable @code{gdb-mi-decode-strings} to the appropriate
-coding-system, most probably @code{utf-8}.  (This is @code{nil} by
-default because GDB may emit octal escapes in situations where
-decoding is undesirable, and also because the program being debugged
-might use an encoding different from the one used to encode non-ASCII
-file names on your system.)
+  By default, source file names and non-ASCII strings in the program
+being debugged are decoded using the default coding-system.  If you
+prefer a different decoding, perhaps because the program being
+debugged uses a different encoding, set the variable
+@code{gdb-mi-decode-strings} to the appropriate coding-system, or to
+@code{nil} to leave such characters as undecoded octal escapes.
 
 @node Breakpoints Buffer
 @subsubsection Breakpoints Buffer
diff --git a/etc/NEWS b/etc/NEWS
index a52122bcea..23b4deddf8 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -806,6 +806,11 @@ Now GDB only uses one source window to display source file by default.
 Customize 'gdb-max-source-window-count' to use more than one window.
 Control source file display by 'gdb-display-source-buffer-action'.
 
++++
+*** The default value of gdb-mi-decode-strings is now t.
+This means that the default coding-system is now used to decode strings
+and source file names from GDB.
+
 ** Gravatar
 
 ---
diff --git a/lisp/progmodes/gdb-mi.el b/lisp/progmodes/gdb-mi.el
index 4bebf88d35..6e9b6830a0 100644
--- a/lisp/progmodes/gdb-mi.el
+++ b/lisp/progmodes/gdb-mi.el
@@ -2455,7 +2455,7 @@ gdbmi-bnf-incomplete-record-result
 ;; files, values of string variables in the inferior, etc., are all
 ;; encoded in the same encoding.
 
-(defcustom gdb-mi-decode-strings nil
+(defcustom gdb-mi-decode-strings t
   "When non-nil, decode octal escapes in GDB output into non-ASCII text.
 
 If the value is a coding-system, use that coding-system to decode
-- 
2.21.1 (Apple Git-122.3)

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Mattias Engdegård]

31 okt. 2020 kl. 14.57 skrev Mattias Engdegård <mattiase@acm.org>:

> The attached patch changes the default value of gdb-mi-decode-strings from nil to t.

This patch has now been pushed since it did not appear controversial. Changes are of course still possible.
With that, the bug is closed.

bug#44173: 28.0.50; gdb-mi mangles strings with octal escapes

[Mattias Engdegård]

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

Here is the complete patch. It does not change the default value of
gdb-mi-decode-strings.


[-- Attachment #2: 0001-Parse-GDB-MI-results-directly-instead-of-going-via-J.patch --]
[-- Type: text/x-patch, Size: 23312 bytes --]

From 413b5e1ade51d9d4f1013649e03330d880004f23 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sun, 25 Oct 2020 12:40:36 +0100
Subject: [PATCH] Parse GDB/MI results directly instead of going via JSON
 (bug#44173)

Translating GDB/MI into JSON is an unnecessary and fragile detour
that made it hard to deal with octal escapes in strings correctly.
Parse GDB/MI directly instead.

* lisp/progmodes/gdb-mi.el (gdb-mi-decode-strings): Adjust doc string.
(gdb-mi-decode, gud-gdbmi-marker-filter): Remove gdb-mi-decode.
(gdb-jsonify-buffer): Remove.
(gdb-mi--parse-tuple-or-list, gdb-mi--parse-c-string)
(gdb-mi--parse-value, gdb-mi--parse-result-or-value)
(gdb-mi--parse-results, gdb-mi--fix-key, gdb-mi--extend-fullname):
New.
(gdb-json-read-buffer, gdb-json-string, gdb-json-partial-output):
Rename to gdb-mi--read-buffer, gdb-mi--from-string and
gdb-mi--partial-output respectively.  Remove useless FIX-LIST
argument.  FIX-KEY is now a symbol, not a string. All callers updated.
* test/lisp/progmodes/gdb-mi-tests.el: New file.
---
 lisp/progmodes/gdb-mi.el            | 290 +++++++++++++++-------------
 test/lisp/progmodes/gdb-mi-tests.el |  44 +++++
 2 files changed, 200 insertions(+), 134 deletions(-)
 create mode 100644 test/lisp/progmodes/gdb-mi-tests.el

diff --git a/lisp/progmodes/gdb-mi.el b/lisp/progmodes/gdb-mi.el
index 9e8af84a60..d31aef5790 100644
--- a/lisp/progmodes/gdb-mi.el
+++ b/lisp/progmodes/gdb-mi.el
@@ -89,7 +89,6 @@
 ;;; Code:
 
 (require 'gud)
-(require 'json)
 (require 'bindat)
 (require 'cl-lib)
 (require 'cl-seq)
@@ -167,7 +166,7 @@ gdb-threads-list
   "Associative list of threads provided by \"-thread-info\" MI command.
 
 Keys are thread numbers (in strings) and values are structures as
-returned from -thread-info by `gdb-json-partial-output'.  Updated in
+returned from -thread-info by `gdb-mi--partial-output'.  Updated in
 `gdb-thread-list-handler-custom'.")
 
 (defvar gdb-running-threads-count nil
@@ -186,7 +185,7 @@ gdb-breakpoints-list
   "Associative list of breakpoints provided by \"-break-list\" MI command.
 
 Keys are breakpoint numbers (in string) and values are structures
-as returned from \"-break-list\" by `gdb-json-partial-output'
+as returned from \"-break-list\" by `gdb-mi--partial-output'
 \(\"body\" field is used). Updated in
 `gdb-breakpoints-list-handler-custom'.")
 
@@ -1266,7 +1265,7 @@ gud-watch
       (message "gud-watch is a no-op in this mode."))))
 
 (defun gdb-var-create-handler (expr)
-  (let* ((result (gdb-json-partial-output)))
+  (let* ((result (gdb-mi--partial-output)))
     (if (not (bindat-get-field result 'msg))
         (let ((var
 	       (list (bindat-get-field result 'name)
@@ -1317,7 +1316,7 @@ gdb-var-list-children
 
 (defun gdb-var-list-children-handler (varnum)
   (let* ((var-list nil)
-	 (output (gdb-json-partial-output "child"))
+	 (output (gdb-mi--partial-output 'child))
 	 (children (bindat-get-field output 'children)))
     (catch 'child-already-watched
       (dolist (var gdb-var-list)
@@ -1392,7 +1391,7 @@ gdb-var-update
              'gdb-var-update))
 
 (defun gdb-var-update-handler ()
-  (let ((changelist (bindat-get-field (gdb-json-partial-output) 'changelist)))
+  (let ((changelist (bindat-get-field (gdb-mi--partial-output) 'changelist)))
     (dolist (var gdb-var-list)
       (setcar (nthcdr 5 var) nil))
     (let ((temp-var-list gdb-var-list))
@@ -2436,7 +2435,7 @@ gdbmi-bnf-incomplete-record-result
       is-complete)))
 
 
-; The following grammar rules are not yet implemented by this GDBMI-BNF parser.
+; The following grammar rules are not parsed directly by this GDBMI-BNF parser.
 ; The handling of those rules is currently done by the handlers registered
 ; in gdbmi-bnf-result-state-configs
 ;
@@ -2458,19 +2457,17 @@ gdbmi-bnf-incomplete-record-result
 ; list ==>
 ;      "[]" | "[" value ( "," value )* "]" | "[" result ( "," result )* "]"
 
+;; FIXME: This is fragile: it relies on the assumption that all the
+;; non-ASCII strings output by GDB, including names of the source
+;; files, values of string variables in the inferior, etc., are all
+;; encoded in the same encoding.
+
 (defcustom gdb-mi-decode-strings nil
   "When non-nil, decode octal escapes in GDB output into non-ASCII text.
 
 If the value is a coding-system, use that coding-system to decode
 the bytes reconstructed from octal escapes.  Any other non-nil value
-means to decode using the coding-system set for the GDB process.
-
-Warning: setting this non-nil might mangle strings reported by GDB
-that have literal substrings which match the \\nnn octal escape
-patterns, where nnn is an octal number between 200 and 377.  So
-we only recommend to set this variable non-nil if the program you
-are debugging really reports non-ASCII text, or some of its source
-file names include non-ASCII characters."
+means to decode using the coding-system set for the GDB process."
   :type '(choice
           (const :tag "Don't decode" nil)
           (const :tag "Decode using default coding-system" t)
@@ -2478,47 +2475,9 @@ gdb-mi-decode-strings
   :group 'gdb
   :version "25.1")
 
-;; The idea of the following function was suggested
-;; by Kenichi Handa <handa@gnu.org>.
-;;
-;; FIXME: This is fragile: it relies on the assumption that all the
-;; non-ASCII strings output by GDB, including names of the source
-;; files, values of string variables in the inferior, etc., are all
-;; encoded in the same encoding.  It also assumes that the \nnn
-;; sequences are not split between chunks of output of the GDB process
-;; due to buffering, and arrive together.  Finally, if some string
-;; included literal \nnn strings (as opposed to non-ASCII characters
-;; converted by GDB/MI to octal escapes), this decoding will mangle
-;; those strings.  When/if GDB acquires the ability to not
-;; escape-protect non-ASCII characters in its MI output, this kludge
-;; should be removed.
-(defun gdb-mi-decode (string)
-  "Decode octal escapes in MI output STRING into multibyte text."
-  (let ((coding
-         (if (coding-system-p gdb-mi-decode-strings)
-             gdb-mi-decode-strings
-           (with-current-buffer
-               (gdb-get-buffer-create 'gdb-partial-output-buffer)
-             buffer-file-coding-system))))
-    (with-temp-buffer
-      (set-buffer-multibyte nil)
-      (prin1 string (current-buffer))
-      (goto-char (point-min))
-      ;; prin1 quotes the octal escapes as well, which interferes with
-      ;; their interpretation by 'read' below.  Remove the extra
-      ;; backslashes to countermand that.
-      (while (re-search-forward "\\\\\\(\\\\[2-3][0-7][0-7]\\)" nil t)
-        (replace-match "\\1" nil nil))
-      (goto-char (point-min))
-      (decode-coding-string (read (current-buffer)) coding))))
-
 (defun gud-gdbmi-marker-filter (string)
   "Filter GDB/MI output."
 
-  ;; If required, decode non-ASCII text encoded with octal escapes.
-  (or (null gdb-mi-decode-strings)
-      (setq string (gdb-mi-decode string)))
-
   ;; Record transactions if logging is enabled.
   (when gdb-enable-debug
     (push (cons 'recv string) gdb-debug-log)
@@ -2565,7 +2524,7 @@ gdb-thread-created
 (defun gdb-thread-exited (_token output-field)
   "Handle =thread-exited async record.
 Unset `gdb-thread-number' if current thread exited and update threads list."
-  (let* ((thread-id (bindat-get-field (gdb-json-string output-field) 'id)))
+  (let* ((thread-id (bindat-get-field (gdb-mi--from-string output-field) 'id)))
     (if (string= gdb-thread-number thread-id)
         (gdb-setq-thread-number nil))
     ;; When we continue current thread and it quickly exits,
@@ -2579,7 +2538,7 @@ gdb-thread-selected
   "Handler for =thread-selected MI output record.
 
 Sets `gdb-thread-number' to new id."
-  (let* ((result (gdb-json-string output-field))
+  (let* ((result (gdb-mi--from-string output-field))
          (thread-id (bindat-get-field result 'id)))
     (gdb-setq-thread-number thread-id)
     ;; Typing `thread N' in GUD buffer makes GDB emit `^done' followed
@@ -2595,7 +2554,7 @@ gdb-thread-selected
 
 (defun gdb-running (_token output-field)
   (let* ((thread-id
-          (bindat-get-field (gdb-json-string output-field) 'thread-id)))
+          (bindat-get-field (gdb-mi--from-string output-field) 'thread-id)))
     ;; We reset gdb-frame-number to nil if current thread has gone
     ;; running. This can't be done in gdb-thread-list-handler-custom
     ;; because we need correct gdb-frame-number by the time
@@ -2624,7 +2583,7 @@ gdb-stopped
   "Given the contents of *stopped MI async record, select new
 current thread and update GDB buffers."
   ;; Reason is available with target-async only
-  (let* ((result (gdb-json-string output-field))
+  (let* ((result (gdb-mi--from-string output-field))
          (reason (bindat-get-field result 'reason))
          (thread-id (bindat-get-field result 'thread-id))
          (retval (bindat-get-field result 'return-value))
@@ -2780,83 +2739,146 @@ gdb-clear-partial-output
   (with-current-buffer (gdb-get-buffer-create 'gdb-partial-output-buffer)
     (erase-buffer)))
 
-(defun gdb-jsonify-buffer (&optional fix-key fix-list)
-  "Prepare GDB/MI output in current buffer for parsing with `json-read'.
-
-Field names are wrapped in double quotes and equal signs are
-replaced with semicolons.
-
-If FIX-KEY is non-nil, strip all \"FIX-KEY=\" occurrences from
-partial output.  This is used to get rid of useless keys in lists
-in MI messages, e.g.: [key=.., key=..].  -stack-list-frames and
--break-info are examples of MI commands which issue such
-responses.
-
-If FIX-LIST is non-nil, \"FIX-LIST={..}\" is replaced with
-\"FIX-LIST=[..]\" prior to parsing.  This is used to fix broken
--break-info output when it contains breakpoint script field
-incompatible with GDB/MI output syntax.
-
-If `default-directory' is remote, full file names are adapted accordingly."
-  (save-excursion
+;; Parse GDB/MI result records: this process converts
+;;  list      [...]      ->  list
+;;  tuple     {...}      ->  list
+;;  result    KEY=VALUE  ->  (KEY . VALUE) where KEY is a symbol
+;;  c-string  "..."      ->  string
+
+(defun gdb-mi--parse-tuple-or-list (end-char)
+  "Parse a tuple or list, either returned as a Lisp list.
+END-CHAR is the ending delimiter; will stop at end-of-buffer otherwise."
+  (let ((items nil))
+    (while (not (or (eobp)
+                    (eq (following-char) end-char)))
+      (let ((item (gdb-mi--parse-result-or-value)))
+        (push item items)
+        (when (eq (following-char) ?,)
+          (forward-char))))
+    (when (eq (following-char) end-char)
+      (forward-char))
+    (nreverse items)))
+
+(defun gdb-mi--parse-c-string ()
+  "Parse a c-string."
+  (let ((start (point))
+        (pieces nil)
+        (octals-used nil))
+    (while (and (re-search-forward (rx (or ?\\ ?\")))
+                (not (eq (preceding-char) ?\")))
+      (push (buffer-substring start (1- (point))) pieces)
+      (cond
+       ((looking-at (rx (any "0-7") (? (any "0-7") (? (any "0-7")))))
+        (push (unibyte-string (string-to-number (match-string 0) 8)) pieces)
+        (setq octals-used t)
+        (goto-char (match-end 0)))
+       ((looking-at (rx (any "ntrvfab\"\\")))
+        (push (cdr (assq (following-char)
+                         '((?n . "\n")
+                           (?t . "\t")
+                           (?r . "\r")
+                           (?v . "\v")
+                           (?f . "\f")
+                           (?a . "\a")
+                           (?b . "\b")
+                           (?\" . "\"")
+                           (?\\ . "\\"))))
+              pieces)
+        (forward-char))
+       (t
+        (warn "Unrecognised escape char: %c" (following-char))))
+      (setq start (point)))
+    (push (buffer-substring start (1- (point))) pieces)
+    (let ((s (apply #'concat (nreverse pieces))))
+      (if (and octals-used gdb-mi-decode-strings)
+          (let ((coding
+                 (if (coding-system-p gdb-mi-decode-strings)
+                     gdb-mi-decode-strings
+                   (buffer-local-value
+                    'buffer-file-coding-system
+                    ;; FIXME: This is somewhat expensive.
+                    (gdb-get-buffer-create 'gdb-partial-output-buffer)))))
+            (decode-coding-string s coding))
+        s))))
+
+(defun gdb-mi--parse-value ()
+  "Parse a value."
+  (cond
+   ((eq (following-char) ?\{)
+    (forward-char)
+    (gdb-mi--parse-tuple-or-list ?\}))
+   ((eq (following-char) ?\[)
+    (forward-char)
+    (gdb-mi--parse-tuple-or-list ?\]))
+   ((eq (following-char) ?\")
+    (forward-char)
+    (gdb-mi--parse-c-string))
+   (t (error "Bad start of result or value: %c" (following-char)))))
+
+(defun gdb-mi--parse-result-or-value ()
+  "Parse a result (key=value) or value."
+  (if (looking-at (rx (group (+ (any "a-zA-Z" ?_ ?-))) "="))
+      (progn
+        (goto-char (match-end 0))
+        (let* ((variable (intern (match-string 1)))
+               (value (gdb-mi--parse-value)))
+          (cons variable value)))
+    (gdb-mi--parse-value)))
+
+(defun gdb-mi--parse-results ()
+  "Parse zero or more result productions as a list."
+  (gdb-mi--parse-tuple-or-list nil))
+
+(defun gdb-mi--fix-key (key value)
+  "Convert any result (key-value pair) in VALUE whose key is KEY to its value."
+  (cond
+   ((atom value) value)
+   ((symbolp (car value))
+    (if (eq (car value) key)
+        (cdr value)
+      (cons (car value) (gdb-mi--fix-key key (cdr value)))))
+   (t (mapcar (lambda (x) (gdb-mi--fix-key key x)) value))))
+
+(defun gdb-mi--extend-fullname (remote value)
+  "Prepend REMOTE to any result string with `fullname' as the key in VALUE."
+  (cond
+   ((atom value) value)
+   ((symbolp (car value))
+    (if (and (eq (car value) 'fullname)
+             (stringp (cdr value)))
+        (cons 'fullname (concat remote (cdr value)))
+      (cons (car value) (gdb-mi--extend-fullname remote (cdr value)))))
+   (t (mapcar (lambda (x) (gdb-mi--extend-fullname remote x)) value))))
+
+(defun gdb-mi--read-buffer (fix-key)
+  "Parse the current buffer as a list of result productions.
+If FIX-KEY is a non-nil symbol, convert all FIX-KEY=VALUE results into VALUE.
+This is used to get rid of useless keys in lists in MI messages;
+eg, [key=.., key=..].  -stack-list-frames and -break-info are
+examples of MI commands which issue such responses."
+  (goto-char (point-min))
+  (let ((results (gdb-mi--parse-results)))
     (let ((remote (file-remote-p default-directory)))
       (when remote
-        (goto-char (point-min))
-        (while (re-search-forward "[\\[,]fullname=\"\\(.+\\)\"" nil t)
-          (replace-match (concat remote "\\1") nil nil nil 1))))
-    (goto-char (point-min))
+        (setq results (gdb-mi--extend-fullname remote results))))
     (when fix-key
-      (save-excursion
-        (while (re-search-forward (concat "[\\[,]\\(" fix-key "=\\)") nil t)
-          (replace-match "" nil nil nil 1))))
-    (when fix-list
-      (save-excursion
-        ;; Find positions of braces which enclose broken list
-        (while (re-search-forward (concat fix-list "={\"") nil t)
-          (let ((p1 (goto-char (- (point) 2)))
-                (p2 (progn (forward-sexp)
-                           (1- (point)))))
-            ;; Replace braces with brackets
-            (save-excursion
-              (goto-char p1)
-              (delete-char 1)
-              (insert "[")
-              (goto-char p2)
-              (delete-char 1)
-              (insert "]"))))))
-    (goto-char (point-min))
-    (insert "{")
-    (let ((re (concat "\\([[:alnum:]_-]+\\)=")))
-      (while (re-search-forward re nil t)
-        (replace-match "\"\\1\":" nil nil)
-        (if (eq (char-after) ?\") (forward-sexp) (forward-char))))
-    (goto-char (point-max))
-    (insert "}")))
-
-(defun gdb-json-read-buffer (&optional fix-key fix-list)
-  "Prepare and parse GDB/MI output in current buffer with `json-read'.
-
-FIX-KEY and FIX-LIST work as in `gdb-jsonify-buffer'."
-  (gdb-jsonify-buffer fix-key fix-list)
-  (save-excursion
-    (goto-char (point-min))
-    (let ((json-array-type 'list))
-      (json-read))))
+      (setq results (gdb-mi--fix-key fix-key results)))
+    results))
 
-(defun gdb-json-string (string &optional fix-key fix-list)
-  "Prepare and parse STRING containing GDB/MI output with `json-read'.
+(defun gdb-mi--from-string (string &optional fix-key)
+  "Prepare and parse STRING containing GDB/MI output.
 
-FIX-KEY and FIX-LIST work as in `gdb-jsonify-buffer'."
+FIX-KEY works as in `gdb-mi--read-buffer'."
   (with-temp-buffer
     (insert string)
-    (gdb-json-read-buffer fix-key fix-list)))
+    (gdb-mi--read-buffer fix-key)))
 
-(defun gdb-json-partial-output (&optional fix-key fix-list)
-  "Prepare and parse gdb-partial-output-buffer with `json-read'.
+(defun gdb-mi--partial-output (&optional fix-key)
+  "Prepare and parse gdb-partial-output-buffer.
 
-FIX-KEY and FIX-KEY work as in `gdb-jsonify-buffer'."
+FIX-KEY works as in `gdb-mi--read-buffer'."
   (with-current-buffer (gdb-get-buffer-create 'gdb-partial-output-buffer)
-    (gdb-json-read-buffer fix-key fix-list)))
+    (gdb-mi--read-buffer fix-key)))
 
 (defun gdb-line-posns (line)
   "Return a pair of LINE beginning and end positions."
@@ -3033,7 +3055,7 @@ def-gdb-trigger-and-handler
 
 (defun gdb-breakpoints-list-handler-custom ()
   (let ((breakpoints-list (bindat-get-field
-                           (gdb-json-partial-output "bkpt" "script")
+                           (gdb-mi--partial-output 'bkpt)
                            'BreakpointTable 'body))
         (table (make-gdb-table)))
     (setq gdb-breakpoints-list nil)
@@ -3355,7 +3377,7 @@ gdb-threads-mode
   'gdb-invalidate-threads)
 
 (defun gdb-thread-list-handler-custom ()
-  (let ((threads-list (bindat-get-field (gdb-json-partial-output) 'threads))
+  (let ((threads-list (bindat-get-field (gdb-mi--partial-output) 'threads))
         (table (make-gdb-table))
         (marked-line nil))
     (setq gdb-threads-list nil)
@@ -3592,7 +3614,7 @@ gdb-memory-column-width
       (error "Unknown format"))))
 
 (defun gdb-read-memory-custom ()
-  (let* ((res (gdb-json-partial-output))
+  (let* ((res (gdb-mi--partial-output))
          (err-msg (bindat-get-field res 'msg)))
     (if (not err-msg)
         (let ((memory (bindat-get-field res 'memory)))
@@ -4001,7 +4023,7 @@ gdb-disassembly-mode
   'gdb-invalidate-disassembly)
 
 (defun gdb-disassembly-handler-custom ()
-  (let* ((instructions (bindat-get-field (gdb-json-partial-output) 'asm_insns))
+  (let* ((instructions (bindat-get-field (gdb-mi--partial-output) 'asm_insns))
          (address (bindat-get-field (gdb-current-buffer-frame) 'addr))
          (table (make-gdb-table))
          (marked-line nil))
@@ -4142,7 +4164,7 @@ gdb-frame-location
       (if res (concat " of " res) ""))))
 
 (defun gdb-stack-list-frames-custom ()
-  (let ((stack (bindat-get-field (gdb-json-partial-output "frame") 'stack))
+  (let ((stack (bindat-get-field (gdb-mi--partial-output 'frame) 'stack))
         (table (make-gdb-table)))
     (set-marker gdb-stack-position nil)
     (dolist (frame stack)
@@ -4270,7 +4292,7 @@ gdb-edit-locals-value
 ;; Don't display values of arrays or structures.
 ;; These can be expanded using gud-watch.
 (defun gdb-locals-handler-custom ()
-  (let ((locals-list (bindat-get-field (gdb-json-partial-output) 'locals))
+  (let ((locals-list (bindat-get-field (gdb-mi--partial-output) 'locals))
         (table (make-gdb-table)))
     (dolist (local locals-list)
       (let ((name (bindat-get-field local 'name))
@@ -4367,7 +4389,7 @@ gdb-frame-locals-buffer
 (defun gdb-registers-handler-custom ()
   (when gdb-register-names
     (let ((register-values
-           (bindat-get-field (gdb-json-partial-output) 'register-values))
+           (bindat-get-field (gdb-mi--partial-output) 'register-values))
           (table (make-gdb-table)))
       (dolist (register register-values)
         (let* ((register-number (bindat-get-field register 'number))
@@ -4457,7 +4479,7 @@ gdb-get-changed-registers
 (defun gdb-changed-registers-handler ()
   (setq gdb-changed-registers nil)
   (dolist (register-number
-           (bindat-get-field (gdb-json-partial-output) 'changed-registers))
+           (bindat-get-field (gdb-mi--partial-output) 'changed-registers))
     (push register-number gdb-changed-registers)))
 
 (defun gdb-register-names-handler ()
@@ -4465,7 +4487,7 @@ gdb-register-names-handler
   ;; only once (in gdb-init-1)
   (setq gdb-register-names nil)
   (dolist (register-name
-           (bindat-get-field (gdb-json-partial-output) 'register-names))
+           (bindat-get-field (gdb-mi--partial-output) 'register-names))
     (push register-name gdb-register-names))
   (setq gdb-register-names (reverse gdb-register-names)))
 \f
@@ -4492,7 +4514,7 @@ gdb-get-main-selected-frame
 (defun gdb-frame-handler ()
   "Set `gdb-selected-frame' and `gdb-selected-file' to show
 overlay arrow in source buffer."
-  (let ((frame (bindat-get-field (gdb-json-partial-output) 'frame)))
+  (let ((frame (bindat-get-field (gdb-mi--partial-output) 'frame)))
     (when frame
       (setq gdb-selected-frame (bindat-get-field frame 'func))
       (setq gdb-selected-file (bindat-get-field frame 'fullname))
diff --git a/test/lisp/progmodes/gdb-mi-tests.el b/test/lisp/progmodes/gdb-mi-tests.el
new file mode 100644
index 0000000000..79493a571b
--- /dev/null
+++ b/test/lisp/progmodes/gdb-mi-tests.el
@@ -0,0 +1,44 @@
+;;; gdb-mi-tests.el --- tests for gdb-mi.el    -*- lexical-binding: t -*-
+
+;; Copyright (C) 2020 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs 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.
+
+;; GNU Emacs 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 GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+(require 'ert)
+(require 'gdb-mi)
+
+(ert-deftest gdb-mi-parse-value ()
+  ;; Test the GDB/MI result/value parser.
+  (should (equal
+           (gdb-mi--from-string
+            "alpha=\"ab\\ncd\",beta=[\"x\",{gamma=\"y\",delta=[]}]")
+           '((alpha . "ab\ncd")
+             (beta . ("x" ((gamma . "y") (delta . ())))))))
+  (should (equal
+           (gdb-mi--from-string
+            "alpha=\"ab\\ncd\",beta=[\"x\",{gamma=\"y\",delta=[]}]"
+            'gamma)
+           '((alpha . "ab\ncd")
+             (beta . ("x" ("y" (delta . ())))))))
+
+  (should (equal (gdb-mi--from-string "alpha=\"a\\303\\245b\"")
+                 `((alpha . ,(string-to-multibyte "a\303\245b")))))
+  (let ((gdb-mi-decode-strings 'utf-8))
+    (should (equal (gdb-mi--from-string "alpha=\"a\\303\\245b\"")
+                   '((alpha . "aåb")))))
+  )
+
+(provide 'gdb-mi-tests)
-- 
2.26.2