[PATCH] tcg/tci: fix logic error when registering helpers via FFI

[PATCH] tcg/tci: fix logic error when registering helpers via FFI

[Icenowy Zheng]

When registering helpers via FFI for TCI, the inner loop that iterates
parameters of the helper reuses (and thus pollutes) the same variable
used by the outer loop that iterates all helpers, thus made some helpers
unregistered.

Fix this logic error by using a dedicated temporary variable for the
inner loop.

Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
---
 tcg/tcg.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index 612a12f58f..adfaf61a32 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -619,6 +619,7 @@ static void tcg_context_init(unsigned max_cpus)
         gpointer hash = (gpointer)(uintptr_t)typemask;
         ffi_status status;
         int nargs;
+        int j;
 
         if (g_hash_table_lookup(ffi_table, hash)) {
             continue;
@@ -634,9 +635,9 @@ static void tcg_context_init(unsigned max_cpus)
 
         if (nargs != 0) {
             ca->cif.arg_types = ca->args;
-            for (i = 0; i < nargs; ++i) {
-                int typecode = extract32(typemask, (i + 1) * 3, 3);
-                ca->args[i] = typecode_to_ffi[typecode];
+            for (j = 0; j < nargs; ++j) {
+                int typecode = extract32(typemask, (j + 1) * 3, 3);
+                ca->args[j] = typecode_to_ffi[typecode];
             }
         }
 
-- 
2.37.1

Re: [PATCH] tcg/tci: fix logic error when registering helpers via FFI

[Alex Bennée]

Icenowy Zheng <uwu@icenowy.me> writes:

> When registering helpers via FFI for TCI, the inner loop that iterates
> parameters of the helper reuses (and thus pollutes) the same variable
> used by the outer loop that iterates all helpers, thus made some helpers
> unregistered.
>
> Fix this logic error by using a dedicated temporary variable for the
> inner loop.
>
> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> ---
>  tcg/tcg.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index 612a12f58f..adfaf61a32 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -619,6 +619,7 @@ static void tcg_context_init(unsigned max_cpus)
>          gpointer hash = (gpointer)(uintptr_t)typemask;
>          ffi_status status;
>          int nargs;
> +        int j;

You could tuck this variable definition...


>  
>          if (g_hash_table_lookup(ffi_table, hash)) {
>              continue;
> @@ -634,9 +635,9 @@ static void tcg_context_init(unsigned max_cpus)
>  
>          if (nargs != 0) {

here to keep things more local.

>              ca->cif.arg_types = ca->args;
> -            for (i = 0; i < nargs; ++i) {
> -                int typecode = extract32(typemask, (i + 1) * 3, 3);
> -                ca->args[i] = typecode_to_ffi[typecode];
> +            for (j = 0; j < nargs; ++j) {
> +                int typecode = extract32(typemask, (j + 1) * 3, 3);
> +                ca->args[j] = typecode_to_ffi[typecode];
>              }
>          }

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH] tcg/tci: fix logic error when registering helpers via FFI

[Icenowy Zheng]

在 2022-10-28星期五的 10:13 +0100，Alex Bennée写道：
> 
> Icenowy Zheng <uwu@icenowy.me> writes:
> 
> > When registering helpers via FFI for TCI, the inner loop that
> > iterates
> > parameters of the helper reuses (and thus pollutes) the same
> > variable
> > used by the outer loop that iterates all helpers, thus made some
> > helpers
> > unregistered.
> > 
> > Fix this logic error by using a dedicated temporary variable for
> > the
> > inner loop.
> > 
> > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> > ---
> >  tcg/tcg.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tcg/tcg.c b/tcg/tcg.c
> > index 612a12f58f..adfaf61a32 100644
> > --- a/tcg/tcg.c
> > +++ b/tcg/tcg.c
> > @@ -619,6 +619,7 @@ static void tcg_context_init(unsigned max_cpus)
> >          gpointer hash = (gpointer)(uintptr_t)typemask;
> >          ffi_status status;
> >          int nargs;
> > +        int j;
> 
> You could tuck this variable definition...
> 
> 
> >  
> >          if (g_hash_table_lookup(ffi_table, hash)) {
> >              continue;
> > @@ -634,9 +635,9 @@ static void tcg_context_init(unsigned max_cpus)
> >  
> >          if (nargs != 0) {
> 
> here to keep things more local.

Oops I was trying to find the nearest existing variable definition when
writing this, and forget this is a block's start.

Thanks for this tip
Icenowy Zheng

> 
> >              ca->cif.arg_types = ca->args;
> > -            for (i = 0; i < nargs; ++i) {
> > -                int typecode = extract32(typemask, (i + 1) * 3,
> > 3);
> > -                ca->args[i] = typecode_to_ffi[typecode];
> > +            for (j = 0; j < nargs; ++j) {
> > +                int typecode = extract32(typemask, (j + 1) * 3,
> > 3);
> > +                ca->args[j] = typecode_to_ffi[typecode];
> >              }
> >          }
> 
> Otherwise:
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> 

Re: [PATCH] tcg/tci: fix logic error when registering helpers via FFI

[Philippe Mathieu-Daudé]

On 28/10/22 09:21, Icenowy Zheng wrote:
> When registering helpers via FFI for TCI, the inner loop that iterates
> parameters of the helper reuses (and thus pollutes) the same variable
> used by the outer loop that iterates all helpers, thus made some helpers
> unregistered.

Oops, I didn't notice while testing, good catch.

> Fix this logic error by using a dedicated temporary variable for the
> inner loop.

Fixes: 22f15579fa ("tcg: Build ffi data structures for helpers")
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> ---
>   tcg/tcg.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)

Re: [PATCH] tcg/tci: fix logic error when registering helpers via FFI

[Richard Henderson]

On 10/28/22 18:21, Icenowy Zheng wrote:
> When registering helpers via FFI for TCI, the inner loop that iterates
> parameters of the helper reuses (and thus pollutes) the same variable
> used by the outer loop that iterates all helpers, thus made some helpers
> unregistered.
> 
> Fix this logic error by using a dedicated temporary variable for the
> inner loop.
> 
> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> ---
>   tcg/tcg.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index 612a12f58f..adfaf61a32 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -619,6 +619,7 @@ static void tcg_context_init(unsigned max_cpus)
>           gpointer hash = (gpointer)(uintptr_t)typemask;
>           ffi_status status;
>           int nargs;
> +        int j;
>   
>           if (g_hash_table_lookup(ffi_table, hash)) {
>               continue;
> @@ -634,9 +635,9 @@ static void tcg_context_init(unsigned max_cpus)
>   
>           if (nargs != 0) {
>               ca->cif.arg_types = ca->args;
> -            for (i = 0; i < nargs; ++i) {
> -                int typecode = extract32(typemask, (i + 1) * 3, 3);
> -                ca->args[i] = typecode_to_ffi[typecode];
> +            for (j = 0; j < nargs; ++j) {
> +                int typecode = extract32(typemask, (j + 1) * 3, 3);
> +                ca->args[j] = typecode_to_ffi[typecode];

Oh my.  I'm surprised any test cases at all worked.
Queued to tcg-next, with the declaration of j moved to the loop itself:

	for (int j = 0; j < nargs; ++j)


r~

Re: [PATCH] tcg/tci: fix logic error when registering helpers via FFI

[Icenowy Zheng]

在 2022-10-29星期六的 06:28 +1100，Richard Henderson写道：
> On 10/28/22 18:21, Icenowy Zheng wrote:
> > When registering helpers via FFI for TCI, the inner loop that
> > iterates
> > parameters of the helper reuses (and thus pollutes) the same
> > variable
> > used by the outer loop that iterates all helpers, thus made some
> > helpers
> > unregistered.
> > 
> > Fix this logic error by using a dedicated temporary variable for
> > the
> > inner loop.
> > 
> > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> > ---
> >   tcg/tcg.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tcg/tcg.c b/tcg/tcg.c
> > index 612a12f58f..adfaf61a32 100644
> > --- a/tcg/tcg.c
> > +++ b/tcg/tcg.c
> > @@ -619,6 +619,7 @@ static void tcg_context_init(unsigned max_cpus)
> >           gpointer hash = (gpointer)(uintptr_t)typemask;
> >           ffi_status status;
> >           int nargs;
> > +        int j;
> >   
> >           if (g_hash_table_lookup(ffi_table, hash)) {
> >               continue;
> > @@ -634,9 +635,9 @@ static void tcg_context_init(unsigned max_cpus)
> >   
> >           if (nargs != 0) {
> >               ca->cif.arg_types = ca->args;
> > -            for (i = 0; i < nargs; ++i) {
> > -                int typecode = extract32(typemask, (i + 1) * 3,
> > 3);
> > -                ca->args[i] = typecode_to_ffi[typecode];
> > +            for (j = 0; j < nargs; ++j) {
> > +                int typecode = extract32(typemask, (j + 1) * 3,
> > 3);
> > +                ca->args[j] = typecode_to_ffi[typecode];
> 
> Oh my.  I'm surprised any test cases at all worked.
> Queued to tcg-next, with the declaration of j moved to the loop
> itself:
> 
>         for (int j = 0; j < nargs; ++j)

Ah I think this is a C99 feature. Is our C standard baseline high
enough to use it?

> 
> 
> r~
> 

Re: [PATCH] tcg/tci: fix logic error when registering helpers via FFI

[Peter Maydell]

On Sat, 29 Oct 2022 at 01:45, Icenowy Zheng <uwu@icenowy.me> wrote:
>
> 在 2022-10-29星期六的 06:28 +1100，Richard Henderson写道：
> > Oh my.  I'm surprised any test cases at all worked.
> > Queued to tcg-next, with the declaration of j moved to the loop
> > itself:
> >
> >         for (int j = 0; j < nargs; ++j)
>
> Ah I think this is a C99 feature. Is our C standard baseline high
> enough to use it?

Mmm, my instinctive reaction was that our style probably
doesn't permit that. But if you do
  git grep 'for (int'
we already use it quite a bit...

-- PMM

Re: [PATCH] tcg/tci: fix logic error when registering helpers via FFI

[Richard Henderson]

On 10/29/22 11:44, Icenowy Zheng wrote:
> Ah I think this is a C99 feature. Is our C standard baseline high
> enough to use it?

Yes, we use C2011.  See the second line of meson.build:

         default_options: ... 'c_std=gnu11'


r~