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

[Antoine Tremblay <antoine dot tremblay at ericsson dot com>]

Antoine Tremblay writes:

> Antoine Tremblay writes:
>
>> 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.
>>
>
> Sorry I got confused with the code patched with the latest 2 patches I
> sent refactoring the single stepping code.
>
> Considering the current code this is handled by the step-over process,
> and should not be an issue as it will always step-over before installing
> any single-step breakpoints.
>
> And step-over removes all breakpoints when stepping over thus
> get_next_pc is ok.
>
> This becomes an issue like I said before with
> https://sourceware.org/ml/gdb-patches/2016-11/msg00939.html
>
> Since with this it's possible to install single-step breakpoints without
> a step-over check.
>
> We could consider this patch a preparation for
> https://sourceware.org/ml/gdb-patches/2016-11/msg00939.html
>
> or just a good pratice to use target_read_memory.
>
> Thanks,
> Antoine

Just to supplement about:
https://sourceware.org/ml/gdb-patches/2016-11/msg00939.html

If we consider this patch the is 2 reasons we can't install step over
breakpoints.

One is to be able to delay a step-over.

The other is since GDBServer inserts single-step breakpoints when it
processes the resume requests and threads are about to be resumed.  If
threads still have pending status, single-step breakpoints are not
installed, so it needs to install them in proceed_all_lwp. And in this
case the single-step breakpoints are inserted outside of a step-over
process.