Re: [PATCH 0/9 V3] Use reinsert breakpoint for vCont;s

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

Yao Qi writes:

> On Mon, Nov 21, 2016 at 10:34:44AM -0500, Antoine Tremblay wrote:
>> > Thread 1 either sees the original instruction on address A or the
>> > breakpoint instruction.  Unless ptrace read/write 32-bit is not
>> > atomic, IOW, partial ptrace write result is visible to other
>> > threads, I don't see why we get SIGILL here.
>> 
>> I think this is the problem, ptrace read/write doesn't seem to be
>> atomic, and thread 1 sees some half written memory. (Given that we get
>> SIGILL/SIGSEGV issues)
>
> We need to check in linux-arm-kernel@.
>
>> 
>> Did you have any reference suggesting it was atomic ?
>> 
>
> No.
>
>> While testing it seems to be atomic for 32bit writes but in thumb mode
>> with a 16 byte write, it is not.
>
> I think you meant "16 bit write".  Why is that?
>

Yes 16 bit write sorry, because it can write a thumb breakpoint :
0xde01.

>> 
>> Given the SIGILL/SIGSEG I get maybe that one is 2 writes of 1 byte ?
>> I'll have to dig in the ptrace code I guess.
>> 
>
> It is good to get some a clear answer instead of ambiguous speculation.
> I think we need to ask in linux-arm-kernel@

Did you see my follow up email ? :
https://sourceware.org/ml/gdb-patches/2016-11/msg00681.html

Also, I think this will become a moot point in the patch I'm about to
post since:

To install a single step breakpoint on a thread GDBServer needs to make sure
that there is not a breakpoint at the thread's current pc, since it
can't determine what is the next_pc of a breakpoint instruction.

Usually for stepping over it's OK since it's stopped at pc X and it
will install a single-step breakpoint at pc X + next_pc_offset.

So need_step_over returns true and GDBServer starts a step_over process,
which removes all breakpoints, installs a single-step breakpoint on the
nextpc and resumes.

But in this case it is installing single-step breakpoints in threads at
different pcs then the one we're stopped, so the step-over process is
not triggered and it should not be.

So GDBSever does not take care to remove all breakpoints like is the
case in the step-over process.  Because of that it can try to install a
single-step breakpoint where there is already a breakpoint in memory and
thus break get_next_pc and install a breakpoint at an invalid location.

Consider this case:

in non-stop, thread 1-3 are stepping in a loop similar to
non-stop-fair-events test.

 - thread 1 hits its single-step breakpoint at pc A.
 - delete its single-step breakpoint.
 - a check for need_step_over is done, but there's no breakpoint at pc A
 anymore, and nobody is stopped there anyway so it returns false.
 - proceed_one_lwp is called on each thread.

 Now here is the problem:

 thread 1 is at pc A
 thread 2 is at pc B

 B is a branch to A.
 
 thread 1 installs a single-step breakpoint at pc B since it's range stepping.
 thread 2 does not have a single step breakpoint but needs one installed.
 
 - proceed_one_lwp finds that it needs to install a single-step
   breakpoint on thread 2.

 - It calls install_single_step_breakpoints, which calls get_next_pc.

 - get_next_pc reads the current instruction in memory at pc B, but
   since it's a breakpoint, it missinterprets the instruction, you can't
   step over a breakpoint like that anyway, but this is what happens
   now.

   A single-step breakpoint is now inserted at an invalid location.

So my approch in my patch is to fix this by always removing all
breakpoints and fast_tracepoints_jumps, like we do in start_step_over
before calling install_software_single_step.

This makes the breakpoint installation a multiple steps process and thus
can't be atomic.

WDYT ?

Thanks,
Antoine