[U-Boot] [PATCH] KGDB set / remove breakpoints

[Mike Frysinger]

On Saturday 17 April 2010 13:20:11 Tonny Tzeng wrote:
> This patch extends the current KGDB logic to handle 'Z' and 'z'
> GDB packets for setting or removing breakpoints.
> 
> Two weak functions have been added to the kgdb_stub.c:
> arch_kgdb_set_sw_break() and arch_kgdb_remove_sw_break() could be
> overrode by the arch implementations.
> 
> Please note, after applying this patch, those architectures, which
> already enabled KGDB support, have to create a new asm/kgdb.h and
> define the length of the break instruction (BREAK_INSTR_SIZE) in that
> file.

i dont think breaking build is a good idea.  i would have the code simply 
disable itself if BREAK_INSTR_SIZE isnt set.

> +/*
> + * Holds information about breakpoints in a kernel. These breakpoints are
> + * added and removed by gdb.
> + */
> +static struct kgdb_bkpt	kgdb_break[KGDB_MAX_BREAKPOINTS];

use a space between the type and the name, not a tab

> +static int kgdb_set_sw_break(int addr)
> +{
> +	int i, breakno = -1;
> +	struct kgdb_bkpt *bkpt;
> +
> +	for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
> +		if ((kgdb_break[i].state == BP_SET) &&
> +		    (kgdb_break[i].bpt_addr == addr))
> +			return -KGDBERR_BPEXIST;
> +		if ((kgdb_break[i].state == BP_REMOVED) &&
> +		    (kgdb_break[i].bpt_addr == addr)) {
> +			breakno = i;
> +			break;
> +		}
> +	}

there are a few places that loop over the structure to find a bpt by its addr.  
would probably be better to write a helper function:
static int kgdb_find_break_by_addr(int addr)
{
	int i;

	for (i = 0; i < KGDB_MAX_BREAKPOINTS; ++i)
		if (kgdb_break[i].bpt_addr == addr)
			return i;

	return -1;
}

then this function becomes a lot simpler:
	breakno = kgdb_find_break_by_addr(addr);
	if (breakno != -1) {
		if (kgdb_break[breakno].state == BP_SET)
			return .....
		....
	} else {
		....

> +#ifndef KGDB_MAX_BREAKPOINTS
> +#define KGDB_MAX_BREAKPOINTS	1000
> +#endif
>
> +struct kgdb_bkpt {
> +	unsigned long		bpt_addr;
> +	unsigned char		saved_instr[BREAK_INSTR_SIZE];
> +	enum kgdb_bptype	type;
> +	enum kgdb_bpstate	state;
> +};

the kgdb_bkpt is going to be about 16 bytes, and you're setting a default of 
1000 ?  that seems like an excessively large number that will simply waste 
memory (16k of it).  please use something much smaller like say 10 or 20.

also, for struct packing, it'd be better if the saved_instr was at the end of 
the struct so it doesnt force an alignment hole in the middle.

the define also needs a CONFIG_ type prefix on it.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100417/fd869d8e/attachment.pgp