[RFC] kprobes with thumb2 conditional code

[Dave Martin]

On Thu, Mar 17, 2011 at 10:46 PM, Nicolas Pitre
<nicolas.pitre at linaro.org> wrote:
> On Thu, 17 Mar 2011, Dave Martin wrote:
>
>> On Fri, Mar 11, 2011 at 5:01 PM, Tixy <tixy at yxit.co.uk> wrote:
>> > Hello All
>> >
>> > I'm about to start work on getting kprobes working with thumb2.
>> >
>> > One of the issues I have is that when probes are placed onto
>> > conditionally executed instructions in a IT block, they may not fire if
>> > the condition is not met. This is because on ARM we use invalid
>> > instructions for breakpoints, and the ARM ARM says:
>> >
>> >  "it is IMPLEMENTATION DEFINED whether the instruction executes as a
>> >   NOP or causes an Undefined Instruction exception"
>> >
>> > This is a similar issue to that already encountered by GDB [1][2]
>> >
>> > If we take this code as an example...
>> >
>> > probe1:
>> > if(condition) {
>> > probe2:
>> >    some_code;
>> > }
>> >
>> > It seams reasonable at first sight that probe2 would only fire if the
>> > condition is true. This will always be the case if the compiler
>> > generates test-and-branch code, but if it generates conditionally
>> > executed instructions for 'some_code' then it gets complicated...
>> >
>> > The current arm kprobes implementation will always fire probe2 because
>>
>> Well, actually it's somewhat unpredictable whether/when a probe on
>> probe2 will fire, because it depends on the code generated by the
>> compiler.  The compiler could choose to use a conditional instruction
>> at probe2, or it could conditionally branch round it, even in ARM.
>
> And we definitely don't want that.
>
>> Or, the whole conditional might disappear to be replaced with an
>> unconditional instruction sequence that has the same overall effect.
>
> Sure, but this is an extreme case which is not specific to ARM or
> Thumb2.  Such a situation could happen on x86 as well.  Let's not worry
> about that and keep ourselves to those cases we have control over.
>
>> > it uses unconditional instructions for its breakpoints. With thumb
>> > instructions we can't force unconditional execution, so we would have an
>> > 'implementation defined' situation whether it would fire or not when the
>> > condition is false. (Thought you would hope it would be consistent on a
>> > particular device.)
>> >
>> > Some options for dealing with this, in increasing order of
>> > complexity...
>> >
>> > 1. Accept the situation as described.
>> >
>> > 2. Change arm probes to use conditional instructions so we would
>> > (hopefully) have consistent undefined behaviour in both arm and thumb
>> > code. (If that isn't an oxymoron :-)
>> >
>> > 3. Do 2, and modify kprobe_handler to test for false firings (breakpoint
>> > when condition false) and not execute the probe's callback functions in
>> > these cases. E.g. consistently make probe2 appear to not fire when
>> > condition is false.
>>
>> My preference would be for option (1) or (3).  For (3), we could
>> choose to extend this behaviour to cover the existing ARM
>> implementation as well as Thumb, which could be a tidier and more
>> consistent approach.  This seems to be the "right" thing, since it
>> means kprobes never fire for real on instructions which are logically
>> not executed, no matter how the compiler (or human) implemented the
>> conditionality.  Currently, ARM kprobes can fire on instructions which
>> would fail their condition check, which might be considered wrong.
>
> Definitely #3.
>
> In the ARM case, this can be achieved by saving the condition code
> of the replaced instruction when installing a probe, and test it
> against the CPSR value from the interrupted state.  If the condition is
> false then just skip the native emulation of the resulting NOP and
> ignore the probe callback.  In the Thumb2 case the IT state in the CPSR
> would indicate right away if the probe should be ignored.

We could also insert a conditional undefined instruction for the probe
-- this may reduce the number of false positives, but it depends on
the hardware implementation.
Since kprobes needs to do explicit condition code checks anyway, use
of conditional undefs doesn't add any functionality, however.

>
>> Alternatively, we could avoid fixing what isn't broken and leave the
>> ARM implementation unchanged.
>
> I wouldn't call the ARM implementation not broken.  It just happens that
> probes are typically installed at the beginning of functions and no
> conditional instructions are normally placed there.  This would explain
> why no one reported this issue so far.

Well, fair enough -- if there's appetite for tidying this up, and we
don't think it will break anything anyone else is doing, then I'd say
fixing it is a good idea.

>
>> Since the compiler will implement some
>> different instruction-level optimisations in Thumb compared with ARM,
>> it may not make sense to try to make the kprobe firing behaviour
>> behave exactly the same around conditional code in both instruction
>> sets -- because the code generation differences may always cause the
>> observed behaviour to be different anyway, just as might happen if you
>> switch to a newer compiler or use a different -march/-mtune
>> combination.
>
> That doesn't matter.  We should respect the expected execution flow at
> the probe location irrespective of any compiler optimization.  It is
> true that code generation might be different between different compilers
> and options, but that's not our problem as long as we don't introduce
> more randomness.  And it happens that making this work properly is
> trivial with Thumb2.  And in ARM mode, the most complex task is to
> determine if a given instruction is conditionally executable or not.

Note that in Thumb, not everything that's conditional is inside an IT
block: there are a couple of conditional branch encodings which have
an explicit condition code.

It's still a pretty simple decoding task, but not quite as simple as
the ARM case.

Cheers
---Dave