Re: symbolic debug of loadable modules with kgdb light

Joel Brobecker
2009-09-30T17:28:18Z

Caz,

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

You're missing an entry for the NEWS file (but good thinking on updating
this file!). This part will be reviewed by a different maintainer (Eli),
so I won't look at it personally. Same for the documentation changes.

For the changes to remote.c, unfortunately, you'll have to describe
the changes on a per-entity basis. For instance:

* remote.c (interrupt_sequence_control_c)
(interrupt_sequence_break, interrupt_sequence_sysrq_g)
(interrupt_sequence_modes): New constants.
(interrupt_sequence_mode): New global variable.
(show_interrupt_sequence): New function.
[...]
(remote_open): Call send_interrupt_sequence if interrupt_on_start.

One last question: How did you test this change? Have you run
the testsuite?

> +const char *interrupt_sequence_mode = interrupt_sequence_control_c;

This variable should be made static.

> +static void show_interrupt_sequence (struct ui_file *file, int from_tty,
> + struct cmd_list_element *c,
> + const char *value)

Just a formatting nit: the function name should be starting at column 0.
Therefore:

static void
show_interrupt_sequence (struct ui_file *file, int from_tty,
struct cmd_list_element *c,
const char *value)

It looks like you have several functions that need to be reformatted
that way. Can you take care of that?

> + internal_error (__FILE__, __LINE__,
> + _("You are sending unexpected %s to the remote target."),
> + interrupt_sequence_mode);

I would rather say:

"Invalid value for interrupt_sequence_mode: %s.",
interrupt_sequence_mode

Your version implies that the user is doing something wrong ("you are
sending"), whereas we should land there only if there was a software
error in GDB.

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

Just a couple of formatting nits: Plese use 2 spaces after each period.
And can you also join the "*/" at the end of the last line:

connecting gdb. */

> /* 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;

In this case, the comment needs to be updated, because this variable
should no longer have any effect on the code. It's just used to implement
a command that is now deprecated. Here is what I would say:

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

That's better, but still not quite right. Let's look at what we're trying
to achieve, at the user level, before we look at the code. What we're
trying to achieve, here, is to allow the user to use "set remotebreak
[on|off]", even if this command is deprecated. So, when the user types
"set remotebreak on", then we need to act as if he entered "set remote
interrupt-sequence break", and when he entered "set remotebreak off",
we need to act as if he entered "set remote interrupt-sequence control-c".

Now, on the technical side, here is how we do this: After the "set
remotebreak" command is entered, the remote_break value gets updated
according to the choice the user made (0 for "off", nonzero for "on"),
and then set_remotebreak gets called back. This is when we look at
the new choice the user made, and update interrupt_sequence_mode
accordingly. Try the following instead:

if (remote_break)
interrupt_sequence_mode = interrupt_sequence_break;
else
interrupt_sequence_mode = interrupt_sequence_control_c;

Please remove the printf of the command documentation.

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

Actually, please print nothing.

> +/*
> + Send interrupt_sequence to remote target.
> +*/

Formatting: Can you join all three lines?

/* Send interrupt_sequence to remote target. */

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

Can you add another internal_error here if interrupt_sequence_mode
has an unexpected value?

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

I would suggest rewording your comment a bit, as it implies that
the interrupt sequence is always sent at startup, which is not true.
It also says "when GDB starts" whereas it should say when the connection
is established. However, this comment is redundant with the comment
you wrote besides the interrupt_on_start declaration, so let's just
drop that one.

> - 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);

That's not quite how things should be done. Everytime a command is
added, we create a struct cmd_list_element for it. In order to
deprecate a command, we then call deprecate_cmd, passing the associated
struct cmd_list_element, and the name of the command that replaces it.
I don't see any other way to get the cmd_list_element for both the set
and show command other than by using lookup_cmd, unfortunately. Most
add_cmd_* functions return a pointer to the new command, but not the
setshow, probably because they create 2 commands rather than one.

So, something like the following just after the call do
add_setshow_boolean_cmd should do the trick:

cmd = lookup_cmd ("set remotebreak", setlist, -1, 1);
deprecate_cmd (cmd, "set remote interrupt-sequence");
cmd = lookup_cmd ("show remotebreak", setlist, -1, 1);
deprecate_cmd (cmd, "show remote interrupt-sequence");

This should have a visual effect similar to below when you try the
"set/show remotebreak" commands:

(gdb) disable tracepoints
Warning: command 'disable tracepoints' is deprecated.
Use 'disable'.

You don't need to change the documentation for the set/show remotebreak
command.

> + 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."),
> + 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);
> +

Eli usually reviews the documentation part of these commands. My comment
is that you'll need to elaborate a little more on the various choices
that are available. Here is an example that passed Eli's review:

> add_setshow_enum_cmd ("multiple-symbols", no_class,
> multiple_symbols_modes, &multiple_symbols_mode,
> _("\
>Set the debugger behavior when more than one symbol are possible matches\n\
>in an expression."), _("\
>Show how the debugger handles ambiguities in expressions."), _("\
>Valid values are \"ask\", \"all\", \"cancel\", and the default is \"all\"."),
> NULL, NULL, &setlist, &showlist);

If we stay on track, and you take care of the comments I made, I believe
the next version of this patch might be the one that gets checked in!

--
Joel