Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

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

Yao Qi writes:

> On 16-11-29 07:07:01, Antoine Tremblay wrote:
>> ** Note these patches depend on:
>> https://sourceware.org/ml/gdb-patches/2016-11/msg00914.html
>>
>> Before, installing single-step breakpoints was done in proceed_one_lwp as
>> each thread was started.
>>
>> This caused a problem on ARM, which is the only architecture using
>> software single-step on which it is not safe to modify an instruction
>> to insert a breakpoint in one thread while the other threads are running.
>> See the architecture manual section "A3.5.4 Concurrent modification and
>> execution of instructions":
>>
>> "The ARMv7 architecture limits the set of instructions that can be
>> executed by one thread of execution as they are being modified by another
>> thread of execution without requiring explicit synchronization.  Except
>> for the instructions identified in this section, the effect of the
>> concurrent modification and execution of an instruction is UNPREDICTABLE
>> ."
>
> This doesn't apply to ptrace.  PTRACE_POKETEXT on a word aligned address
> is atomic, so threads observe the coherent result, either breakpoint
> instruction or the original instruction.  We don't see any fails in -marm
> mode, do we?
>

Indeed.

> In -mthumb mode, breakpoint can be 16-bit or 32-bit, but GDBserver still
> PTRACE_POKETEXT on a word aligned address (in linux-low.c:linux_write_memory)
> and write a word each time.  It should be atomic, however, if the 32-bit
> thumb-2 instruction is 2-byte aligned, we end up two PTRACE_POKETEXT,
> which is non-atomic.  That is the root cause of the problem, AFAICS.
>

Haaa makes total sense now :) thx!

> 32-bit thumb-2 breakpoint was invented for single step 32-bit thumb-2
> instruction in IT block,
> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-January/007488.html
> but we can use 16-bit thumb breakpoint instruction anywhere else.  If I
> force GDBserver to use 16-bit thumb breakpoint instruction even on 32-bit
> thumb-2 instruction, I can't see any fails in schedlock.exp anymore!  See
> the patch below.
>
> Out of IT block, we can use 16-bit thumb breakpoint for any thumb code.  In
> IT block, we can use 16-bit thumb breakpoint for 16-bit instruction, and
> 32-bit thumb-2 breakpoint for 4-byte aligned 32-bit thumb-2 instruction.
> However, we can not use either 16-bit or 32-bit breakpoint for 2-byte
> aligned 32-bit thumb-2 instruction in IT block.
>
> The reason we can't use 16-bit breakpoint on a 32-bit instruction in IT
> block is that 16-bit breakpoint instruction makes the second half of
> 32-bit instruction to another different 16-bit instruction, and the 16-bit
> breakpoint instruction may not be executed at all, so the second half
> of instruction may be executed, and the result is completely unknown.
> GDB sets two breakpoints on two branches, "if" branch and "else" branch,
> because GDB doesn't know how does the instruction to be executed affect
> the flag.
>
> 	      /* There are conditional instructions after this one.
> 		 If this instruction modifies the flags, then we can
> 		 not predict what the next executed instruction will
> 		 be.  Fortunately, this instruction is architecturally
> 		 forbidden to branch; we know it will fall through.
> 		 Start by skipping past it.  */
>
> GDB/GDBserver has to emulate the instruction on how does it affect the
> flag, and only insert the breakpoint on the "true" branch.  Since the
> target instruction will be definitely executed, we can safely use
> 16-bit breakpoint instruction.

Ouch, reading the kernel thread it looks like this emulation would be
complex to say the least.

I think it would be better to get the current single stepping working
with the stop all threads logic since GDBServer was working like that
when GDB was doing the single stepping anyway. This would fix the current
regression.

Then work could be done to improve GDBServer to be better at
non-stopping.

WDYT ?

Thanks,
Antoine