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

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

Pedro Alves writes:

> On 02/17/2017 01:41 AM, Antoine Tremblay wrote:
>>
>> Pedro Alves writes:
>>
>>> On 01/30/2017 01:28 PM, Antoine Tremblay wrote:
>>>
>>>>> 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.
>>>
>>
>> Sorry for the delay I just saw your mail...
>>
>>> Can we restrict the stopping-all-threads to the case that
>>> needs it, only?
>>
>> Possibly but I would like to avoid introducing 2 execution paths in the
>> run control, it's already hard to follow as it is and it would introduce
>> a lot of code in the arch independant code just for this case, that's
>> something I would like to avoid too.
>
> I don't immediately see how this would imply introducing lots of code
> in the run control.

Well lots can be debatable, but at first glance you would basically need
this current posted patch + what it removes and a switch between the 2.

And I'm not sure how the IT block detection would be done.

It just seems like much to me, I'm actually very surprised that you're
suggesting having those two paths.

It seems like it creates much to maintain to support this particular
problem with the ARM breakpoints.

Maybe it's just that I had such a hard time getting all that run control
right, it's full of traps and corner cases with all-stop/non-stop,
stopping, suspending, deleting the breakpoints, re-adding them at the
right time etc... Adding more state is giving me quite a headache.

> We shouldn't be imposing this stop-all-threads
> on all archs, since it adds a lot of slowness (and
> the more threads the inferior has, the worse it gets).

I would be the first the agree usually that's something I would like to
avoid but considering that it was crashing the inferior in the only
architecture using this, not stopping seemed less important.

Also I'm not arguing to keep it like this forever but until there's a
better solution to the problem it seemed reasonable to me to take the
performance hit.

And I was pretty much certain that before another arch uses this we
would have figured it out.

Is there another arch on the horizon that would use this ?

>So if
> we already need the 2 execution paths, making the condition that
> determines whether to pause all threads consider more state
> doesn't seem to add that much complexity to the run control
> part itself.
>
>>> An optimization that would avoid this would be to use a
>>> hardware watchpoint/breakpoint (if available) for single-stepping.
>>> IIRC, you can ARM a breakpoint (or was it a watchpoint) on ARM for
>>> triggering when the PC is different from the current PC (or really,
>>> some specified address).
>>
>> I did not know that but I'm worried about the limited number of hardware
>> watchpoints available. IIRC on my board I had only 4, if GDBServer can't
>> find one available would it refuse to single step ? That would be
>> weird... ?
>
> My thought was that we'd give preference to user-requested
> watchpoints.  I.e., treat this as an optimization.  We always need to
> pause all threads in order to install watchpoints in all
> threads (watchpoints must be inserted with the thread paused, and
> we do that on thread resume).  So if a request for a user-watchpoints
> comes in, we'd just interrupt the current single-step as we currently
> do, install the watchpoints, and go back to single-stepping, if it didn't
> manage to finish, as we currently do.  At this point, we notice that we
> don't have free hardware watchpoints left, and fallback to do
> the slow software single-step as before.
>

OK I see. Interesting.

>>
>> It's an interesting approch however I'll dig about it more.
>>
>>>
>>> In IT blocks, this would probably make the thread stop on
>>> instructions that aren't really executed (e.g., the then/else
>>> branches when the condition is correspondingly false/true),
>>
>> Really ? I need to find something about that in the arch man...
>
> AFAIK, in IT blocks, all instructions always "execute", but, when
> the condition is false, the instruction becomes as-if a nop.
> The only reason breakpoints don't stop there currently is that
> breakpoints are just another instruction (actually an undefined
> instruction the kernel is aware of, that causes an undefined
> instruction exception that the kernel translates to a SIGTRAP
> instead of a SIGILL), and when the condition is false, the breakpoint
> instruction becomes a nop too...  If you have a hardware trap
> set to trap at such an address though, then it should trap, I believe,
> as if you had armed a hardware breakpoint to trap on the address
> of a real nop instruction.
>

Seems to make sense :) I'll test it out with a hardware breakpoint.

Thanks!,
Antoine