Re: [Openocd-development] software breakpoint with multicore systems
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...@stuge.se]
Sent: Sunday, October 02, 2011 1:05 AM
To: Michel JAOUEN
Cc: James Zhao; openocd-development@lists.berlios.de
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
> 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
0001-breakpoint-indentation.patch
Description: 0001-breakpoint-indentation.patch
0002-breakpoint-smp-software-breakpoint-issue.patch
Description: 0002-breakpoint-smp-software-breakpoint-issue.patch
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
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
> 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
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] software breakpoint with multicore systems
Can someone review these patches and comment? Thanks! -- Øyvind Harboe - Can Zylin Consulting help on your project? US toll free 1-866-980-3434 http://www.zylin.com/ ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] software breakpoint with multicore systems
Hello, Here is a patch correcting this issue. Best regards. 0001-breakpoint-smp-software-breakpoint-correction.patch Description: 0001-breakpoint-smp-software-breakpoint-correction.patch ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] software breakpoint with multicore systems
Hello, When I implemented the smp, I focused on hardware breakpoints. And later on , I noticed that the behavior was incorrect for software breakpoints (as you mentioned) For software breakpoints, I was planning to : * Create in target an addionnal field swbrk_target , * This field will be initialized at smp init, by selecting a dedicated target In the smp list (i.e according to core id number) , * And the the software breakpoint will be added/removed by using the swbrk_target , when smp is set to one. Best Regards Michel JAOUEN From: openocd-development-boun...@lists.berlios.de [mailto:openocd-development-boun...@lists.berlios.de] On Behalf Of James Zhao Sent: Monday, September 26, 2011 11:54 PM To: openocd-development@lists.berlios.de Subject: [Openocd-development] software breakpoint with multicore systems I am not sure if this is a bug or I have misunderstood something the code, but there seems to be a problem with software breakpoints. When adding a software breakpoint, the function breakpoint_add() would add a breakpoint to all targets, if smp is on. But since software breakpoints are done but replacing the instruction at the memory address with something to break when run, then why do we need to do this for all the cores, isn't once enough? James This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
[Openocd-development] software breakpoint with multicore systems
I am not sure if this is a bug or I have misunderstood something the code, but there seems to be a problem with software breakpoints. When adding a software breakpoint, the function breakpoint_add() would add a breakpoint to all targets, if smp is on. But since software breakpoints are done but replacing the instruction at the memory address with something to break when run, then why do we need to do this for all the cores, isn't once enough? James --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. --- ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
