Fix for setting/clearing breaks take 2

[Jonathan Larmour]

John Newlin wrote:
> My previous patch had a small bug in it.  It really works this time.  ;)

Can you give a bit more background on this? This patch doesn't seem right 
to me.... surely you should be defining things so that "*(t_inst*)pc = 
(t_inst)HAL_BREAKINST;" will work?

If there is a problem setting the breakpoint due to something not being 
aligned appropriately, then the __read/write_mem_safe will not necessarily 
  have completed.

Can you tell me what the size of HAL_BREAKINST is? And the alignment 
requirements?

Jifl

> ------------------------------------------------------------------------
> 
> Index: hal/common/current/ChangeLog
> ===================================================================
> RCS file: /cvs/ecos/ecos/packages/hal/common/current/ChangeLog,v
> retrieving revision 1.103
> diff -c -r1.103 ChangeLog
> *** hal/common/current/ChangeLog	22 Apr 2004 15:26:33 -0000	1.103
> --- hal/common/current/ChangeLog	21 May 2004 22:18:31 -0000
> ***************
> *** 1,3 ****
> --- 1,13 ----
> + 2004-05-19  John Newlin <jnewlin@stretchinc.com>
> + 
> + 	* src/hal_stub.c:
> + 	(cyg_hal_gdb_interrupt) 
> + 	(cyg_hal_gdb_remove_break): Changed both to use
> + 	_read_mem_safe/__write_mem_safe for inserting a breakpoint, and
> + 	restoring the original instruction.
> + 	The Xtensa architecture (and others maybe?) can have unaligned
> + 	instructions, which caused unaligned load/store exception.
> + 	
>   2004-04-22  Jani Monoses <jani@iv.ro>
>   
>   	 * cdl/hal.cdl :
> Index: hal/common/current/src/hal_stub.c
> ===================================================================
> RCS file: /cvs/ecos/ecos/packages/hal/common/current/src/hal_stub.c,v
> retrieving revision 1.38
> diff -c -r1.38 hal_stub.c
> *** hal/common/current/src/hal_stub.c	6 May 2003 21:00:20 -0000	1.38
> --- hal/common/current/src/hal_stub.c	21 May 2004 22:18:31 -0000
> ***************
> *** 281,286 ****
> --- 281,288 ----
>   void 
>   cyg_hal_gdb_interrupt (target_register_t pc)
>   {
> +     t_inst break_inst = HAL_BREAKINST;
> + 
>       CYGARC_HAL_SAVE_GP();
>   
>       // Clear flag that we Continued instead of Stepping
> ***************
> *** 290,298 ****
>           cyg_hal_gdb_remove_break( (target_register_t)break_buffer.targetAddr );
>   
>       if (NULL == break_buffer.targetAddr) {
> !         break_buffer.targetAddr = (t_inst*) pc;
> !         break_buffer.savedInstr = *(t_inst*)pc;
> !         *(t_inst*)pc = (t_inst)HAL_BREAKINST;
>   
>           __data_cache(CACHE_FLUSH);
>           __instruction_cache(CACHE_FLUSH);
> --- 292,306 ----
>           cyg_hal_gdb_remove_break( (target_register_t)break_buffer.targetAddr );
>   
>       if (NULL == break_buffer.targetAddr) {
> ! 	// Not always safe to read/write directly to program
> ! 	// memory due to possibly unaligned instruction, use the
> ! 	// provided memory functions instead.
> ! 	__read_mem_safe(&break_buffer.savedInstr, (t_inst*)pc, HAL_BREAKINST_SIZE);
> ! 	__write_mem_safe(&break_inst, (t_inst*)pc, HAL_BREAKINST_SIZE);
> ! 
> ! 	// Save the PC where we put the break, so we can remove
> ! 	// it after the target takes the break.
> ! 	break_buffer.targetAddr = (t_inst*)pc;
>   
>           __data_cache(CACHE_FLUSH);
>           __instruction_cache(CACHE_FLUSH);
> ***************
> *** 308,314 ****
>           return 0;
>   
>       if ((t_inst*)pc == break_buffer.targetAddr) {
> !         *(t_inst*)pc = break_buffer.savedInstr;
>           break_buffer.targetAddr = NULL;
>   
>           __data_cache(CACHE_FLUSH);
> --- 316,323 ----
>           return 0;
>   
>       if ((t_inst*)pc == break_buffer.targetAddr) {
> ! 
> ! 	__write_mem_safe(&break_buffer.savedInstr, (t_inst*)pc, HAL_BREAKINST_SIZE);
>           break_buffer.targetAddr = NULL;
>   
>           __data_cache(CACHE_FLUSH);


-- 
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
--["No sense being pessimistic, it wouldn't work anyway"]-- Opinions==mine