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

[Antoine Tremblay <antoine dot tremblay at ericsson dot com>]

Yao Qi writes:

> On 17-01-30 08:28:29, Antoine Tremblay wrote:
>>
>> Yao Qi writes:
>>
>> > On Fri, Jan 27, 2017 at 6:24 PM, Antoine Tremblay
>> > <antoine.tremblay@ericsson.com> wrote:
>> >>
>> >> Yao Qi writes:
>> >>
>> >>> On Fri, Jan 27, 2017 at 4:06 PM, Antoine Tremblay
>> >>> <antoine.tremblay@ericsson.com> wrote:
>> >
>> >>> If emulation is complex, probably
>> >>> we can partially fix this problem by "always using 16-bit thumb instruction
>> >>> if program is out of IT block".
>> >>>
>> >>
>> >> I would be against that since it would leave the feature partially
>> >> working and crashing the program when it fails...
>> >>
>> >> It would also be a regression compared to previous GDBServer.
>> >
>> > There won't be any regression.  16-bit thumb breakpoint works pretty well
>> > for any thumb instructions (16-bit and 32-bit) if program is out of IT block.
>> > The 32-bit thumb-2 breakpoint was added for single step IT block.
>>
>> Yes so there will be a regression for single step inside an IT block if
>> we keep using the 32 bit breakpoint since this one can be non atomic if
>> it's not aligned properly.
>>
>
> It does fail, but not a regression, because current GDBserver fails to
> write 32-bit thumb breakpoint to 2-byte aligned 32-bit instruction
> atomically, regardless the program is within IT block or not.

I mean it's a regression compared to GDB 7.11. In 7.11 GDBServer will
stop all threads write the 4 bytes and then restart all threads so the
issue is not present.

Yes it wasn't atomic before either but it did not create an issue due to
the stopping of the threads.

Or maybe I misunderstand what you mean... ?

> My
> suggestion is that "let us fix this problem when program is out of IT
> block by using 16-bit thumb breakpoint".  That will fix the issue
> of atomic write when program is out of IT block, and leave the problem
> there when program is within IT block.  Why is it a regression?
>

Well like I said before in 7.11 because GDBServer stopped the threads to
write the 4 bytes, it did not have to be atomic and schedlock.exp etc
passed, single stepping a program even with a IT block did not fail.

I would like to re-validate this since you're introducing doubt into my
mind but I can't at the moment, I hope my memory serves me right but I
have not retested this now.

>> We could write a test case for it and it would fail like schedlock
>> did. But it would not with the single stepping controlled by GDB like it
>> was before.
>>
>> >
>> >> Also, IT blocks are a common place to have a breakpoint/tracepoint.
>> >>
>> >
>> > We don't change anything when setting breakpoint inside IT block.
>>
>> Well that's a problem if we write a 32 bit thumb2 breakpoint aligned on
>> 2 bytes like discussed before.
>>
>
> That is just the sub-set of the original problem.
>
>> >
>> >>>> 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 ?
>> >>>>
>> >>>
>> >>> Sounds like we are applying the ARM linux limitation to a general
>> >>> place.
>> >>> Other software single step targets may write breakpoint in atomic way,
>> >>> so we don't need to stop all threads.  Even in -marm mode, we don't
>> >>> have to stop all threads on inserting breakpoints.
>> >>
>> >> Well ARM is the only software single stepping target, while I agreee
>> >> that we would be affecting general code, I think that since there is
>> >> no software single step breakpoint target that supports atomic
>> >> breakpoint writes at the moment it's normal that the run control
>> >> doesn't support that.
>> >
>> > No, ARM Linux ptrace POKETEXT is _atomic_ if the address is 4-byte
>> > aligned, so 32-bit arm breakpoint and 16-bit thumb breakpoint can be
>> > written atomically.  32-bit thumb-2 breakpoint can be written atomically
>> > too if the address is 4-byte aligned.
>> >
>> > The only problem we have is inserting a breakpoint on a 2-byte aligned
>> > 32-bit thumb-2 instruction inside IT block, we can not use neither 16-bit thumb
>> > breakpoint nor 32-bit thumb breakpoint.  Everything works fine in other
>> > cases.
>> >
>>
>> I mean software single stepping as a whole, which means considering all
>> cases, is not atomic. I don't see how we could leave that case
>> unaddressed ?
>>
>> Especially since it will crash the inferior ?
>>
>
> I am thinking how do we make progress here.  Nowadays, the multi-threaded
> program may crash almost everywhere, but we can partially fix it to an
> extent that the multi-threaded program may only crash in side IT block.
> Is it a progress?  I feel it is a good example to apply "divide and
> conquer" to a complicated engineering problem.  We can easily fix the
> first half of this problem, and then think about the second half.
>

Well I agree with the divide an conquer approach, I would just have
divided it another way by stopping all threads so that we fix all the
problem right now, and then think about the second half which would be
to allow non-stop operations.

The solution to the program crashing in the IT block seems non-trivial
and I don't know how much time will pass before a fix is done.  I'm
afraid this would linger for a long time but maybe I'm wrong, do you plan
to address the second part for 8.0 too ? 

I would feel better if GDB worked for all cases meanwhile.

This is more important to me than not stopping the threads, especially
since in 7.11 the threads were stopped.

My point is that if we can fix the problem completely now while we think
about a better solution isn't that preferable to leaving GDB partially
fixed ?

>> >>
>> >> I don't count -marm as an arch since there no way to check that all the
>> >> program including shared libs etc is -marm, I don't think we could make
>> >> the distinction in GDBServer AFAIK.
>> >
>> > We can check with -mthumb.  My hack in last email fixes fails in
>> > schedlock.exp in thumb mode.
>>
>> Yes but schedlock.exp is not made to test the 2 byte aligned thumb2
>> 32-bit breakpoint IIRC.
>>
>
> We can write one test for single step 2-byte aligned thumb-2 32-bit
> instruction.  No problem.