RE: symbolic debug of loadable modules with kgdb light

[Caz Yokoyama <cazyokoyama@gmail.com>]

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

Hello Joel,
I revised my patch. As I wrote in ChangeLog, I expanded interrupt_sequence
to arbitrary characters include ^C and BREAK instead of 3 choices.
-caz

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

Caz,

A couple of opening remarks:

Is there any way you can quote emails using the standard '>' or '|'
instead of using using '------------------'.  This convention makes
it harder to separate my text from your replies.

The second remark is that the other Global Maintainers that I talked to
about this, felt that sending (or not) the interrupt sequence when
connecting to the target should be done independently of the actual
interruption sequence. We all really like Daniel's (drow) suggestion:

    set/show remote interrupt-sequence <interrupt|BREAK|BREAK-g>
    set/show remote interrupt-after-connection [on|off]

If "set remote interrupt-sequence interrupt" sounds weird, then
perhaps we should go back to your initial suggestion of using
"control-c".

The last remark is that some of us felt that it was unusual to introduce
some Linux-specific code in remote.c, and that this could be easily
handled in a netcat-like wrapper.  You'd then do:

    (gdb) target remote | kernel-wrapper /foo/device

What do you think? We're not opposed to your patch, but the wrapper
does have the advantage of allowing you to work with any debugger,
including older versions of it :).

> 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.

OK, let's use BREAK then.

> +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.

You'll have to be a little more detailed in the ChangeLog entry.
Have a look at the ChangeLog file for tons of example on how we
are expected to write them.

> +/* 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";
[etc]

Can you change the name of this option and add the second option
as per above?

Also, still in line with having these two new commands, this makes
the old "set remotedebug" command deprecated.  To help users transition
to the new set of commands, can you do the following: If the user tries
to change the behavior using "set remotebreak", then issue a warning
that this command is deprecated in favor of "set remote interrupt-...",
and then change the set remote interrupt-sequence setting accordingly.
If the user types "show remotebreak", then just emit the deprecated
warning, but then either print nothing, or print the output that
"show remote interrupt-sequence" would print.  To achieve this, you
will need: deprecate_cmd, and you'll need to use the set_fun/show_func
arguments of the add_..._cmd function.


> +    fprintf_filtered (file,
> +		      ("You are sending %s to the remote target "
> +		       "which is not expected."), remote_break_mode);

Let's replace that with an internal_error.

> +  /* Send the Magic SysRg g sequence in order to interrupt
                       ^^^^^ typo: SysRq
> +     the execution of Linux kernel.  */
> +  if (remote_break_mode == remote_break_sysrq_g)
> +    {
> +    serial_send_break (remote_desc);
> +    serial_write (remote_desc, "g", 1);

This block needs to be conditionalized on the second option.

> +  /* 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);
> +    }

If the if/while/etc block only contains one statement, then we prefer
that the curly braces NOT be used. Can you remove them, please?

> ===================================================================
> 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 22:39:33 -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

I don't think you meant to make these changes (and they are not
described in the ChangeLog). Just make sure you undo them in your
local sandbox, to make sure you don't accidently check them in.

> Index: gdb/doc/gdb.texinfo

The documentation patch is reviewed by Eli. Can you also add an entry
in the NEWS file describing the new commands, and explaining that
the old "set/show remotebreak" commands are now deprecated?

-- 
Joel

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

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.10906
diff -u -r1.10906 ChangeLog
--- gdb/ChangeLog	25 Sep 2009 21:39:52 -0000	1.10906
+++ gdb/ChangeLog	26 Sep 2009 03:38:17 -0000
@@ -1,3 +1,24 @@
+2009-09-25  Kazuyoshi Caz Yokoyama  <caz@caztech.com>
+
+	* remote.c: Add a new remote variable, interrupt_sequence and
+	interrupt-on-start. interrupt_sequence provides arbitrary number of
+	characters include Ctrl-C to the remote target to stop the execution
+	of the program on the target. Not only characters but it also send
+	BREAK signal to the target. For example,
+	^C: ASCII ETX character (Ctrl-c, 0x03)
+	BREAK: BREAK signal
+	BREAK-g: BREAK signal and 'g' which is Magic SysRq which interrupt
+	         the execution of Linux kernel 
+	abcdef: ASCII character, abcdef
+	abc-xyz: ASCII character, abcxyz
+	remotebreak is deprecated even though it is still available. It switches
+	^C and BREAK.
+	interrupt-on-start is a boolean variable and controls whether
+	interrupt_sequence is sent when gdb starts. When you debug Linux kernel,
+	you probably do
+	set remote interrupt-sequence BREAK-g
+	set remote interrupt-on-start on
+	
 2009-09-25  Tom Tromey  <tromey@redhat.com>
 
 	PR python/10664:
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	26 Sep 2009 03:38:20 -0000
@@ -546,13 +546,66 @@
    this can go away.  */
 static int wait_forever_enabled_p = 1;
 
+/* Interrupt sequence to the remote target to interrupt the program
+   or OS.  */
+
+#define CTRL_C "^C"  /* default of interrupt_sequence */
+#define BREAK "BREAK"
+static char *interrupt_sequence = NULL;
+
+/* 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 int remote_break = 0;
+
+static void
+set_remote_break (char *args, int from_tty, struct cmd_list_element *c)
+{
+  if (strcmp(interrupt_sequence, CTRL_C) == 0)
+    {
+      interrupt_sequence = xmalloc(sizeof(BREAK) + 1);
+      strcpy(interrupt_sequence, BREAK);
+      remote_break = 1;
+    }
+  else if (strcmp(interrupt_sequence, BREAK) == 0)
+    {
+      interrupt_sequence = xmalloc(sizeof(CTRL_C) + 1);
+      strcpy(interrupt_sequence, CTRL_C);
+      remote_break = 0;
+    }
+#if 0
+  fprintf_filtered (file, _("\
+Deprecated. Use interrupt_sequence instead."));
+#endif
+}
+
+static void
+show_remote_break (struct ui_file *file, int from_tty,
+		   struct cmd_list_element *c,
+		   const char *value)
+{
+  if (strcmp(interrupt_sequence, CTRL_C) == 0)
+    fprintf_filtered (file, _("\
+Whether to send break if interrupted is off.\n\
+Deprecated. Use set remote interrupt_sequence instead.\n"));
+  else if (strcmp(interrupt_sequence, "BREAK") == 0)
+    fprintf_filtered (file, _("\
+Whether to send break if interrupted is on.\n\
+Deprecated. Use set remote interrupt_sequence instead.\n"));
+  else
+    fprintf_filtered (file, _("\
+Sending %s as interrupt_sequence.\n\
+Deprecated. Use interrupt_sequence instead.\n"), interrupt_sequence);
+}
 
 /* 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
@@ -2585,6 +2638,38 @@
   int extended_p;
 };
 
+/*
+  Send interrupt_sequence to remote target.
+*/
+static void
+send_interrupt_sequence ()
+{
+  char *b, *p, ch;
+
+  b = xmalloc (strlen(interrupt_sequence) + 1);
+  if (b == NULL)
+    internal_error (__FILE__, __LINE__,
+		    _("can't allocate b in send_interrupt_sequence()"));
+  strcpy (b, interrupt_sequence);
+  for (p = strtok (b, "-");
+       p; p = strtok(NULL, "-"))
+    {
+      if (strcmp(p, CTRL_C) == 0)
+	serial_write (remote_desc, "\x03", 1);
+      else if (strcmp(p, "BREAK") == 0)
+	serial_send_break (remote_desc);
+      else if (*p == '\\')
+	{
+	  sscanf (p, "%c", &ch);
+	  serial_write (remote_desc, &ch, 1);
+	}
+      else
+	serial_write (remote_desc, p, strlen(p));
+    }
+
+  xfree (b);
+}
+
 static void
 remote_start_remote (struct ui_out *uiout, void *opaque)
 {
@@ -2598,6 +2683,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 +4112,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 +9143,33 @@
 terminating `#' character and checksum."),
 	   &maintenancelist);
 
-  add_setshow_boolean_cmd ("remotebreak", no_class, &remote_break, _("\
+  add_setshow_boolean_cmd ("remotebreak", class_obscure, &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.  */
+			   set_remote_break, show_remote_break,
 			   &setlist, &showlist);
 
+  /* When set command is called for the item which is defined by
+     add_setshow_string_cmd, a string is freed when the string is not NULL
+     even it is set by initialization.  */
+  interrupt_sequence = xmalloc(sizeof(CTRL_C) + 1);
+  strcpy(interrupt_sequence, CTRL_C);
+  add_setshow_string_noescape_cmd ("interrupt-sequence", class_support,
+				   &interrupt_sequence, _("\
+Set interrupt sequence to remote target."), _("\
+Show interrupt sequence to remote target."), _("\
+\\xNN, ^C and BREAK are interpreted as appropriately."),
+				   NULL, NULL,
+				   &remote_set_cmdlist, &remote_show_cmdlist);
+
+  add_setshow_boolean_cmd ("interrupt-on-start", no_class, &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, _("\