* [Qemu-trivial] [PATCH] tci: Remove invalid assertions
@ 2017-02-02 19:56 Stefan Weil
2017-02-02 20:00 ` [Qemu-trivial] [Qemu-devel] " Eric Blake
2017-02-03 12:32 ` [Qemu-trivial] " Peter Maydell
0 siblings, 2 replies; 6+ messages in thread
From: Stefan Weil @ 2017-02-02 19:56 UTC (permalink / raw)
To: Richard Henderson; +Cc: Peter Maydell, qemu-devel, qemu-trivial, Stefan Weil
tb_jmp_insn_offset and tb_jmp_reset_offset are pointers
and cannot be used with ARRAY_SIZE.
Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
tcg/tci/tcg-target.inc.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/tcg/tci/tcg-target.inc.c b/tcg/tci/tcg-target.inc.c
index 26ee9b1664..b6a15569f8 100644
--- a/tcg/tci/tcg-target.inc.c
+++ b/tcg/tci/tcg-target.inc.c
@@ -566,7 +566,6 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
case INDEX_op_goto_tb:
if (s->tb_jmp_insn_offset) {
/* Direct jump method. */
- tcg_debug_assert(args[0] < ARRAY_SIZE(s->tb_jmp_insn_offset));
/* Align for atomic patching and thread safety */
s->code_ptr = QEMU_ALIGN_PTR_UP(s->code_ptr, 4);
s->tb_jmp_insn_offset[args[0]] = tcg_current_code_size(s);
@@ -575,7 +574,6 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
/* Indirect jump method. */
TODO();
}
- tcg_debug_assert(args[0] < ARRAY_SIZE(s->tb_jmp_reset_offset));
s->tb_jmp_reset_offset[args[0]] = tcg_current_code_size(s);
break;
case INDEX_op_br:
--
2.11.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-trivial] [Qemu-devel] [PATCH] tci: Remove invalid assertions
2017-02-02 19:56 [Qemu-trivial] [PATCH] tci: Remove invalid assertions Stefan Weil
@ 2017-02-02 20:00 ` Eric Blake
2017-02-02 20:10 ` Stefan Weil
2017-02-03 12:32 ` [Qemu-trivial] " Peter Maydell
1 sibling, 1 reply; 6+ messages in thread
From: Eric Blake @ 2017-02-02 20:00 UTC (permalink / raw)
To: Stefan Weil, Richard Henderson; +Cc: qemu-trivial, Peter Maydell, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 487 bytes --]
On 02/02/2017 01:56 PM, Stefan Weil wrote:
> tb_jmp_insn_offset and tb_jmp_reset_offset are pointers
> and cannot be used with ARRAY_SIZE.
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> tcg/tci/tcg-target.inc.c | 2 --
> 1 file changed, 2 deletions(-)
mst posted an alternative patch:
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00551.html
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH] tci: Remove invalid assertions
2017-02-02 20:00 ` [Qemu-trivial] [Qemu-devel] " Eric Blake
@ 2017-02-02 20:10 ` Stefan Weil
2017-02-02 20:25 ` Eric Blake
2017-02-02 22:12 ` Michael S. Tsirkin
0 siblings, 2 replies; 6+ messages in thread
From: Stefan Weil @ 2017-02-02 20:10 UTC (permalink / raw)
To: Eric Blake
Cc: Richard Henderson, qemu-trivial, Peter Maydell, qemu-devel,
Michael S. Tsirkin
Am 02.02.2017 um 21:00 schrieb Eric Blake:
> On 02/02/2017 01:56 PM, Stefan Weil wrote:
>> tb_jmp_insn_offset and tb_jmp_reset_offset are pointers
>> and cannot be used with ARRAY_SIZE.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>> tcg/tci/tcg-target.inc.c | 2 --
>> 1 file changed, 2 deletions(-)
>
> mst posted an alternative patch:
> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00551.html
Yes, I noticed that, too. It's not obvious that this new
assertion will be correct, and none of the other targets
has that kind of assertion. Only two targets use an
assertion which detects NULL pointers, but NULL pointers
will result in an abort anyway.
Do you think that there are reasons why TCI should use
the assertion suggested by Michael?
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH] tci: Remove invalid assertions
2017-02-02 20:10 ` Stefan Weil
@ 2017-02-02 20:25 ` Eric Blake
2017-02-02 22:12 ` Michael S. Tsirkin
1 sibling, 0 replies; 6+ messages in thread
From: Eric Blake @ 2017-02-02 20:25 UTC (permalink / raw)
To: Stefan Weil
Cc: Richard Henderson, qemu-trivial, Peter Maydell, qemu-devel,
Michael S. Tsirkin
[-- Attachment #1.1: Type: text/plain, Size: 1050 bytes --]
On 02/02/2017 02:10 PM, Stefan Weil wrote:
> Am 02.02.2017 um 21:00 schrieb Eric Blake:
>> On 02/02/2017 01:56 PM, Stefan Weil wrote:
>>> tb_jmp_insn_offset and tb_jmp_reset_offset are pointers
>>> and cannot be used with ARRAY_SIZE.
>>>
>> mst posted an alternative patch:
>> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00551.html
>
>
> Yes, I noticed that, too. It's not obvious that this new
> assertion will be correct, and none of the other targets
> has that kind of assertion. Only two targets use an
> assertion which detects NULL pointers, but NULL pointers
> will result in an abort anyway.
>
> Do you think that there are reasons why TCI should use
> the assertion suggested by Michael?
I don't have any strong opinions on which patch is better, so much as
just pointing out that alternative proposals exist so that we have good
threading in making the decision on which one to go with.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH] tci: Remove invalid assertions
2017-02-02 20:10 ` Stefan Weil
2017-02-02 20:25 ` Eric Blake
@ 2017-02-02 22:12 ` Michael S. Tsirkin
1 sibling, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2017-02-02 22:12 UTC (permalink / raw)
To: Stefan Weil
Cc: Eric Blake, Richard Henderson, qemu-trivial, Peter Maydell,
qemu-devel
On Thu, Feb 02, 2017 at 09:10:26PM +0100, Stefan Weil wrote:
> Am 02.02.2017 um 21:00 schrieb Eric Blake:
> > On 02/02/2017 01:56 PM, Stefan Weil wrote:
> > > tb_jmp_insn_offset and tb_jmp_reset_offset are pointers
> > > and cannot be used with ARRAY_SIZE.
> > >
> > > Signed-off-by: Stefan Weil <sw@weilnetz.de>
> > > ---
> > > tcg/tci/tcg-target.inc.c | 2 --
> > > 1 file changed, 2 deletions(-)
> >
> > mst posted an alternative patch:
> > https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00551.html
>
>
> Yes, I noticed that, too. It's not obvious that this new
> assertion will be correct, and none of the other targets
> has that kind of assertion. Only two targets use an
> assertion which detects NULL pointers, but NULL pointers
> will result in an abort anyway.
>
> Do you think that there are reasons why TCI should use
> the assertion suggested by Michael?
>
> Stefan
You know what this code does and I don't, not really.
I just did a monkey patch guessing at what was intended
(value is used as an array index, so we do a bounds check).
I sent the patch before I saw yours simply to fix the build in a way
that's as unintrusive as possible: args[0] seemed to come from guest so
I thought it might be prudent to do a bounds check.
So feel free to ignore mine. Here's an ack for yours
Acked-by: Michael S. Tsirkin <mst@redhat.com>
--
MST
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-trivial] [PATCH] tci: Remove invalid assertions
2017-02-02 19:56 [Qemu-trivial] [PATCH] tci: Remove invalid assertions Stefan Weil
2017-02-02 20:00 ` [Qemu-trivial] [Qemu-devel] " Eric Blake
@ 2017-02-03 12:32 ` Peter Maydell
1 sibling, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2017-02-03 12:32 UTC (permalink / raw)
To: Stefan Weil; +Cc: Richard Henderson, QEMU Developers, QEMU Trivial
On 2 February 2017 at 19:56, Stefan Weil <sw@weilnetz.de> wrote:
> tb_jmp_insn_offset and tb_jmp_reset_offset are pointers
> and cannot be used with ARRAY_SIZE.
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> tcg/tci/tcg-target.inc.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/tcg/tci/tcg-target.inc.c b/tcg/tci/tcg-target.inc.c
> index 26ee9b1664..b6a15569f8 100644
> --- a/tcg/tci/tcg-target.inc.c
> +++ b/tcg/tci/tcg-target.inc.c
> @@ -566,7 +566,6 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
> case INDEX_op_goto_tb:
> if (s->tb_jmp_insn_offset) {
> /* Direct jump method. */
> - tcg_debug_assert(args[0] < ARRAY_SIZE(s->tb_jmp_insn_offset));
> /* Align for atomic patching and thread safety */
> s->code_ptr = QEMU_ALIGN_PTR_UP(s->code_ptr, 4);
> s->tb_jmp_insn_offset[args[0]] = tcg_current_code_size(s);
> @@ -575,7 +574,6 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
> /* Indirect jump method. */
> TODO();
> }
> - tcg_debug_assert(args[0] < ARRAY_SIZE(s->tb_jmp_reset_offset));
> s->tb_jmp_reset_offset[args[0]] = tcg_current_code_size(s);
> break;
> case INDEX_op_br:
> --
> 2.11.0
Applied to master as a buildfix; thanks.
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-03 12:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-02 19:56 [Qemu-trivial] [PATCH] tci: Remove invalid assertions Stefan Weil
2017-02-02 20:00 ` [Qemu-trivial] [Qemu-devel] " Eric Blake
2017-02-02 20:10 ` Stefan Weil
2017-02-02 20:25 ` Eric Blake
2017-02-02 22:12 ` Michael S. Tsirkin
2017-02-03 12:32 ` [Qemu-trivial] " Peter Maydell
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).