RE: symbolic debug of loadable modules with kgdb light

[Caz Yokoyama <cazyokoyama@gmail.com>]

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

Hello Joel,
Here is the patch for 1. Let me know if I forget something. Also any
comments are welcome. Thank you.
-caz

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

> Do you think we have to check correctness of user input for
> interrupt_sequence?

Yes, if only for the vast majority of users who will be using either
the default or using BREAK.

> Setting-up debugging environment of Linux device driver by gdb is not
> a trivial task

I don't doubt that, and I do not doubt that users prefer flexibility.
The current solution, which offers the user 3 choices for the interrupt
sequence, has no kernel dependency. It's very clear what each choices
does, and I do not understand how allowing free text instead of a defined
set of choices helps make things easier, especially when only specific
choices will actually be accepted in the end. You are arguing that we may
need more choices in the future. I answered that we can worry about that
later, *when/if* the situation actually arises.  Adding more choices is
a matter of seconds with a 10-line patch.

But, again, I also repeat that, if you think this is unnacceptable,
then perhaps we can accomodate extensibility while not needing code
recompilation by using a technical solution based on XML. You can have
a look at how "catch syscall" is implemented for an illustration.
But this can be done as a second phase, after the patch on which
you've been working on is approved and checked in.

In other words:

  1. Implement a patch that adds:
        set/show remote interrupt-sequence <control-c|BREAK|BREAK-g>
        set/show remote interrupt-at-startup [on|off]
     Make the old set/show remotedebug deprecated (please take a look
     at my earlier reply on what I mean by that)

  2. Look into adding extensibility through the use of an XML file

If you prefer to do all the work in one patch, you are welcome to do so,
but you are letting the best be the enemy of good, IMO, and only delaying
the time when vanilla FSF GDB can debug Linux Kernel modules.

-- 
Joel

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

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.10923
diff -u -r1.10923 ChangeLog
--- gdb/ChangeLog	29 Sep 2009 16:27:05 -0000	1.10923
+++ gdb/ChangeLog	30 Sep 2009 04:41:50 -0000
@@ -1,3 +1,13 @@
+2009-09-29  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. The syntax is
+	set remote interrupt-sequence [control-c | break | sysrq-g]
+	control-c is a default. Some system prefers break which is high level of
+	serial line for some certain time. Linux kernel prefers sysrq-g, a.k.a
+	Magic SysRq. It is break signal and character 'g'.
+
 2009-09-29  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* ia64-tdep.c (ia64_convert_from_func_ptr_addr): New variable buf.
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	30 Sep 2009 04:41:52 -0000
@@ -3,6 +3,15 @@
 
 *** Changes since GDB 6.8
 
+* "set/show remotebreak" command is deprecated. "set/show remote interrupt-sequence"
+is added. They add break signal followed by a character 'g' in addition to control-c
+and a break signal. break signal and g is also known as Magic SysRq and it interrupts
+Linux kernel.
+
+* "set/show remote interrupt-on-start" command is added. When this is ON,
+gdb sends interrupt-sequence to the remote target when gdb starts. This is needed when
+you debug Linux kernel.
+
 * GDB now has an interface for JIT compilation.  Applications that
 dynamically generate code can create symbol files in memory and register
 them with GDB.  For users, the feature should work transparently, and
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	30 Sep 2009 04:41:54 -0000
@@ -546,14 +546,83 @@
    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[] = "control-c";
+const char interrupt_sequence_break[] = "break";
+const char interrupt_sequence_sysrq_g[] = "sysrq-g";
+static const char *interrupt_sequence_modes[] =
+  {
+    interrupt_sequence_control_c,
+    interrupt_sequence_break,
+    interrupt_sequence_sysrq_g,
+    NULL
+  };
+const char *interrupt_sequence_mode = interrupt_sequence_control_c;
+
+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_sysrq_g)
+    fprintf_filtered (file,
+		      _("Send a break signal and 'g' a.k.a. sysrq-g and Magic SysRq to "
+			"the remote target to interrupt the execution "
+			"of Linux kernel.\n"));
+  else
+    internal_error (__FILE__, __LINE__,
+		    _("You are sending unexpected %s to the remote target."),
+		    interrupt_sequence_mode);
+}
+
+/* This boolean variable specifies whether interrupt_sequence is sent
+   to remote target when gdb starts. This is mostly needed when you debug
+   Linux kernel. Linux kernel expects BREAK g which is Magic SysRq for
+   connecting gdb.
+  */
+static int interrupt_on_start = 0;
 
 /* 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 set_remotebreak (char *args, int from_tty, struct cmd_list_element *c)
+{
+  printf_filtered (_("%s\n"), c->doc);
+  if (interrupt_sequence_mode == interrupt_sequence_control_c)
+    {
+      interrupt_sequence_mode = interrupt_sequence_break;
+      remote_break = 0;
+    }
+  else
+    {
+      interrupt_sequence_mode = interrupt_sequence_control_c;
+      remote_break = 1;
+    }
+}
+
+static void show_remotebreak (struct ui_file *file, int from_tty,
+			      struct cmd_list_element *c,
+			      const char *value)
+{
+  fprintf_filtered (file, _("%s\n"), c->doc);
+}
+
 /* 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 +2654,23 @@
   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_sysrq_g)
+    {
+      serial_send_break (remote_desc);
+      serial_write (remote_desc, "g", 1);
+    }
+}
+
 static void
 remote_start_remote (struct ui_out *uiout, void *opaque)
 {
@@ -2598,6 +2684,12 @@
   /* Ack any packet which the remote side has already sent.  */
   serial_write (remote_desc, "+", 1);
 
+  /* Interrupt_sequence is sent to remote target when gdb starts.
+     This is mostly needed when you debug Linux kernel.
+     Linux kernel expects BREAK g which is Magic SysRq for connecting gdb.  */
+  if (interrupt_on_start)
+    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 +4113,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
@@ -9056,13 +9144,30 @@
 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."), _("\
+  add_setshow_boolean_cmd ("remotebreak", class_obscure, &remote_break, _("\
+Deprecated. Use \"set remote interrupt-sequence [control-c|break]\" instead."), _("\
+Deprecated. Use \"show remote interrupt-sequence\" instead."), _("\
 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);
 
+  add_setshow_enum_cmd ("interrupt-sequence", class_support,
+			interrupt_sequence_modes, &interrupt_sequence_mode, _("\
+Set interrupt sequence to remote target, control-c/break/sysrq-g."), _("\
+Show interrupt sequence to remote target."),
+			NULL,
+			NULL, show_interrupt_sequence,
+			&remote_set_cmdlist,
+			&remote_show_cmdlist);
+
+  add_setshow_boolean_cmd ("interrupt-on-start", class_support,
+			   &interrupt_on_start, _("\
+Set whether to send interrupt sequence when gdb starts."), _("		\
+Show whether to send interrupt sequence when gdb starts."), _("		\
+If set, interrupt sequence is sent to the remote target."),
+			   NULL, NULL,
+			   &remote_set_cmdlist, &remote_show_cmdlist);
+
   /* Install commands for configuring memory read/write packets.  */
 
   add_cmd ("remotewritesize", no_class, set_memory_write_packet_size, _("\
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	30 Sep 2009 04:41:58 -0000
@@ -1,3 +1,9 @@
+2009-09-29  Kazuyoshi Caz Yokoyama  <caz@caztech.com>
+
+	* gdb.texinfo: remove "set/show remotebreak" command.
+	Add "set/show remote interrupt-sequence" and
+	"set/show remote interrupt-on-start" 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	30 Sep 2009 04:42:11 -0000
@@ -14940,20 +14940,6 @@
 @item show remotebaud
 Show the current speed of the remote connection.
 
-@item set remotebreak
-@cindex interrupt remote programs
-@cindex BREAK signal instead of Ctrl-C
-@anchor{set remotebreak}
-If set to on, @value{GDBN} sends a @code{BREAK} signal to the remote
-when you type @kbd{Ctrl-c} to interrupt the program running
-on the remote.  If set to off, @value{GDBN} sends the @samp{Ctrl-C}
-character instead.  The default is off, since most remote systems
-expect to see @samp{Ctrl-C} as the interrupt signal.
-
-@item show remotebreak
-Show whether @value{GDBN} sends @code{BREAK} or @samp{Ctrl-C} to
-interrupt the remote program.
-
 @item set remoteflow on
 @itemx set remoteflow off
 @kindex set remoteflow
@@ -15011,6 +14997,34 @@
 target system.  If it is not set, the target will use a default
 filename (e.g.@: the last program run).
 
+@item set interrupt-sequence
+@cindex interrupt remote programs
+@cindex select control-c, break or sysrq-g
+@anchor{set interrupt-sequence}
+Allow the user to specify what sequence to send to the remote target
+when he requests a program interruption: Although @kbd{control-c} is usually
+what remote systems expect (this is the default, here), it is
+sometimes preferable to send a @code{break}.  On other systems such
+as the Linux kernel, a @code{break} followed by @code{g}, which is
+@code{Magic SysRq g}
+is required in order to interrupt the execution.
+
+@item show interrupt-sequence
+Show which of @samp{Ctrl-C}, @code{break} or @code{break} followed by @code{g}
+is sent by @value{GDBN} to interrupt the remote program.
+
+@item set interrupt-on-start
+@cindex send interrupt-sequence on start
+@anchor{set interrupt-on-start}
+Specify whether interrupt_sequence is sent
+to remote target when gdb starts. This is mostly needed when you debug
+@code{Linux kernel}. Linux kernel expects @code{BREAK g} which is @code{Magic SysRq}
+in order to connect gdb.
+
+@item show interrupt-on-start
+Show whether interrupt_sequence is sent
+to remote target when gdb starts.
+
 @kindex set tcp
 @kindex show tcp
 @item set tcp auto-retry on
@@ -29817,9 +29831,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}, @samp{BREAK} or a @samp{BREAK g},
+control of which is specified via @value{GDBN}'s @samp{interrupt-sequence}
+setting (@pxref{set remote 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 +29850,10 @@
 (@pxref{X packet}), used for binary downloads, may include an unescaped
 @code{0x03} as part of its packet.
 
+@code{BREAK g} is also known as Magic SysRq and is @code{BREAK} and character '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