1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] tci: Remove invalid assertions

Stefan Weil posted 1 patch 7 years, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170202195601.11286-1-sw@weilnetz.de
Test checkpatch passed
Test docker passed
Test s390x failed
tcg/tci/tcg-target.inc.c | 2 --
1 file changed, 2 deletions(-)
[Qemu-devel] [PATCH] tci: Remove invalid assertions
Posted by Stefan Weil 7 years, 2 months ago 
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
Re: [Qemu-devel] [PATCH] tci: Remove invalid assertions
Posted by Eric Blake 7 years, 2 months ago 
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
Re: [Qemu-devel] [PATCH] tci: Remove invalid assertions
Posted by Stefan Weil 7 years, 2 months ago 
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
Re: [Qemu-devel] [PATCH] tci: Remove invalid assertions
Posted by Eric Blake 7 years, 2 months ago 
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
Re: [Qemu-devel] [PATCH] tci: Remove invalid assertions
Posted by Michael S. Tsirkin 7 years, 2 months ago 
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
Re: [Qemu-devel] [PATCH] tci: Remove invalid assertions
Posted by Peter Maydell 7 years, 2 months ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.