Fix for setting/clearing breaks take 2

[John Newlin]

Jonathan Larmour wrote:

> 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?

Unfortunately that does not work on the Xtensa architecture.  Xtensa has 
instructions that are 2 bytes or 3 bytes.  The break instruction is 
2-bytes, however it can be on any alignment (due to the intermixed 
3-byte instructions throwing off the alignment).  Yes, the architecture 
is weird that way, instructions can be on any alignment, but the 
load/store instructions require aligned data.  It does make for some 
compact code though.  :)

>
> 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.

Those functions check alignment, and for something byte aligned will do 
byte read/writes.  Much like memcpy.   A friend suggested this might be 
better:

break_buffer.savedInstr = cyg_hal_gdb_insert_breakinst(pc);


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

It's 2 bytes, but can fall on any alignment.

>
> 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);
>
>
>