Re: [PATCH v2] plugins: Set final instruction count in plugin_gen_tb_end

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 19/7/23 00:05, Alex Bennée wrote:
> 
> Matt Borgerson <contact@mborgerson.com> writes:
> 
>> Thanks Alex!
>>
>>
>> On Mon, Jul 17, 2023 at 8:34 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>>>
>>>
>>> Alex Bennée <alex.bennee@linaro.org> writes:
>>>
>>>> Matt Borgerson <contact@mborgerson.com> writes:
>>>>
>>>>> Translation logic may partially decode an instruction, then abort and
>>>>> remove the instruction from the TB. This can happen for example when an
>>>>> instruction spans two pages. In this case, plugins may get an incorrect
>>>>> result when calling qemu_plugin_tb_n_insns to query for the number of
>>>>> instructions in the TB. This patch updates plugin_gen_tb_end to set the
>>>>> final instruction count.
>>>>
>>>> For some reason this fails to apply cleanly:
>>>>
>>>>    git am ./v2_20230714_contact_plugins_set_final_instruction_count_in_plugin_gen_tb_end.mbx
>>>>    Applying: plugins: Set final instruction count in plugin_gen_tb_end
>>>>    error: corrupt patch at line 31
>>>>    Patch failed at 0001 plugins: Set final instruction count in
>>>>    plugin_gen_tb_end
>>>
>>> I think some newlines crept in, I was able to fix. Queued to
>>> for-8.1/misc-fixes with the assert added.
> 
> Hmm so I ran into an issue:
> 
>    ./qemu-sh4 -plugin tests/plugin/libbb.so -d plugin ./tests/tcg/sh4-linux-user/testthread
>    ERROR:../../accel/tcg/plugin-gen.c:874:plugin_gen_tb_end: assertion failed: (num_insns <= ptb->n)
>    Bail out! ERROR:../../accel/tcg/plugin-gen.c:874:plugin_gen_tb_end: assertion failed: (num_insns <= ptb->n)
>    qemu: uncaught target signal 11 (Segmentation fault) - core dumped
>    bb's: 9202, insns: 42264
>    fish: Job 1, './qemu-sh4 -plugin tests/plugin…' terminated by signal SIGSEGV (Address boundary error)
> 
> Further investigation shows that gUSA sequences can cause the number of
> instructions to run ahead, which also makes the setting of the ptb->n =
> num_insns unsafe, running ahead of the number of instructions signalled
> by plugin_gen_insn_start/plugin_gen_insn_end.
> 
>    Thread 1 hit Hardware watchpoint 5: *(int *) 0x7ffd410a2904
>    Old value = 4
>    New value = 1
>    0x000055f148c00ea8 in decode_gusa (ctx=0x7ffd410a28f0, env=0x55f14a4106e8) at ../../target/sh4/translate.c:2167
>    2167        ctx->base.num_insns += max_insns - 1;
>    (rr) p max_insns
>    $6 = 4
>    (rr) p max_insns -1
>    $7 = 3

Is this the 'fail' label? (You can check by using '-d unimp'
which would dump "Unrecognized gUSA sequence").

'fail' is followed by 'done' which updates ctx->base.num_insns.

>    (rr) p ctx->base.num_insns
>    $8 = 1
> 
> So I think we have to drop this for now until we can either fix
> decode_gusa or find something else.
> 
> Richard,
> 
> Any ideas?
> 
> 
>