* [PATCH] accel/tcg: assert insn_idx will always be valid before plugin_inject_cb
@ 2021-09-03 14:59 Alex Bennée
2021-09-12 21:37 ` Richard Henderson
0 siblings, 1 reply; 6+ messages in thread
From: Alex Bennée @ 2021-09-03 14:59 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, Richard Henderson, Alex Bennée, Paolo Bonzini
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.
Fixes: Coverity 1459509
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
accel/tcg/plugin-gen.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 88e25c6df9..b38aa1bb36 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -820,10 +820,9 @@ static void pr_ops(void)
static void plugin_gen_inject(const struct qemu_plugin_tb *plugin_tb)
{
TCGOp *op;
- int insn_idx;
+ int insn_idx = -1;
pr_ops();
- insn_idx = -1;
QSIMPLEQ_FOREACH(op, &tcg_ctx->plugin_ops, plugin_link) {
enum plugin_gen_from from = op->args[0];
enum plugin_gen_cb type = op->args[1];
@@ -834,6 +833,7 @@ static void plugin_gen_inject(const struct qemu_plugin_tb *plugin_tb)
type == PLUGIN_GEN_ENABLE_MEM_HELPER) {
insn_idx++;
}
+ g_assert(from == PLUGIN_GEN_FROM_TB || insn_idx >= 0);
plugin_inject_cb(plugin_tb, op, insn_idx);
}
pr_ops();
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] accel/tcg: assert insn_idx will always be valid before plugin_inject_cb
2021-09-03 14:59 [PATCH] accel/tcg: assert insn_idx will always be valid before plugin_inject_cb Alex Bennée
@ 2021-09-12 21:37 ` Richard Henderson
2021-09-13 10:06 ` Alex Bennée
0 siblings, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2021-09-12 21:37 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: peter.maydell, Paolo Bonzini
On 9/3/21 7:59 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.
>
> Fixes: Coverity 1459509
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> accel/tcg/plugin-gen.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
> index 88e25c6df9..b38aa1bb36 100644
> --- a/accel/tcg/plugin-gen.c
> +++ b/accel/tcg/plugin-gen.c
> @@ -820,10 +820,9 @@ static void pr_ops(void)
> static void plugin_gen_inject(const struct qemu_plugin_tb *plugin_tb)
> {
> TCGOp *op;
> - int insn_idx;
> + int insn_idx = -1;
>
> pr_ops();
> - insn_idx = -1;
> QSIMPLEQ_FOREACH(op, &tcg_ctx->plugin_ops, plugin_link) {
> enum plugin_gen_from from = op->args[0];
> enum plugin_gen_cb type = op->args[1];
> @@ -834,6 +833,7 @@ static void plugin_gen_inject(const struct qemu_plugin_tb *plugin_tb)
> type == PLUGIN_GEN_ENABLE_MEM_HELPER) {
> insn_idx++;
> }
> + g_assert(from == PLUGIN_GEN_FROM_TB || insn_idx >= 0);
> plugin_inject_cb(plugin_tb, op, insn_idx);
Hmm. This is the single caller of plugin_inject_cb.
I think we could simplify all of this by inlining it, so that we can put these blocks into
their proper place within the switch.
Also, existing strageness in insn_idx being incremented for non-insns? Should it be named
something else? I haven't looked at how it's really used in the end.
r~
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] accel/tcg: assert insn_idx will always be valid before plugin_inject_cb
2021-09-12 21:37 ` Richard Henderson
@ 2021-09-13 10:06 ` Alex Bennée
2021-09-13 12:55 ` Richard Henderson
0 siblings, 1 reply; 6+ messages in thread
From: Alex Bennée @ 2021-09-13 10:06 UTC (permalink / raw)
To: Richard Henderson; +Cc: peter.maydell, qemu-devel, Paolo Bonzini
Richard Henderson <richard.henderson@linaro.org> writes:
> On 9/3/21 7:59 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.
>> Fixes: Coverity 1459509
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>> accel/tcg/plugin-gen.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
>> index 88e25c6df9..b38aa1bb36 100644
>> --- a/accel/tcg/plugin-gen.c
>> +++ b/accel/tcg/plugin-gen.c
>> @@ -820,10 +820,9 @@ static void pr_ops(void)
>> static void plugin_gen_inject(const struct qemu_plugin_tb *plugin_tb)
>> {
>> TCGOp *op;
>> - int insn_idx;
>> + int insn_idx = -1;
>> pr_ops();
>> - insn_idx = -1;
>> QSIMPLEQ_FOREACH(op, &tcg_ctx->plugin_ops, plugin_link) {
>> enum plugin_gen_from from = op->args[0];
>> enum plugin_gen_cb type = op->args[1];
>> @@ -834,6 +833,7 @@ static void plugin_gen_inject(const struct qemu_plugin_tb *plugin_tb)
>> type == PLUGIN_GEN_ENABLE_MEM_HELPER) {
>> insn_idx++;
>> }
>> + g_assert(from == PLUGIN_GEN_FROM_TB || insn_idx >= 0);
>> plugin_inject_cb(plugin_tb, op, insn_idx);
>
> Hmm. This is the single caller of plugin_inject_cb.
>
> I think we could simplify all of this by inlining it, so that we can
> put these blocks into their proper place within the switch.
I guess. This was the simplest form for calming coverity but I can
experiment with more refactoring.
> Also, existing strageness in insn_idx being incremented for non-insns?
It shouldn't be - it's just using the presence of the memory
instrumentation as a proxy for the start of a instruction and dealing
with the slightly different start of block boundary.
> Should it be named something else? I haven't looked at how it's
> really used in the end.
We need the insn idx to find the registered callbacks for a given
instruction later. We could maybe embed a metadata TCGOp that could
track this for us but that might make TCG a bit more confusing as it
doesn't really need that information?
>
>
> r~
--
Alex Bennée
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] accel/tcg: assert insn_idx will always be valid before plugin_inject_cb
2021-09-13 10:06 ` Alex Bennée
@ 2021-09-13 12:55 ` Richard Henderson
2021-09-13 14:06 ` Alex Bennée
0 siblings, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2021-09-13 12:55 UTC (permalink / raw)
To: Alex Bennée; +Cc: peter.maydell, qemu-devel, Paolo Bonzini
On 9/13/21 3:06 AM, Alex Bennée wrote:
>> Also, existing strageness in insn_idx being incremented for non-insns?
>
> It shouldn't be - it's just using the presence of the memory
> instrumentation as a proxy for the start of a instruction and dealing
> with the slightly different start of block boundary.
>
>> Should it be named something else? I haven't looked at how it's
>> really used in the end.
>
> We need the insn idx to find the registered callbacks for a given
> instruction later. We could maybe embed a metadata TCGOp that could
> track this for us but that might make TCG a bit more confusing as it
> doesn't really need that information?
We have a metadata op for marking instruction boundaries already: INDEX_op_insn_start.
r~
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] accel/tcg: assert insn_idx will always be valid before plugin_inject_cb
2021-09-13 12:55 ` Richard Henderson
@ 2021-09-13 14:06 ` Alex Bennée
2021-09-13 14:11 ` Richard Henderson
0 siblings, 1 reply; 6+ messages in thread
From: Alex Bennée @ 2021-09-13 14:06 UTC (permalink / raw)
To: Richard Henderson; +Cc: peter.maydell, qemu-devel, Paolo Bonzini
Richard Henderson <richard.henderson@linaro.org> writes:
> On 9/13/21 3:06 AM, Alex Bennée wrote:
>>> Also, existing strageness in insn_idx being incremented for non-insns?
>> It shouldn't be - it's just using the presence of the memory
>> instrumentation as a proxy for the start of a instruction and dealing
>> with the slightly different start of block boundary.
>>
>>> Should it be named something else? I haven't looked at how it's
>>> really used in the end.
>> We need the insn idx to find the registered callbacks for a given
>> instruction later. We could maybe embed a metadata TCGOp that could
>> track this for us but that might make TCG a bit more confusing as it
>> doesn't really need that information?
>
> We have a metadata op for marking instruction boundaries already:
> INDEX_op_insn_start.
Hmm so we have a separate list for speedy access:
/* list to quickly access the injected ops */
QSIMPLEQ_HEAD(, TCGOp) plugin_ops;
I wonder if we should drop that and just scan QTAILQ_HEAD(, TCGOp) ops
so we can be properly aligned with the current instruction.
Alternatively we could just emit INDEX_op_insn_start to the plugin list
as well?
>
>
> r~
--
Alex Bennée
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] accel/tcg: assert insn_idx will always be valid before plugin_inject_cb
2021-09-13 14:06 ` Alex Bennée
@ 2021-09-13 14:11 ` Richard Henderson
0 siblings, 0 replies; 6+ messages in thread
From: Richard Henderson @ 2021-09-13 14:11 UTC (permalink / raw)
To: Alex Bennée; +Cc: peter.maydell, qemu-devel, Paolo Bonzini
On 9/13/21 7:06 AM, Alex Bennée wrote:
> Hmm so we have a separate list for speedy access:
>
> /* list to quickly access the injected ops */
> QSIMPLEQ_HEAD(, TCGOp) plugin_ops;
>
> I wonder if we should drop that and just scan QTAILQ_HEAD(, TCGOp) ops
> so we can be properly aligned with the current instruction.
> Alternatively we could just emit INDEX_op_insn_start to the plugin list
> as well?
I suspect that just scanning the whole list would be easiest. Then you don't need to
maintain your own separate list.
r~
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-13 14:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-09-03 14:59 [PATCH] accel/tcg: assert insn_idx will always be valid before plugin_inject_cb Alex Bennée
2021-09-12 21:37 ` Richard Henderson
2021-09-13 10:06 ` Alex Bennée
2021-09-13 12:55 ` Richard Henderson
2021-09-13 14:06 ` Alex Bennée
2021-09-13 14:11 ` Richard Henderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).