Re: [RFC PATCH 2/4] xen/arm64: bitops: justify uninitialized variable inside a macro

[Julien Grall <julien@xxxxxxx>]

Hi,

On 14/07/2023 12:49, Nicola Vetrini wrote:
> The macro 'testop' expands to a function that declares the local
> variable 'oldbit', which is written before being set, but is such a
> way that is not amenable to automatic checking.
>
> Therefore, a deviation comment, is introduced to document this situation.
>
> A similar reasoning applies to macro 'guest_testop'.
Would you be able to check if the code below (only compile tested so 
far) would silence Eclair?
diff --git a/xen/arch/arm/arm64/lib/bitops.c 
b/xen/arch/arm/arm64/lib/bitops.c
index 20e3f3d6ceaf..f7a10b790f2a 100644
--- a/xen/arch/arm/arm64/lib/bitops.c
+++ b/xen/arch/arm/arm64/lib/bitops.c
@@ -64,13 +64,15 @@ bool name##_timeout(int nr, volatile void *p, 
unsigned int max_try)         \
 }

 #define testop(name, instr) 
     \
-static always_inline bool int_##name(int nr, volatile void *p, int 
*oldbit, \
-                                     bool timeout, unsigned int 
max_try)    \
+static always_inline int int_##name(int nr, volatile void *p, 
    \
+                                    bool allow_timeout, bool *timeout, 
    \
+                                    unsigned int max_try) 
    \
 { 
     \
     volatile uint32_t *ptr = (uint32_t *)p + BITOP_WORD((unsigned 
int)nr);  \
     unsigned int bit = (unsigned int)nr % BITOP_BITS_PER_WORD; 
     \
     const uint32_t mask = BITOP_MASK(bit); 
     \
     unsigned long res, tmp; 
     \
+    int oldbit; 
    \
     \
     do 
     \
     { 
     \
@@ -79,27 +81,30 @@ static always_inline bool int_##name(int nr, 
volatile void *p, int *oldbit, \
         "   lsr     %w1, %w3, %w5 // Save old value of bit\n" 
     \
         "   " __stringify(instr) "  %w3, %w3, %w4 // Toggle bit\n" 
     \
         "   stlxr   %w0, %w3, %2\n" 
     \
-        : "=&r" (res), "=&r" (*oldbit), "+Q" (*ptr), "=&r" (tmp) 
    \
+        : "=&r" (res), "=&r" (oldbit), "+Q" (*ptr), "=&r" (tmp) 
    \
         : "r" (mask), "r" (bit) 
     \
         : "memory"); 
     \
     \
         if ( !res ) 
     \
             break; 
     \
-    } while ( !timeout || ((--max_try) > 0) ); 
    \
+    } while ( !allow_timeout || ((--max_try) > 0) ); 
    \
     \
     dmb(ish); 
     \
     \
-    *oldbit &= 1; 
    \
+    ASSERT(!allow_timeout || timeout); 
    \
+    if ( timeout ) 
    \
+        *timeout = !res; 
    \
+    else if ( !res ) 
    \
+        ASSERT_UNREACHABLE(); 
    \
     \
-    return !res; 
    \
+    return (oldbit & 1); 
    \
 } 
     \
     \
 int name(int nr, volatile void *p) 
     \
 { 
     \
     int oldbit; 
     \
     \
-    if ( !int_##name(nr, p, &oldbit, false, 0) ) 
    \
-        ASSERT_UNREACHABLE(); 
    \
+    oldbit = int_##name(nr, p, false, NULL, 0); 
    \
     \

Cheers,

--
Julien Grall