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:

> 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