RE: symbolic debug of loadable modules with kgdb light

Caz Yokoyama
2009-09-24T22:42:45Z

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

Another revise. Include document.
-caz

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

Hello Joel,
Here is the revised patch according to you.
-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 #2: remotebreak.patch --]
[-- Type: application/octet-stream, Size: 8384 bytes --]

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.10903
diff -u -r1.10903 ChangeLog
--- gdb/ChangeLog 24 Sep 2009 17:58:04 -0000 1.10903
+++ gdb/ChangeLog 24 Sep 2009 22:39:30 -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 Joel Brobecker <brobecker@adacore.com>

* record.c (record_open, record_store_registers, record_xfer_partial):
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 22:39:33 -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 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
Index: gdb/doc/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/doc/ChangeLog,v
retrieving revision 1.957
diff -u -r1.957 ChangeLog
--- gdb/doc/ChangeLog 24 Sep 2009 17:49:56 -0000 1.957
+++ gdb/doc/ChangeLog 24 Sep 2009 22:39:37 -0000
@@ -1,3 +1,8 @@
+2009-09-24 Kazuyoshi Caz Yokoyama <caz@caztech.com>
+
+ * gdb.texinfo: change the type of remotebreak. It may be one of
+ "interrutpt", "break" or "sysrq-g".
+
2009-09-23 Joel Brobecker <brobecker@adacore.com>

* observer.texi (solib_unloaded): Document explicitly the fact that
Index: gdb/doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.628
diff -u -r1.628 gdb.texinfo
--- gdb/doc/gdb.texinfo 21 Sep 2009 12:48:36 -0000 1.628
+++ gdb/doc/gdb.texinfo 24 Sep 2009 22:39:50 -0000
@@ -14942,17 +14942,19 @@

@item set remotebreak
@cindex interrupt remote programs
-@cindex BREAK signal instead of Ctrl-C
+@cindex select Ctrl-C, BREAK signal or BREAK g
@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.
+Allow the user to specify what sequence to send to the remote
+when he requests a program interruption: Although @kbd{Ctrl-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 remotebreak
-Show whether @value{GDBN} sends @code{BREAK} or @samp{Ctrl-C} to
-interrupt the remote program.
+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 remoteflow on
@itemx set remoteflow off
@@ -29807,7 +29809,8 @@
@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},
+attempt to interrupt it by sending one of @samp{Ctrl-C}, @code{BREAK} or
+@code{BREAK} followed by @code{g},
control of which is specified via @value{GDBN}'s @samp{remotebreak}
setting (@pxref{set remotebreak}).