RE: symbolic debug of loadable modules with kgdb light

[Caz Yokoyama <cazyokoyama at gmail dot com>]

Hello Joel,
Here is the revised patch according to you.
ndex: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.10902
diff -u -r1.10902 ChangeLog
--- gdb/ChangeLog	23 Sep 2009 17:27:39 -0000	1.10902
+++ gdb/ChangeLog	24 Sep 2009 16:33:18 -0000
@@ -1,3 +1,9 @@
+2009-09-23  Kazuyoshi Caz Yokoyama  <caz@caztech.com>
+
+	* remote.c: Allow the user to select one of ^C, a break or
+	Magic SysRq g as the sequence to the remote in order to
+	interrupt the execution.
+
 2009-09-23  John Wright  <john.wright@hp.com>
 
 	PR gdb/10684:
Index: gdb/remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.372
diff -u -r1.372 remote.c
--- gdb/remote.c	10 Sep 2009 22:47:56 -0000	1.372
+++ gdb/remote.c	24 Sep 2009 16:33:21 -0000
@@ -546,13 +546,48 @@
    this can go away.  */
 static int wait_forever_enabled_p = 1;
 
+/* Allow the user to specify what sequence to send to the remote
+   when he requests a program interruption: Although ^C is usually
+   what remote systems expect (this is the default, here), it is
+   sometimes preferable to send a break.  On other systems such
+   as the Linux kernel, a break followed by g, which is Magic SysRq g
+   is required in order to interrupt the execution.  */
+const char remote_break_interrupt[] = "interrutpt";
+const char remote_break_break[] = "break";
+const char remote_break_sysrq_g[] = "sysrq-g";
+static const char *remote_break_modes[] =
+  {
+    remote_break_interrupt,
+    remote_break_break,
+    remote_break_sysrq_g,
+    NULL
+  };
+const char *remote_break_mode = remote_break_interrupt;
 
-/* This variable chooses whether to send a ^C or a break when the user
-   requests program interruption.  Although ^C is usually what remote
-   systems expect, and that is the default here, sometimes a break is
-   preferable instead.  */
-
-static int remote_break;
+static void show_remotebreak (struct ui_file *file, int from_tty,
+			      struct cmd_list_element *c,
+			      const char *value)
+{
+  if (remote_break_mode == remote_break_interrupt)
+    fprintf_filtered (file,
+		      ("To interrupt the execution of the program, "
+		       "send the ASCII ETX character (Ctrl-c) "
+		       "to the remote target."));
+  else if (remote_break_mode == remote_break_break)
+    fprintf_filtered (file,
+		      ("To interrupt the execution of the program, "
+		       "send a BREAK signal "
+		       "to the remote target."));
+  else if (remote_break_mode == remote_break_sysrq_g)
+    fprintf_filtered (file,
+		      ("To interrupt the execution of Linux kernel, "
+		       "send a BREAK signal and 'g' "
+		       "to the remote Linux kernel."));
+  else
+    fprintf_filtered (file,
+		      ("You are sending %s to the remote target "
+		       "which is not expected."), remote_break_mode);
+}
 
 /* Descriptor for I/O to remote machine.  Initialize it to NULL so that
    remote_open knows that we don't have a file open when the program
@@ -2598,6 +2633,14 @@
   /* Ack any packet which the remote side has already sent.  */
   serial_write (remote_desc, "+", 1);
 
+  /* Send the Magic SysRg g sequence in order to interrupt
+     the execution of Linux kernel.  */
+  if (remote_break_mode == remote_break_sysrq_g)
+    {
+    serial_send_break (remote_desc);
+    serial_write (remote_desc, "g", 1);
+    }
+
   /* The first packet we send to the target is the optional "supported
      packets" request.  If the target can answer this, it will tell us
      which later probes to skip.  */
@@ -4021,12 +4064,20 @@
   if (rs->cached_wait_status)
     return;
 
-  /* Send a break or a ^C, depending on user preference.  */
-
-  if (remote_break)
-    serial_send_break (remote_desc);
-  else
-    serial_write (remote_desc, "\003", 1);
+  /* Send ^C, a break or a break g, depending on user preference.  */
+  if (remote_break_mode == remote_break_interrupt)
+    {
+      serial_write (remote_desc, "\003", 1);
+    }
+  else if (remote_break_mode == remote_break_break)
+    {
+      serial_send_break (remote_desc);
+    }
+  else if (remote_break_mode == remote_break_sysrq_g)
+    {
+      serial_send_break (remote_desc);
+      serial_write (remote_desc, "g", 1);
+    }
 }
 
 /* This is the generic stop called via the target vector. When a target
@@ -9056,12 +9107,12 @@
 terminating `#' character and checksum."),
 	   &maintenancelist);
 
-  add_setshow_boolean_cmd ("remotebreak", no_class, &remote_break, _("\
-Set whether to send break if interrupted."), _("\
-Show whether to send break if interrupted."), _("\
-If set, a break, instead of a cntrl-c, is sent to the remote target."),
-			   NULL, NULL, /* FIXME: i18n: Whether to send break
if interrupted is %s.  */
-			   &setlist, &showlist);
+  add_setshow_enum_cmd ("remotebreak", class_support,
+			remote_break_modes, &remote_break_mode, _("\
+Set remote break sequence."), _("\
+Show remote break sequence."), NULL,
+			NULL, show_remotebreak,
+			&setlist, &showlist);
 
   /* Install commands for configuring memory read/write packets.  */
 
Index: gdb/remote.h
===================================================================
RCS file: /cvs/src/src/gdb/remote.h,v
retrieving revision 1.17
diff -u -r1.17 remote.h
--- gdb/remote.h	3 Jan 2009 05:57:53 -0000	1.17
+++ gdb/remote.h	24 Sep 2009 16:33:21 -0000
@@ -77,4 +77,7 @@
 
 int remote_filename_p (const char *filename);
 
+extern const char remote_break_sysrq_g[];
+extern const char *remote_break_mode;
+
 #endif
-caz

-----Original Message-----
From: Caz Yokoyama [mailto:caz@caztech.com] 
Sent: Wednesday, September 23, 2009 4:36 AM
To: 'Joel Brobecker'
Cc: 'Pedro Alves'; 'gdb-patches@sourceware.org'
Subject: RE: symbolic debug of loadable modules with kgdb light

I prefer "Magic SysRq" instead of "Magic SysReq" because keyboard has
"SysRq".
-caz

-----Original Message-----
From: Caz Yokoyama [mailto:caz@caztech.com] 
Sent: Tuesday, September 22, 2009 9:16 PM
To: 'Joel Brobecker'
Cc: 'Pedro Alves'; 'gdb-patches@sourceware.org'
Subject: RE: symbolic debug of loadable modules with kgdb light

Hello Joel,
See below for my comments which are under -----.
-caz

-----Original Message-----
From: Joel Brobecker [mailto:brobecker@adacore.com] 
Sent: Tuesday, September 22, 2009 5:48 PM
To: Caz Yokoyama
Cc: 'Pedro Alves'; gdb-patches@sourceware.org
Subject: Re: symbolic debug of loadable modules with kgdb light

Global Maintainers (and other contributors),

This patch proposes to change "set remotebreak" from being an on/off
setting (on = send BREAK to interrupt, while "off" = send Ctrl-c to
interrupt), to becoming an enum setting with 3 values:
  1. the first value (the default) is to send ctrl-c
  2. the second value is to send the BREAK sequence
  3. the third value (new behavior) is to send BREAK followed by g
     (Magic SysReq g in the case of the Linux kernel).

This is not exactly upward compatible.  My real concern is front-ends,
but I don't think this is a setting that's so common that front-ends
know about. So I think that the idea of expanding this setting is OK.
Any objection?

Pedro, Daniel, or anyone with experience in remote.c,

I'd love your feedback on this patch... Especially the part
that sends an interrupt after having established the connection
with the remote agent if the interrupt is set to BREAK+g...
See below.
--------------
I am afraid I don't understand what you are saying. Are you able to tell me
what the remote agent mean? Does it mean gdbserver for example? 

Caz,

> This patch generalizes remotebreak. It becomes an enum string from a
> Boolean. It may be "Ctrl-C", "BREAK" or "BREAK-g". When it is "BREAK-g",
gdb
> also sends BREAK g to connect to Linux kernel when it starts.

Can you provide a ChangeLog entry for all changes you are submitting
here. See for instance this message that explains what happens, and
then provides a patch and its ChangeLog.  Another little detail is
that most maintainers tend to prefer unified diffs, so if you could
send these instead of context diff, that'd be much appreciated.

Last thing: As you're modifying a user-settable setting, the
documentation will also need to be updated. But since we're still
at the discussion stage, it's OK if you prefer to work on the
documentation at the end.

> + /* This variable chooses whether to send a ^C, a break or a break g
> +    when the user requests program interruption.
> +    Although ^C is usually what remote systems expect,
> +    and that is the default here, sometimes a break is
> +    preferable instead. For interrupting Linux kernel, a break and g is
> +    expected which is Magic SysReq g.  */

I would rephrase this a little bit in order to avoid repeating
the same thing twice. For instance:

/* Allow the user to specify what sequence to send to the remote
   when he requests a program interruption: Although ^C is usually
   what remote systems expect (this is the default, here), it is
   sometimes preferable to send a break.  On other systems such
   as the Linux kernel, a break followed by g, which is Magic SysReq g
   is required in order to interrupt the execution.  */
--------------
Great. The meaning is clear.

> + const char bs_Crtl_C[] = "Ctrl-C";
> + const char bs_BREAK[] = "BREAK";
> + const char bs_BREAK_g[] = "BREAK-g";
> + static const char *remotebreak_enum[] = {
> +   bs_Crtl_C,
> +   bs_BREAK,
> +   bs_BREAK_g,
> +   NULL

I personally don't really like the names used here. Perhaps Pedro,
who has more experience with the remote protocol code might have
suggestions that fit better with the rest of the code. Here is what
I suggest:
  - "interrupt" -> send ^C
  - "break" -> send the BREAK sequence
  - "break-g" -> send the BREAK sequence followed by g

I'd also like to rename your constants to avoid the capital letters.
It's more in line with the current GDB style. Overall, I suggest
something like this:

    const char remote_break_interrupt[] = "interrutpt"
    const char remote_break_break[] = "break"
    const char remote_break_sysreq_g[] "sysreq-g"
    static const char *remote_break_modes[] =
    {
      remote_break_interrupt,
      remote_break_break,
      remote_break_sysreq_g,
      NULL
    };
    static const char *remote_break_mode = remote_break_interrupt;
---------
BREAK is usually all capitalized in chip manual because it is high level of
serial line for certain time. But I don't care weather capitalized or not.

> ! static void show_remotebreak(struct ui_file *file, int from_tty,
> ! 			      struct cmd_list_element *c,
> ! 			      const char *value)
> ! {
> !   fprintf_unfiltered (file, "remote systems expect %s to be
> interrupted\n",
> ! 		      remotebreak_string);
> ! }

Let's use fprintf_filtered, otherwise there is no paging. Rather that
talking of what the remote system expects, I'd rather also say what
GDB does. I would suggest something like this:

  if (remote_break_mode == remote_break_interrupt)
    fprintf_filtered (_("To interrupt the execution of the program, "
                        "send the ASCII ETX character (Ctrl-c) "
                        "to the remote target."));
  else if (remote_break_mode = remote_break_break)
    [...]

Please note how messages printed by the debugger are enclosed inside _().
This is for i18n. Also because of i18n, you cannot build the message
piecemeal such as:

  fprintf_filtered (_("To interrupt the execution of the program, "));
  if (remote_break_mode == remote_break_interrupt)
    fprintf_filtered (_("send the ASCII ETX [...]));

Translators often need the entire sentence in order to translate it.

> +   /* send break sequence on debugging Linux kernel */
> +   if (remotebreak_string == bs_BREAK_g) {
> +     serial_send_break (remote_desc);
> +     serial_write (remote_desc, "g", 1);
> +   }

A couple of nits first: For GDB, the curly brace should be on the next
line of code:

  if (remotebreak_string == bs_BREAK_g)
    {
      [...]

And sentences in comments should start with a capital letter, and
end with a period. I am sorry if this is irritating you, but GDB follows
the GNU Coding Style. So, you comment above should be:

    /* Send the Magic SysReg g sequence when debugging the Linux kernel.  */

(notice the two spaces at the end of the sentence as well.

However, this being said, I really don't know about this change.
It seems to me that this part should not be controlled by the setting
that you're modifying, but by another setting. At the very least.
In fact, why do you need this at all? Can't your remote agent achieve
the same effect as you've established the connection???
----------------
I don't want to go to another room to type Magic SysRq every time when I
start debugging my device driver. Linux kernel has Magic SysRq to be
interrupted by gdb. I don't expect Linux kernel changes Magic SysRq to ^C or
BREAK. Therefore, the only way to interrupt Linux kernel is that gdb sends
Magic SysRq. Is this the one you want to hear? I am confused.

I'd like to have others' opinion on this one.


-- 
Joel

Attachment: remotebreak.patch
Description: Binary data