RE: symbolic debug of loadable modules with kgdb light

[Caz Yokoyama <cazyokoyama@gmail.com>]

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

Hello Joel,
Please start the paperwork for filing an assignment with the FSF. I don't
have a employer. This is a volunteer work while on job hunting.
Please find the updated patch in attachment.
- interrupt_sequence_sysrq_g -> interrupt_sequence_break_g
- Linux kernel -> The Linux kernel
- Tested. No regression.
-caz

-----Original Message-----
From: Joel Brobecker [mailto:brobecker@adacore.com] 
Sent: Thursday, October 01, 2009 4:33 PM
To: Caz Yokoyama
Cc: 'Eli Zaretskii'; gdb-patches@sourceware.org; 'Daniel Jacobowitz'; 'Pedro
Alves'
Subject: Re: symbolic debug of loadable modules with kgdb light

> +2009-10-01  Kazuyoshi Caz Yokoyama  <caz@caztech.com>
> +
> +	* remote.c (interrupt_sequence_control_c)
> +	(interrupt_sequence_break, interrupt_sequence_sysrq_g)
> +	(interrupt_sequence_modes): New constants.
> +	(interrupt_sequence_mode, interrupt_on_connect): New variable.
> +	(show_interrupt_sequence): New function.
> +	(set_remotebreak, show_remotebreak): New function.
> +	(send_interrupt_sequence): New function.
> +	(remote_start_remote): Call send_interrupt_sequence if
> +	interrupt_on_connect is true.
> +	(remote_stop_as): Call send_interrupt_sequence.
> +	(_initialize_remote): Add interrupt-sequence and
interrupt-on-connect,
> +	modify remotebreak to call set_remotebreak and show_remotebreak.

Very nice and clean! Just a few nits as shown below, but they are minor
and can be mechanically fixed.  So the code part of the patch is
pre-approved, after you address the last comments below (it means that
you have permission to commit the patch, but please make sure to send
a copy of the patch that you end up checking in).  I think Eli already
approved the latest version of the documentation part, but he'll
probably want to have a last quick look.

And of course, now that I talk about checking in your patch, I am
now only realizing that you do not seem to have an assignment filed
with the FSF. This is a requirement for us to accept your patch.
Perhaps you have an assignment on file through your employer?
I couldn't find you in their records using either your name or
email domain.  Let me know if you'd like me to get you started with
the paperwork. It takes a few weeks, so start ASAP if needed.

> +const char interrupt_sequence_sysrq_g[] = "BREAK-g";

You forgot to rename this constant to interrupt_sequence_break_g,
to match the actual litteral value.

> +/* This boolean variable specifies whether interrupt_sequence is sent
> +   to remote target when gdb connect to it.
> +   This is mostly needed when you debug Linux kernel.
> +   Linux kernel expects BREAK g which is Magic SysRq g for connecting
gdb.  */

I've noticed a few English mistakes. I started pointed them out, but in
the end, it's probably simpler if I give you the correct version:

/* This boolean variable specifies whether interrupt_sequence is sent
   to the remote target when gdb connects to it.
   This is mostly needed when you debug theLinux kernel: The Linux kernel
   expects BREAK g which is Magic SysRq g for connecting gdb.  */

> +  struct cmd_list_element *cmd;
> +  static char *cmd_name;

This should not be static. That way, it gets allocated on the stack
and then released after the function returns.

-- 
Joel

[-- Attachment #2: remotebreak.patch --]
[-- Type: application/octet-stream, Size: 12928 bytes --]

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.10924
diff -u -r1.10924 ChangeLog
--- gdb/ChangeLog	1 Oct 2009 20:09:20 -0000	1.10924
+++ gdb/ChangeLog	2 Oct 2009 00:27:20 -0000
@@ -1,3 +1,18 @@
+2009-10-01  Kazuyoshi Caz Yokoyama  <caz@caztech.com>
+
+	* remote.c (interrupt_sequence_control_c)
+	(interrupt_sequence_break, interrupt_sequence_break_g)
+	(interrupt_sequence_modes): New constants.
+	(interrupt_sequence_mode, interrupt_on_connect): New variable.
+	(show_interrupt_sequence): New function.
+	(set_remotebreak, show_remotebreak): New function.
+	(send_interrupt_sequence): New function.
+	(remote_start_remote): Call send_interrupt_sequence if
+	interrupt_on_connect is true.
+	(remote_stop_as): Call send_interrupt_sequence.
+	(_initialize_remote): Add interrupt-sequence and interrupt-on-connect,
+	modify remotebreak to call set_remotebreak and show_remotebreak.
+
 2009-10-01  Phil Muldoon  <pmuldoon@redhat.com>
 
 	* infcall.c (call_function_by_hand): Add a new cleanup branch for
Index: gdb/NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.331
diff -u -r1.331 NEWS
--- gdb/NEWS	15 Sep 2009 03:30:04 -0000	1.331
+++ gdb/NEWS	2 Oct 2009 00:27:22 -0000
@@ -234,6 +234,24 @@
 
 * New commands (for set/show, see "New options" below)
 
+set remote interrupt-sequence [Ctrl-C | BREAK | BREAK-g]
+show remote interrupt-sequence
+  Allow the user to select one of ^C, a BREAK signal or BREAK-g
+  as the sequence to the remote target in order to interrupt the execution.
+  Ctrl-C is a default.  Some system prefers BREAK which is high level of
+  serial line for some certain time.  Linux kernel prefers BREAK-g, a.k.a
+  Magic SysRq g.  It is BREAK signal and character 'g'.
+
+set remote interrupt-on-connect [on | off]
+show remote interrupt-on-connect
+  When interrupt-on-connect is ON, gdb sends interrupt-sequence to
+  remote target when gdb connects to it.  This is needed when you debug
+  Linux kernel.
+
+set remotebreak [on | off]
+show remotebreak
+Deprecated.  Use "set/show remote interrupt-sequence" instead.
+
 catch syscall [NAME(S) | NUMBER(S)]
   Catch system calls.  Arguments, which should be names of system
   calls or their numbers, mean catch only those syscalls.  Without
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	2 Oct 2009 00:27:25 -0000
@@ -546,14 +546,76 @@
    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 interrupt_sequence_control_c[] = "Ctrl-C";
+const char interrupt_sequence_break[] = "BREAK";
+const char interrupt_sequence_break_g[] = "BREAK-g";
+static const char *interrupt_sequence_modes[] =
+  {
+    interrupt_sequence_control_c,
+    interrupt_sequence_break,
+    interrupt_sequence_break_g,
+    NULL
+  };
+static const char *interrupt_sequence_mode = interrupt_sequence_control_c;
 
-/* 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 void
+show_interrupt_sequence (struct ui_file *file, int from_tty,
+			 struct cmd_list_element *c,
+			 const char *value)
+{
+  if (interrupt_sequence_mode == interrupt_sequence_control_c)
+    fprintf_filtered (file,
+		      _("Send the ASCII ETX character (Ctrl-c) "
+			"to the remote target to interrupt the "
+			"execution of the program.\n"));
+  else if (interrupt_sequence_mode == interrupt_sequence_break)
+    fprintf_filtered (file,
+		      _("send a break signal to the remote target "
+			"to interrupt the execution of the program.\n"));
+  else if (interrupt_sequence_mode == interrupt_sequence_break_g)
+    fprintf_filtered (file,
+		      _("Send a break signal and 'g' a.k.a. Magic SysRq g to "
+			"the remote target to interrupt the execution "
+			"of Linux kernel.\n"));
+  else
+    internal_error (__FILE__, __LINE__,
+		    _("Invalid value for interrupt_sequence_mode: %s."),
+		    interrupt_sequence_mode);
+}
 
+/* This boolean variable specifies whether interrupt_sequence is sent
+   to the remote target when gdb connects to it.
+   This is mostly needed when you debug the Linux kernel: The Linux kernel
+   expects BREAK g which is Magic SysRq g for connecting gdb.  */
+static int interrupt_on_connect = 0;
+
+/* This variable is used to implement the "set/show remotebreak" commands.
+   Since these commands are now deprecated in favor of "set/show remote
+   interrupt-sequence", it no longer has any effect on the code.  */
 static int remote_break;
 
+static void
+set_remotebreak (char *args, int from_tty, struct cmd_list_element *c)
+{
+  if (remote_break)
+    interrupt_sequence_mode = interrupt_sequence_break;
+  else
+    interrupt_sequence_mode = interrupt_sequence_control_c;
+}
+
+static void
+show_remotebreak (struct ui_file *file, int from_tty,
+		  struct cmd_list_element *c,
+		  const char *value)
+{
+}
+
 /* 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
    starts.  */
@@ -2585,6 +2647,25 @@
   int extended_p;
 };
 
+/* Send interrupt_sequence to remote target.  */
+static void
+send_interrupt_sequence ()
+{
+  if (interrupt_sequence_mode == interrupt_sequence_control_c)
+    serial_write (remote_desc, "\x03", 1);
+  else if (interrupt_sequence_mode == interrupt_sequence_break)
+    serial_send_break (remote_desc);
+  else if (interrupt_sequence_mode == interrupt_sequence_break_g)
+    {
+      serial_send_break (remote_desc);
+      serial_write (remote_desc, "g", 1);
+    }
+  else
+    internal_error (__FILE__, __LINE__,
+		    _("Invalid value for interrupt_sequence_mode: %s."),
+		    interrupt_sequence_mode);
+}
+
 static void
 remote_start_remote (struct ui_out *uiout, void *opaque)
 {
@@ -2598,6 +2679,9 @@
   /* Ack any packet which the remote side has already sent.  */
   serial_write (remote_desc, "+", 1);
 
+  if (interrupt_on_connect)
+    send_interrupt_sequence ();
+
   /* 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 +4105,8 @@
   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 interrupt_sequence to remote target.  */
+  send_interrupt_sequence ();
 }
 
 /* This is the generic stop called via the target vector. When a target
@@ -8993,6 +9073,8 @@
 _initialize_remote (void)
 {
   struct remote_state *rs;
+  struct cmd_list_element *cmd;
+  char *cmd_name;
 
   /* architecture specific data */
   remote_gdbarch_data_handle =
@@ -9060,8 +9142,31 @@
 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.  */
+			   set_remotebreak, show_remotebreak,
 			   &setlist, &showlist);
+  cmd_name = "remotebreak";
+  cmd = lookup_cmd (&cmd_name, setlist, "", -1, 1);
+  deprecate_cmd (cmd, "set remote interrupt-sequence");
+  cmd_name = "remotebreak"; /* needed because lookup_cmd updates the pointer */
+  cmd = lookup_cmd (&cmd_name, showlist, "", -1, 1);
+  deprecate_cmd (cmd, "show remote interrupt-sequence");
+
+  add_setshow_enum_cmd ("interrupt-sequence", class_support,
+			interrupt_sequence_modes, &interrupt_sequence_mode, _("\
+Set interrupt sequence to remote target."), _("\
+Show interrupt sequence to remote target."), _("\
+Valid value is \"Ctrl-C\", \"BREAK\" or \"BREAK-g\". The default is \"Ctrl-C\"."),
+			NULL, show_interrupt_sequence,
+			&remote_set_cmdlist,
+			&remote_show_cmdlist);
+
+  add_setshow_boolean_cmd ("interrupt-on-connect", class_support,
+			   &interrupt_on_connect, _("\
+Set whether interrupt-sequence is sent to remote target when gdb connects to."), _("		\
+Show whether interrupt-sequence is sent to remote target when gdb connects to."), _("		\
+If set, interrupt sequence is sent to remote target."),
+			   NULL, NULL,
+			   &remote_set_cmdlist, &remote_show_cmdlist);
 
   /* Install commands for configuring memory read/write packets.  */
 
Index: gdb/doc/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/doc/ChangeLog,v
retrieving revision 1.958
diff -u -r1.958 ChangeLog
--- gdb/doc/ChangeLog	26 Sep 2009 16:47:13 -0000	1.958
+++ gdb/doc/ChangeLog	2 Oct 2009 00:27:28 -0000
@@ -1,3 +1,8 @@
+2009-10-01  Kazuyoshi Caz Yokoyama  <caz@caztech.com>
+
+	* gdb.texinfo (Remote Configuration): Add "set/show remote
+	interrupt-sequence" and "set/show remote interrupt-on-connect" command.
+
 2009-09-26  Pierre Muller  <muller@ics.u-strasbg.fr>
 
 	* gdb.texinfo (Cygwin Native): Mention support for Ctrl-BREAK.
Index: gdb/doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.629
diff -u -r1.629 gdb.texinfo
--- gdb/doc/gdb.texinfo	26 Sep 2009 16:47:13 -0000	1.629
+++ gdb/doc/gdb.texinfo	2 Oct 2009 00:27:40 -0000
@@ -15011,6 +15011,34 @@
 target system.  If it is not set, the target will use a default
 filename (e.g.@: the last program run).
 
+@item set remote interrupt-sequence
+@cindex interrupt remote programs
+@cindex select Ctrl-C, BREAK or BREAK-g
+Allow the user to select one of @samp{Ctrl-C}, a @code{BREAK} or
+@samp{BREAK-g} as the
+sequence to the remote target in order to interrupt the execution.
+@samp{Ctrl-C} is a default.  Some system prefers @code{BREAK} which
+is high level of serial line for some certain time.
+Linux kernel prefers @samp{BREAK-g}, a.k.a Magic SysRq g.
+It is @code{BREAK} signal followed by character @code{g}.
+
+@item show interrupt-sequence
+Show which of @samp{Ctrl-C}, @code{BREAK} or @code{BREAK-g}
+is sent by @value{GDBN} to interrupt the remote program.
+@code{BREAK-g} is BREAK signal followed by @code{g} and
+also known as Magic SysRq g.
+
+@item set remote interrupt-on-connect
+@cindex send interrupt-sequence on start
+Specify whether interrupt-sequence is sent to remote target when
+@value{GDBN} connects to it.  This is mostly needed when you debug
+Linux kernel.  Linux kernel expects @code{BREAK} followed by @code{g}
+which is known as Magic SysRq g in order to connect @value{GDBN}.
+
+@item show interrupt-on-connect
+Show whether interrupt-sequence is sent
+to remote target when @value{GDBN} connects to it.
+
 @kindex set tcp
 @kindex show tcp
 @item set tcp auto-retry on
@@ -29817,9 +29845,9 @@
 @cindex interrupts (remote protocol)
 
 When a program on the remote target is running, @value{GDBN} may
-attempt to interrupt it by sending a @samp{Ctrl-C} or a @code{BREAK},
-control of which is specified via @value{GDBN}'s @samp{remotebreak}
-setting (@pxref{set remotebreak}).
+attempt to interrupt it by sending a @samp{Ctrl-C}, @code{BREAK} or
+a @code{BREAK} followed by @code{g},
+control of which is specified via @value{GDBN}'s @samp{interrupt-sequence}.
 
 The precise meaning of @code{BREAK} is defined by the transport
 mechanism and may, in fact, be undefined.  @value{GDBN} does not
@@ -29836,6 +29864,10 @@
 (@pxref{X packet}), used for binary downloads, may include an unescaped
 @code{0x03} as part of its packet.
 
+@code{BREAK} followed by @code{g} is also known as Magic SysRq g.
+When Linux kernel receives this sequence from serial port,
+it stops execution and connects to gdb.
+
 Stubs are not required to recognize these interrupt mechanisms and the
 precise meaning associated with receipt of the interrupt is
 implementation defined.  If the target supports debugging of multiple