Re: [RFC PATCH v2] accel/tcg: re-factor plugin_inject_cb so we can assert insn_idx is valid

[Richard Henderson <richard.henderson@linaro.org>]

On 9/14/21 5:04 AM, Alex Bennée wrote:
> Coverity doesn't know enough about how we have arranged our plugin TCG
> ops to know we will always have incremented insn_idx before injecting
> the callback. Let us assert it for the benefit of Coverity and protect
> ourselves from accidentally breaking the assumption and triggering
> harder to grok errors deeper in the code if we attempt a negative
> indexed array lookup.
> 
> However to get to this point we re-factor the code and remove the
> second hand instruction boundary detection in favour of scanning the
> full set of ops and using the existing INDEX_op_insn_start to cleanly
> detect when the instruction has started. As we no longer need the
> plugin specific list of ops we delete that.
> 
> My initial benchmarks shows no discernible impact of dropping the
> plugin specific ops list.
> 
> Fixes: Coverity 1459509
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/tcg/tcg.h      |   3 -
>   accel/tcg/plugin-gen.c | 157 ++++++++++++++++++++++-------------------
>   2 files changed, 85 insertions(+), 75 deletions(-)
> 
> diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
> index 44ccd86f3e..49a02db7e6 100644
> --- a/include/tcg/tcg.h
> +++ b/include/tcg/tcg.h
> @@ -604,9 +604,6 @@ struct TCGContext {
>   
>       /* descriptor of the instruction being translated */
>       struct qemu_plugin_insn *plugin_insn;
> -
> -    /* list to quickly access the injected ops */
> -    QSIMPLEQ_HEAD(, TCGOp) plugin_ops;
>   #endif
>   
>       GHashTable *const_table[TCG_TYPE_COUNT];
> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
> index 88e25c6df9..f145b815c0 100644
> --- a/accel/tcg/plugin-gen.c
> +++ b/accel/tcg/plugin-gen.c
> @@ -163,11 +163,7 @@ static void gen_empty_mem_helper(void)
>   static void gen_plugin_cb_start(enum plugin_gen_from from,
>                                   enum plugin_gen_cb type, unsigned wr)
>   {
> -    TCGOp *op;
> -
>       tcg_gen_plugin_cb_start(from, type, wr);
> -    op = tcg_last_op();
> -    QSIMPLEQ_INSERT_TAIL(&tcg_ctx->plugin_ops, op, plugin_link);

The plugin_link field of TCGOp is now dead too.  Otherwise,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~