On 21/11/2024 3:19 pm, Frediano Ziglio wrote:
> On Thu, Nov 21, 2024 at 2:50 PM Andrew Cooper <[email protected]>
> wrote:
>> for_each_set_bit()'s use of a double for loop had an accidental bug with a
>> break in the inner loop leading to an infinite outer loop.
>>
>> Adjust for_each_set_bit() to avoid this behaviour, and add extend
>> test_for_each_set_bit() with a test case for this.
>>
>> Fixes: ed26376f20bf ("xen/bitops: Introduce for_each_set_bit()")
>> Signed-off-by: Andrew Cooper <[email protected]>
>> ---
>> CC: Jan Beulich <[email protected]>
>> CC: Roger Pau Monné <[email protected]>
>> CC: Frediano Ziglio <[email protected]>
>> CC: Stefano Stabellini <[email protected]>
>> CC: Julien Grall <[email protected]>
>>
>> Both GCC and Clang seem happy with this, even at -O1:
>>
>> https://godbolt.org/z/o6ohjrzsY
>> ---
>> xen/common/bitops.c | 16 ++++++++++++++++
>> xen/include/xen/bitops.h | 2 +-
>> 2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/common/bitops.c b/xen/common/bitops.c
>> index 91ae961440af..0edd62d25c28 100644
>> --- a/xen/common/bitops.c
>> +++ b/xen/common/bitops.c
>> @@ -110,6 +110,22 @@ static void __init test_for_each_set_bit(void)
>>
>> if ( ull != ull_res )
>> panic("for_each_set_bit(uint64) expected %#"PRIx64", got
>> %#"PRIx64"\n", ull, ull_res);
>> +
>> + /* Check that we break from the middle of the loop */
>> + ui = HIDE(0x80001008U);
>> + ui_res = 0;
>> + for_each_set_bit ( i, ui )
>> + {
>> + static __initdata unsigned int count;
>> +
>> + if ( count++ > 1 )
>> + break;
>> +
>> + ui_res |= 1U << i;
>> + }
>> +
>> + if ( ui_res != 0x1008 )
>> + panic("for_each_set_bit(break) expected 0x1008, got %#x\n", ui_res);
>> }
>>
>> static void __init test_multiple_bits_set(void)
>> diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
>> index 79615fb89d04..448b2d3e0937 100644
>> --- a/xen/include/xen/bitops.h
>> +++ b/xen/include/xen/bitops.h
>> @@ -299,7 +299,7 @@ static always_inline attr_const unsigned int
>> fls64(uint64_t x)
>> * A copy of @val is taken internally.
>> */
>> #define for_each_set_bit(iter, val) \
>> - for ( typeof(val) __v = (val); __v; ) \
>> + for ( typeof(val) __v = (val); __v; __v = 0 ) \
>> for ( unsigned int (iter); \
>> __v && ((iter) = ffs_g(__v) - 1, true); \
>> __v &= __v - 1 )
>>
>> base-commit: e0058760a0c7935ad0690d8b9babb9050eceedf0
> Not a fun of static variables but it's just in the test,
Oh, I guess I can do it with just a local variable. Incremental diff is:
@@ -86,7 +86,7 @@ static void __init test_fls(void)
static void __init test_for_each_set_bit(void)
{
- unsigned int ui, ui_res = 0;
+ unsigned int ui, ui_res = 0, tmp;
unsigned long ul, ul_res = 0;
uint64_t ull, ull_res = 0;
@@ -113,12 +113,11 @@ static void __init test_for_each_set_bit(void)
/* Check that we break from the middle of the loop */
ui = HIDE(0x80001008U);
+ tmp = 0;
ui_res = 0;
for_each_set_bit ( i, ui )
{
- static __initdata unsigned int count;
-
- if ( count++ > 1 )
+ if ( tmp++ > 1 )
break;
ui_res |= 1U << i;
> Reviewed-by: Frediano Ziglio <[email protected]>
Thanks. This turns out to be a whole lot easier than I was fearing.
~Andrew
