Re: [PATCH 2/6] gdb: move display of completion results into completion_result class

[Andrew Burgess <aburgess@redhat.com>]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: lsix@lancelotsix.com, gdb-patches@sourceware.org
>> Date: Fri, 12 Apr 2024 23:20:33 +0100
>> 
>> OK, so now the bad news.
>> 
>> My patched GDB doesn't "just work" with emacs.
>> 
>> Remember though, an unpatched GDB doesn't work either when trying to
>> complete a filename with spaces in, so it's not that I've made things
>> worse.
>> 
>> I tracked at least one bug down to this function in gud.el:
>> 
>>   (defun gud-gdb-completion-at-point ()
>>     "Return the data to complete the GDB command before point."
>>     (let ((end (point))
>>           (start
>>            (save-excursion
>>              (skip-chars-backward "^ " (comint-line-beginning-position))
>>              (point))))
>>       ;; FIXME: `gud-gdb-run-command-fetch-lines' has some nasty side-effects on
>>       ;; the buffer (via `gud-delete-prompt-marker'): it removes the prompt and
>>       ;; then re-adds it later, thus messing up markers and overlays along the
>>       ;; way (bug#18282).
>>       ;; We use an "insert-before" marker for `start', since it's typically right
>>       ;; after the prompt, which works around the problem, but is a hack (and
>>       ;; comes with other downsides, e.g. if completion adds text at `start').
>>       (list (copy-marker start t) end
>>             (completion-table-dynamic
>>              (apply-partially gud-gdb-completion-function
>>                               (buffer-substring (comint-line-beginning-position)
>>                                                 start))))))
>>                                                 
>> This is trying to figure out everything before the completion word on
>> the current line and doing the apply-partial with this context on
>> gud-gdb-completion-function.
>> 
>> So if we are completing 'file /tmp/abc<tab>' then we'd end up doing:
>> 
>>   (apply-partial gud-gdb-completion-function "file ")
>> 
>> The completion context is calculated as running from the start of the
>> line up to 'start', which is calculated in the 'let'.  And 'start' is
>> calculated by starting from 'point' and moving backward to the first
>> whitespace.
>> 
>> That's not going to work for completing any line that contains a space.
>> 
>> I think this function would need to get smarter to understand about
>> escaping and quoting.
>
> Yes.  Would you mind submitting a bug report about that, using the
> command "M-x report-emacs-bug"?

I absolutely will file the bug report once this series (or whatever this
series becomes) is merged.  However, I wanted to see if there were
additional bugs hiding in the emacs code, these might indicate changes
that need to be made (or not made) on the GDB side.

So the good news is, that with a little hacking I managed to get emacs
working!

Disclaimer: My emacs lisp knowledge is pretty weak, so please consider
this a proof of concept rather than code I'm actually suggesting should
be merged into emacs.  When I file the bug I'll include this code again,
but by adding it here we can hopefully see what needs to change.

The new and updated emacs code is at the end of this email if anyone
wants to play with emacs and this new completion.  Drop this new code
into a file, load the emacs 'gdb' mode, then eval the new code.  This
adds some new functions, and replaces one existing function.

With this in place I now get completion for filenames containing
whitespace.  Given this directory tree:

  /tmp/eee/
  ├── aa bb
  │   ├── f1
  │   └── f2
  └── aa cc
      ├── g1
      └── g2

  (gdb) file /tmp/eee/aa<TAB>
    =>  file /tmp/eee/aa\ <TAB>
  Possible completions are:		<--- These are offered in a new buffer.
  /tmp/eee/aa\ bb/			<--- I select this entry.
  /tmp/eee/aa\ cc/
    => file /tmp/eee/aa\ bb/<TAB>
    => file /tmp/eee/aa\ bb/f<TAB>
  Possible completions are:		<--- These are offered in a new buffer.
  /tmp/eee/aa\ bb/f1
  /tmp/eee/aa\ bb/f2			<--- I select this entry.
    => file /tmp/eee/aa\ bb/f2<ENTER>
  ... gdb tries to load the file ...

And if also works with quoting:

  (gdb) file "/tmp/eee/a<TAB>
    =>  file "/tmp/eee/aa <TAB>
  Possible completions are:		<--- These are offered in a new buffer.
  "/tmp/eee/aa bb/
  "/tmp/eee/aa cc/			<--- I select this entry.
    =>  file "/tmp/eee/aa cc/<TAB>
    =>  file "/tmp/eee/aa cc/g<TAB>
  Possible completions are:		<--- These are offered in a new buffer.
  "/tmp/eee/aa cc/g1"			<--- I select this entry.
  "/tmp/eee/aa cc/g2"
    =>  file "/tmp/eee/aa cc/g1"<ENTER>
 ... gdb tries to load the file ...

As I said, I'm not claiming that emacs is "fixed", but I think I've
shown what needs to be done to get this working, and also I've
satisfied myself that everything can be made to work.

Thanks,
Andrew



---

;; New function: Return true if the character at POS is escaped, that
;; is, is the character as POS preceeded by a backslash.
(defun gud-gdb-is-char-escaped (pos)
  (char-equal ?\\ (char-before pos)))

;; New function: Move point forward as skip-chars-forward does,
;; passing CHARS and LIMIT to skip-chars-forward.  However, after
;; calling skip-chars-forward, if the character we are looking at is
;; escaped (with a backslash) then skip that character, and call
;; skip-chars-forward again.
(defun gud-gdb-skip-to-unquoted (chars limit)
  (while (and (< (point) limit)
              (progn
                (skip-chars-forward chars limit)
                (if (gud-gdb-is-char-escaped (point))
                    (progn
                      (forward-char)
                      t)
                  nil)))))

;; New function: Move point forward skipping over a 'word'.  A word
;; can be quoted, starts with a single or double quote, in which case
;; the corresponding quote marks the end of the word.  Or a word can
;; be unquoted, in which case the word ends at the next white space.
;;
;; Never move forward past LIMIT.
;;
;; In an unquoted word white space can be escaped.
;;
;; In a double quoted word, double quotes can be escaped.
(defun gud-gdb-forward-maybe-quoted-word (limit)
  (cond ((char-equal ?\' (char-after))
         (progn
           (forward-char)
           (skip-chars-forward "^'" limit)))
        ((char-equal ?\" (char-after))
         (progn
           (forward-char)
           (gud-gdb-skip-to-unquoted "^\"" limit)))
        (t
         (progn
           (gud-gdb-skip-to-unquoted "^[:space:]" limit)))))

;; Assuming point is at the start of a line, return the position for
;; the start of a completion word.  The point will be moved by calling
;; this function.
;;
;; Never search past LIMIT.
;;
;; The completion word is the last word on the line, the word might be
;; quoted though.
(defun gud-gdb-find-completion-word (limit)
  (let ((completion-word-start nil))
    (while (< (point) limit)
      (setf completion-word-start (point))
      (gud-gdb-forward-maybe-quoted-word limit)
      (skip-chars-forward " " limit))

    completion-word-start))

;; This replaces an existing emacs function.  I've changed the logic
;; for finding the completion word.
(defun gud-gdb-completion-at-point ()
  "Return the data to complete the GDB command before point."
  (let* ((end (point))
        (start
         (save-excursion
           (goto-char (comint-line-beginning-position))
           (gud-gdb-find-completion-word end))))
    ;; FIXME: `gud-gdb-run-command-fetch-lines' has some nasty side-effects on
    ;; the buffer (via `gud-delete-prompt-marker'): it removes the prompt and
    ;; then re-adds it later, thus messing up markers and overlays along the
    ;; way (bug#18282).
    ;; We use an "insert-before" marker for `start', since it's typically right
    ;; after the prompt, which works around the problem, but is a hack (and
    ;; comes with other downsides, e.g. if completion adds text at `start').
    (list (copy-marker start t) end
          (completion-table-dynamic
           (apply-partially gud-gdb-completion-function
                            (buffer-substring (comint-line-beginning-position)
                                              start))))))