| Message ID | 20221028072145.1593205-1-uwu@icenowy.me |
|---|---|
| State | New |
| Headers | show |
| Series | tcg/tci: fix logic error when registering helpers via FFI | expand |
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>
在 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> >
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(-)
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~
在 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~ >
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
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~
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]; } }
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(-)