font-lock in machine mode of gdba

font-lock in machine mode of gdba

[Masatake YAMATO]

I've defined font-lock keywords for `gdb-assembler-mode'.  Please,
review the patch. I'm afraid the it is too decorative.

2004-10-18  Masatake YAMATO  <jet@gyve.org>

	* progmodes/gdb-ui.el (gdb-ann3): Define a key which toggles
	source view and assembler view.
	(gdb-assembler-font-lock-keywords): New font lock keywords
	definition.
	(gdb-assembler-mode): Use 'gdb-assembler-font-lock-keywords'.

Index: lisp/progmodes/gdb-ui.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/progmodes/gdb-ui.el,v
retrieving revision 1.24
diff -u -r1.24 gdb-ui.el
--- lisp/progmodes/gdb-ui.el	6 Oct 2004 07:20:53 -0000	1.24
+++ lisp/progmodes/gdb-ui.el	17 Oct 2004 15:23:05 -0000
@@ -1939,6 +1947,17 @@
     (suppress-keymap map)
     map))
 
+(defconst gdb-assembler-font-lock-keywords
+  '(("\\$?0x*[0-9a-f]+" . font-lock-constant-face)
+    ("<\\([^+>]+\\)\\+[0-9]+>:" 
+     1 font-lock-function-name-face)
+    ("%\\sw+" . font-lock-variable-name-face)
+    ("^\\(Dump of assembler code for function\\) \\(.+\\):" 
+     (1 font-lock-comment-face)
+     (2 font-lock-function-name-face))
+    ("^\\(End of assembler dump\\.\\)" . font-lock-comment-face))
+    "Font lock keywords used in `gdb-assembler-mode'.")
+
 (defun gdb-assembler-mode ()
   "Major mode for viewing code assembler.
 
@@ -1951,6 +1970,9 @@
   (setq fringes-outside-margins t)
   (setq buffer-read-only t)
   (use-local-map gdb-assembler-mode-map)
+  (make-local-variable 'font-lock-defaults)
+  (setq font-lock-defaults '(gdb-assembler-font-lock-keywords))
+  (unless font-lock-mode (font-lock-mode 1))
   (gdb-invalidate-assembler))
 
 (defun gdb-assembler-buffer-name ()

Re: font-lock in machine mode of gdba

[Stefan Monnier]

> +(defconst gdb-assembler-font-lock-keywords
> +  '(("\\$?0x*[0-9a-f]+" . font-lock-constant-face)
> +    ("<\\([^+>]+\\)\\+[0-9]+>:" 
> +     1 font-lock-function-name-face)
> +    ("%\\sw+" . font-lock-variable-name-face)
> +    ("^\\(Dump of assembler code for function\\) \\(.+\\):" 
> +     (1 font-lock-comment-face)
> +     (2 font-lock-function-name-face))
> +    ("^\\(End of assembler dump\\.\\)" . font-lock-comment-face))
> +    "Font lock keywords used in `gdb-assembler-mode'.")

Seems OK.
 
> +  (unless font-lock-mode (font-lock-mode 1))

This is WRONG.  The user should be able to choose whether he wants font-lock
or not.  He does that using either asm-mode-hook or global-font-lock-mode.


        Stefan

Re: font-lock in machine mode of gdba

[Nick Roberts]

Stefan Monnier writes:
 > > +(defconst gdb-assembler-font-lock-keywords
 > > +  '(("\\$?0x*[0-9a-f]+" . font-lock-constant-face)
 > > +    ("<\\([^+>]+\\)\\+[0-9]+>:" 
 > > +     1 font-lock-function-name-face)
 > > +    ("%\\sw+" . font-lock-variable-name-face)
 > > +    ("^\\(Dump of assembler code for function\\) \\(.+\\):" 
 > > +     (1 font-lock-comment-face)
 > > +     (2 font-lock-function-name-face))
 > > +    ("^\\(End of assembler dump\\.\\)" . font-lock-comment-face))
 > > +    "Font lock keywords used in `gdb-assembler-mode'.")
 > 
 > Seems OK.
 >  
 > > +  (unless font-lock-mode (font-lock-mode 1))
 > 
 > This is WRONG.  The user should be able to choose whether he wants font-lock
 > or not.  He does that using either asm-mode-hook or global-font-lock-mode.

asm-mode, asm-mode-font-lock-keywords, etc doesn't work because the
disassembly view differs from that of assembler code (it adds the machine
address at the start of each line, for one thing). Since this buffer is not
editable would font-lock-face be the right thing to use here?

Nick

font-lock in machine mode of gdba

[Nick Roberts]

 > I've defined font-lock keywords for `gdb-assembler-mode'.  Please,
 > review the patch. I'm afraid the it is too decorative.
 > 
 > 2004-10-18  Masatake YAMATO  <jet@gyve.org>
 > 
 > 	* progmodes/gdb-ui.el (gdb-ann3): Define a key which toggles
 > 	source view and assembler view.
 > 	(gdb-assembler-font-lock-keywords): New font lock keywords
 > 	definition.
 > 	(gdb-assembler-mode): Use 'gdb-assembler-font-lock-keywords'.
 >... 

I think it should at least be consistent with asm-mode where keywords like
movl, andl, etc are fontified with font-lock-keyword-face and operands and
registers are left unfontified.

Nick

Re: font-lock in machine mode of gdba

[Masatake YAMATO]

>  > I've defined font-lock keywords for `gdb-assembler-mode'.  Please,
>  > review the patch. I'm afraid the it is too decorative.
>  > 
>  > 2004-10-18  Masatake YAMATO  <jet@gyve.org>
>  > 
>  > 	* progmodes/gdb-ui.el (gdb-ann3): Define a key which toggles
>  > 	source view and assembler view.
>  > 	(gdb-assembler-font-lock-keywords): New font lock keywords
>  > 	definition.
>  > 	(gdb-assembler-mode): Use 'gdb-assembler-font-lock-keywords'.
>  >... 
> 
> I think it should at least be consistent with asm-mode where keywords like
> movl, andl, etc are fontified with font-lock-keyword-face and operands and
> registers are left unfontified.

I have changed both gdb-ui.el and asm-mode.el; and used the same face in the
both mode for the same object. Do you think operand fontification is too decorative?

Now font-lock-mode is turn-on in gdb-assembler-mode when global-font-lock-mode is 
true. gdb-assembler-mode-hook is also provided. See the comments at the tail of
font-core.el.

Index: lisp/progmodes/gdb-ui.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/progmodes/gdb-ui.el,v
retrieving revision 1.24
diff -u -r1.24 gdb-ui.el
--- lisp/progmodes/gdb-ui.el	6 Oct 2004 07:20:53 -0000	1.24
+++ lisp/progmodes/gdb-ui.el	19 Oct 2004 02:20:41 -0000
@@ -1939,6 +1960,19 @@
     (suppress-keymap map)
     map))
 
+(defvar gdb-assembler-font-lock-keywords
+  '(("[^\$]0x[0-9a-f]+" . font-lock-constant-face)
+    ("^\\(0x*[0-9a-f]+\\) ?\\(<\\(\\sw+\\)\\+[0-9]+>\\)?:[ \t]+\\(\\sw+\\)"
+     (1 font-lock-constant-face) 
+     (3 font-lock-function-name-face) 
+     (4 font-lock-keyword-face))
+    ("%\\sw+" . font-lock-variable-name-face)
+    ("^\\(Dump of assembler code for function\\) \\(.+\\):" 
+     (1 font-lock-comment-face)
+     (2 font-lock-function-name-face))
+    ("^\\(End of assembler dump\\.\\)" . font-lock-comment-face))
+  "Font lock keywords used in `gdb-assembler-mode'.")
+
 (defun gdb-assembler-mode ()
   "Major mode for viewing code assembler.
 
@@ -1951,7 +1985,12 @@
   (setq fringes-outside-margins t)
   (setq buffer-read-only t)
   (use-local-map gdb-assembler-mode-map)
-  (gdb-invalidate-assembler))
+  (set (make-local-variable 'font-lock-defaults)
+       '(gdb-assembler-font-lock-keywords))
+  (when global-font-lock-mode
+    (font-lock-mode 1))
+  (gdb-invalidate-assembler)
+  (run-hooks 'gdb-assembler-mode-hook))
 
 (defun gdb-assembler-buffer-name ()
   (with-current-buffer gud-comint-buffer
Index: lisp/progmodes/asm-mode.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/progmodes/asm-mode.el,v
retrieving revision 1.28
diff -u -r1.28 asm-mode.el
--- lisp/progmodes/asm-mode.el	1 Sep 2003 15:45:34 -0000	1.28
+++ lisp/progmodes/asm-mode.el	19 Oct 2004 02:20:41 -0000
@@ -90,7 +90,10 @@
     2 font-lock-keyword-face)
    ;; directive started from ".".
    ("^\\(\\.\\(\\sw\\|\\s_\\)+\\)\\>[^:]?"
-    1 font-lock-keyword-face))
+    1 font-lock-keyword-face)
+   ;; %register
+   ("%\\sw+" . font-lock-variable-name-face)
+   )
  "Additional expressions to highlight in Assembler mode.")
 
 ;;;###autoload

Re: font-lock in machine mode of gdba

[Stefan Monnier]

> Now font-lock-mode is turn-on in gdb-assembler-mode when
> global-font-lock-mode is  true. gdb-assembler-mode-hook is also
> provided. See the comments at the tail of font-core.el.

This is still wrong (well, maybe not "wrong", but...).  Your patch should
only set font-lock-defaults and nothing more.  It should be all
that's needed.  If it's not enough, then there's a bug elsewhere (typically
an omission to call `kill-all-local-variables).


        Stefan

Re: font-lock in machine mode of gdba

[Nick Roberts]

Stefan Monnier writes:
 > > Now font-lock-mode is turn-on in gdb-assembler-mode when
 > > global-font-lock-mode is  true. gdb-assembler-mode-hook is also
 > > provided. See the comments at the tail of font-core.el.
 > 
 > This is still wrong (well, maybe not "wrong", but...).  Your patch should
 > only set font-lock-defaults and nothing more.  It should be all
 > that's needed.  If it's not enough, then there's a bug elsewhere (typically
 > an omission to call `kill-all-local-variables).

Currently these modes can't run kill-all-local-variables. See the FIXME in
gdb-get-create-buffer (which I think you put there a long while ago):

	  ;; FIXME: This should be set after calling the function, since the
	  ;; function should run kill-all-local-variables.

In gdb-get-create-buffer, gdb-buffer-type must currently be set *before*
the mode function because it (the mode function) runs a trigger (like
gdb-invalidate-assembler). This trigger needs to be moved out of the
mode function and into gdb-get-create-buffer. To do this gdb-get-create-buffer
needs to know the name of the trigger. How about something like this:

(defun gdb-get-create-buffer (key)
  "Create a new gdb  buffer of the type specified by KEY.
The key should be one of the cars in `gdb-buffer-rules-assoc'."
  (or (gdb-get-buffer key)
      (let* ((rules (assoc key gdb-buffer-rules-assoc))
	     (name (funcall (gdb-rules-name-maker rules)))
	     (new (get-buffer-create name)))
	(with-current-buffer new
	  (let ((trigger))	  
	    (if (cdr (cdr rules))
		(setq trigger (funcall (car (cdr (cdr rules))))))
	    (set (make-local-variable 'gdb-buffer-type) key)
	    (set (make-local-variable 'gud-minor-mode)
		 (with-current-buffer gud-comint-buffer gud-minor-mode))
	    (set (make-local-variable 'tool-bar-map) gud-tool-bar-map)
	    (if trigger (funcall trigger)))
	  new))))

So that gdb-assembler-mode would look like:

(defun gdb-assembler-mode ()
  "Major mode for viewing code assembler.

\\{gdb-assembler-mode-map}"
  (kill-all-local-variables)
  (setq major-mode 'gdb-assembler-mode)
  (setq mode-name "Machine")
  (setq gdb-overlay-arrow-position nil)
  (add-to-list 'overlay-arrow-variable-list 'gdb-overlay-arrow-position)
  (put 'gdb-overlay-arrow-position 'overlay-arrow-string "=>")
  (setq fringes-outside-margins t)
  (setq buffer-read-only t)
  (use-local-map gdb-assembler-mode-map)
  (gdb-invalidate-assembler)
  (set (make-local-variable 'font-lock-defaults)
       '(gdb-assembler-font-lock-keywords))
  (run-mode-hooks 'gdb-assembler-mode-hook)
  'gdb-invalidate-assembler)

and similarly for the other mode functions. This appears to work and so
does font-lock in the assembler buffer. I'm not looking for points for style,
just confirmation that I'm making sense and that run-mode-hooks doesn't
have to be the last item in the mode function.

Nick

Re: font-lock in machine mode of gdba

[Nick Roberts]

 > > I think it should at least be consistent with asm-mode where keywords like
 > > movl, andl, etc are fontified with font-lock-keyword-face and operands and
 > > registers are left unfontified.
 > 
 > I have changed both gdb-ui.el and asm-mode.el; and used the same face in the
 > both mode for the same object. Do you think operand fontification is too decorative?
 > 
 > Now font-lock-mode is turn-on in gdb-assembler-mode when global-font-lock-mode is 
 > true. gdb-assembler-mode-hook is also provided. See the comments at the tail of
 > font-core.el.

I don't know if the operand fontification too decorative but is it logical?
Should a register name have font-lock-variable-face and an address
font-lock-constant-face? I don't use disassembly for debugging or code in
assembler. Perhaps someone who does can comment.

Would gdb-assembler-mode-hook ever be used?

Nick

Re: font-lock in machine mode of gdba

[Stefan Monnier]

> Would gdb-assembler-mode-hook ever be used?

gdb-assembler-mode is a mjor mode, so it should run gdb-assembler-mode-hook.
Actually it should run it with `run-mode-hooks' rather than with
`run-hooks'.

Whether it will ever be used is not very important.  I can imagine someone
using it to change the font-lock keywords or to turn font-lock OFF (or ON)
in this particular kind of buffer, or to change the
`truncate-lines' settings, or to ...


        Stefan

Re: font-lock in machine mode of gdba

[Masatake YAMATO]

> Whether it will ever be used is not very important.  I can imagine someone
> using it to change the font-lock keywords or to turn font-lock OFF (or ON)
> in this particular kind of buffer, 

Yes. font-lock control is what I intended.

Re: font-lock in machine mode of gdba

[Masatake YAMATO]

>  > > I think it should at least be consistent with asm-mode where keywords like
>  > > movl, andl, etc are fontified with font-lock-keyword-face and operands and
>  > > registers are left unfontified.
>  > 
>  > I have changed both gdb-ui.el and asm-mode.el; and used the same face in the
>  > both mode for the same object. Do you think operand fontification is too decorative?
>  > 
>  > Now font-lock-mode is turn-on in gdb-assembler-mode when global-font-lock-mode is 
>  > true. gdb-assembler-mode-hook is also provided. See the comments at the tail of
>  > font-core.el.
> 
> I don't know if the operand fontification too decorative but is it logical?
> Should a register name have font-lock-variable-face and an address
> font-lock-constant-face? I don't use disassembly for debugging or code in
> assembler. Perhaps someone who does can comment.

I'm using disassembly for hacking and understanding glibc's
/lib/ld-linux.so.2 on i386.  (gdba is extremely useful.)  I feel my
definition is not so bad. The face usage is consistent with asm-mode.

BTW, What I really need is fontification on x/i commands output.

Masatake YAMATO

Re: font-lock in machine mode of gdba

[Nick Roberts]

 > I'm using disassembly for hacking and understanding glibc's
 > /lib/ld-linux.so.2 on i386.  (gdba is extremely useful.)  I feel my
 > definition is not so bad. The face usage is consistent with asm-mode.

OK. I've installed these changes, trying to bear in mind Stefan's comments.
Note, however, that your fontification (gdb-assembler-font-lock-keywords)
doesn't seem to work for system library functions e.g:

0x40062155 <__libc_start_main+133>:	push   %edi

On my system (Mandrake 9.2) you can get this by breaking any C prgram in
main and going up a level (you might have to click on the GUD buffer to get
fontification).

Nick

Re: font-lock in machine mode of gdba

[Masatake YAMATO]

Hi,

>  > I'm using disassembly for hacking and understanding glibc's
>  > /lib/ld-linux.so.2 on i386.  (gdba is extremely useful.)  I feel my
>  > definition is not so bad. The face usage is consistent with asm-mode.
> 
> OK. I've installed these changes, trying to bear in mind Stefan's comments.
> Note, however, that your fontification (gdb-assembler-font-lock-keywords)
> doesn't seem to work for system library functions e.g:
> 
> 0x40062155 <__libc_start_main+133>:	push   %edi
> 
> On my system (Mandrake 9.2) you can get this by breaking any C prgram in
> main and going up a level (you might have to click on the GUD buffer to get
> fontification).
> 
> Nick

I could reproduce the bug.

In my last kewords definition, underscores in a function name was not handled.
Now underscores and periods are handled.

I've changed my mind: the addresses have no fontification becuase finally I think 
too decorative.

The address fontification will be worth if other gud-ui buffers
introduce font-lock; the awareness of address string in the buffers
will be improved by using the same face on the all gud-ui buffers.
However, I have not used other buffers of gud-ui well yet, I should
not touch more. Switch view between source and machine code by
keyboard is much more important:)

Masatake YAMATO

Index: lisp/progmodes/gdb-ui.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/progmodes/gdb-ui.el,v
retrieving revision 1.25
diff -u -r1.25 gdb-ui.el
--- lisp/progmodes/gdb-ui.el	23 Oct 2004 22:00:48 -0000	1.25
+++ lisp/progmodes/gdb-ui.el	23 Oct 2004 23:34:03 -0000
@@ -1950,13 +1981,15 @@
     map))
 
 (defvar gdb-assembler-font-lock-keywords
-  '(("[^\$]0x[0-9a-f]+" . font-lock-constant-face)
-    ("^\\(0x*[0-9a-f]+\\) ?\\(<\\(\\sw+\\)\\+[0-9]+>\\)?:[ \t]+\\(\\sw+\\)"
-     (1 font-lock-constant-face) 
-     (3 font-lock-function-name-face) 
+  '(;; <__function.name+n>
+    ("<\\(\\(\\sw\\|[_.]\\)+\\)\\(\\+[0-9]+\\)?>"
+     (1 font-lock-function-name-face))
+    ;; 0xNNNNNNNN <__function.name+n>: opcode
+    ("^0x[0-9a-f]+ \\(<\\(\\(\\sw\\|[_.]\\)+\\)\\+[0-9]+>\\)?:[ \t]+\\(\\sw+\\)"
      (4 font-lock-keyword-face))
+    ;; %register(at least i386)
     ("%\\sw+" . font-lock-variable-name-face)
-    ("^\\(Dump of assembler code for function\\) \\(.+\\):" 
+    ("^\\(Dump of assembler code for function\\) \\(.+\\):"
      (1 font-lock-comment-face)
      (2 font-lock-function-name-face))
     ("^\\(End of assembler dump\\.\\)" . font-lock-comment-face))

Re: font-lock in machine mode of gdba

[Nick Roberts]

 > I could reproduce the bug.
 > 
 > In my last kewords definition, underscores in a function name was not handled.
 > Now underscores and periods are handled.
 > 
 > I've changed my mind: the addresses have no fontification becuase finally I think 
 > too decorative.

Please install these kinds of changes directly, you don't need to refer them
to me.

 > The address fontification will be worth if other gud-ui buffers
 > introduce font-lock; the awareness of address string in the buffers
 > will be improved by using the same face on the all gud-ui buffers.
 > However, I have not used other buffers of gud-ui well yet, I should
 > not touch more. Switch view between source and machine code by
 > keyboard is much more important:)

In general, hat might be a good idea but note that I've specifically turned
font-lock off in the frames buffer. That seemed to be necessary to get
inverse video for the current frame, although that might not be the case now
kill-all-local-variables is used.

Nick