Re: [Openocd-development] software breakpoint with multicore systems

[Michel J. <mic...@st...>]

Hello, 
I have tried to follow your advice.
And here are 2 patches , one changing breakpoint.c file (indentation, coding style)
And the other solving the expected issue.

Best regards

-----Original Message-----
From: Peter Stuge [mailto:pe...@st...] 
Sent: Sunday, October 02, 2011 1:05 AM
To: Michel JAOUEN
Cc: James Zhao; ope...@li...
Subject: Re: [Openocd-development] software breakpoint with multicore systems

Hi,

Michel JAOUEN wrote:
> Here is a patch correcting this issue.

Content-Description: 0001-breakpoint-smp-software-breakpoint-correction.patch
> From 5fc6a3e930bdfde679b50f00cee98ba1273b8ee9 Mon Sep 17 00:00:00 2001
> From: Michel Jaouen <mic...@st...>
> Date: Fri, 30 Sep 2011 18:42:52 +0200
> Subject: [PATCH] breakpoint : smp software breakpoint correction
> 
> ---
>  src/target/breakpoints.c |   14 +++++++++++---
>  src/target/target.c      |    7 +++++++
>  2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/src/target/breakpoints.c b/src/target/breakpoints.c
> index 5a0fc40..d7acf00 100644
> --- a/src/target/breakpoints.c
> +++ b/src/target/breakpoints.c
> @@ -228,6 +228,8 @@ int retval = ERROR_OK;
>  		struct target_list *head;
>  		struct target *curr;
>  		head = target->head;
> +		if (type == BKPT_SOFT)  breakpoint_add_internal(head->target, address,length, type);
> +		else
>  		while(head != (struct target_list*)NULL)
>  		{

Sorry, but this is unacceptable. You are adding a new level of
conditionals without changing indentation. Come on.

I understand and very much appreciate the desire to touch as few
lines of codes as possible, but that objective can not take
precedence over correct indentation, which is absolutely fundamental
for quality source code, which I guess we all want to work with.

A suggestion is to correctly use goto, in order to avoid changing a
very large block of code, if this makes sense. You might not need to
use goto, but might be able to use return instead. I haven't looked
into this function so I can't say for sure, but the above is in any
case not OK.


> @@ -330,15 +332,19 @@ void breakpoint_remove_internal(struct target *target, uint32_t address)
>  	if (breakpoint)
>  	{
>  		breakpoint_free(target, breakpoint);
> +		return 1;
>  	}
>  	else
>  	{
> +		if (!target->smp)
>  		LOG_ERROR("no breakpoint at address 0x%8.8" PRIx32 " found", address);
> +        return 0;
>  	}

Same here about indentation. The added return line is also using
the wrong indentation character, please check your editor settings
and adjust it to the code style already used by the file. (I really
like the feature of some editors to autodetect the indent style used
in a file, but of course not all editors support this.)


>  void breakpoint_remove(struct target *target, uint32_t address)
>  {
> -    if ((target->smp))
> +	int found = 0;
> +	if ((target->smp))
>  	{

Please avoid mixing whitespace changes with other changes. Send one
patch which fixes all whitespace issues at once.


> --- a/src/target/target.c
> +++ b/src/target/target.c
> @@ -3023,6 +3023,13 @@ COMMAND_HANDLER(handle_bp_command)
>  	{
>  		case 0:
>  			return handle_bp_command_list(CMD_CTX);
> +		case 2:
> +			{
> +				asid = 0;
> +				COMMAND_PARSE_NUMBER(u32, CMD_ARGV[0], addr);
> +				COMMAND_PARSE_NUMBER(u32, CMD_ARGV[1], length);
> +				return handle_bp_command_set(CMD_CTX, addr, asid, length, hw);
> +			}

Why are these {} added, and why have the variable when it isn't used?


//Peter