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

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

Antoine Tremblay writes:

> 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

In fact thinking more about this we may need to remove all breakpoints
at any pc since get_next_pc may read memory in other places then the
current pc to deal with atomic sequences for example or for other
instructions too.

If it reads a breakpoint in memory there it may come-up with an invalid
next pc.

This is a problem with the current step-over logic too.

So we would either need to be able to read past any
breakpoint/fast_tracepoint_jump... anywhere
or uninstall everything before calling get_next_pc.

I'm not sure which one is best at the moment, opinions on this are
welcome.

Thanks,
Antoine