Re: [PATCH 1/3] Fix inferior memory reading in GDBServer for arm/aarch32.

[Antoine Tremblay <antoine.tremblay@ericsson.com>]

Yao Qi writes:

> On Mon, Nov 28, 2016 at 07:27:56AM -0500, Antoine Tremblay wrote:
>> Before this patch, some functions would read the inferior memory with
>> (*the_target)->read_memory, which returns the raw memory, rather than the
>> shadowed memory.
>> 
>> This is wrong since these functions do not expect to read a breakpoint
>> instruction and can lead to invalid behavior.
>> 
>> Use of raw memory in get_next_pcs_read_memory_unsigned_integer for example
>> could lead to get_next_pc returning an invalid pc.
>
> Can you elaborate under what circumstance breakpoints are still in memory
> when these functions are called?  Can we have a test case?
>  

Here is an example:

In non-stop mode multiple threads are stepping, like in the
non-stop-fair-events.exp test.

GDB:
 thread 1
 step&

GDBServer:
 thread 1 is at instruction A
 installs single step breakpoint on instruction B

GDB:
 thread 2
 step&

GDBServer:

 thread 2 is at instruction B

 GDBServer needs to install a single step breakpoint at the next
 instruction from B.

 To do so get_next_pc is called, but since the single step
 breakpoint for thread 1 at instruction B is there. get_next_pc
 reads the current instruction as a breakpoint instruction and fails.

Note that I used a user driven example here to make it more clear but
this is also true while range-stepping in a loop for example:

 - thread 1 hits its single-step breakpoint deletes it
 - it's not out of a range-step so
 - tries to install a single-step breakpoint at the next
instruction
 - but thread 2 has a breakpoint at thread 1's current
instruction and get_next_pc fails.

This is already tested by non-stop-fair-events.exp, the test will fail
without this patch.

Note that this test is testing both range-stepping and the user
stepping.

>> @@ -769,15 +770,15 @@ arm_sigreturn_next_pc (struct regcache *regcache, int svc_number,
>>    gdb_assert (svc_number == __NR_sigreturn || svc_number == __NR_rt_sigreturn);
>>  
>>    collect_register_by_name (regcache, "sp", &sp);
>> -  (*the_target->read_memory) (sp, (unsigned char *) &sp_data, 4);
>> +  target_read_memory (sp, (unsigned char *) &sp_data, 4);
>>  
>>    pc_offset = arm_linux_sigreturn_next_pc_offset
>>      (sp, sp_data, svc_number, __NR_sigreturn == svc_number ? 1 : 0);
>>  
>> -  (*the_target->read_memory) (sp + pc_offset, (unsigned char *) &next_pc, 4);
>> +  target_read_memory (sp + pc_offset, (unsigned char *) &next_pc, 4);
>>  
>>    /* Set IS_THUMB according the CPSR saved on the stack.  */
>> -  (*the_target->read_memory) (sp + pc_offset + 4, (unsigned char *) &cpsr, 4);
>> +  target_read_memory (sp + pc_offset + 4, (unsigned char *) &cpsr, 4);
>>    *is_thumb = ((cpsr & CPSR_T) != 0);
>
> We are reading from stack, so we don't need to check weather there is
> a breakpoint or not.

Ho right, is it worth it to make the distinction however ?

I mean, would it be better general practice to use target_read_memory
unless we absolutely need to use the_target->read_memory like with
breakpoint_at funcs.. ? The counterpart looks more error prone for the
developer...