RE: symbolic debug of loadable modules with kgdb light

[Caz Yokoyama <cazyokoyama at gmail dot com>]

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: remotebreak.patch
Description: Binary data