symbolic debug of loadable modules with kgdb light

Caz Yokoyama cazyokoyama@gmail.com
Wed Sep 23 04:16:00 GMT 2009


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



More information about the Gdb-patches mailing list