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

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

Antoine Tremblay writes:

> 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.

Sorry for what may seem like a monologue there, but we can't read past
breakpoitns etc all the time since we have know idea of the memory
aligment involved, we don't want to check around a single byte read to
see if it looks like a breakpoint.

So before any call to get_next_pc, we need to remove everthing, I'll
send a patch in that regard.

Thanks,
Antoine