Re: [Mesa-dev] [PATCH] gallium/util: fix p_atomic_dec_zero macros

2014-06-16 Thread Maarten Lankhorst 

op 13-06-14 07:39, Michel Dänzer schreef:

On 12.06.2014 17:00, Maarten Lankhorst wrote:

I'm pretty sure that p_atomic_dec_zero should return 1 if the count
drops to zero.

Cc: 10.2 10.1 10.0 mesa-sta...@lists.freedesktop.org

I don't think the stable tag is justified: These bugs have been there
for more than four years. Nothing in Gallium can work properly if the
return value of p_atomic_dec_zero() is inverted, so if there was
significant use of the broken variants, we should have heard about it
long ago.

Yeah, they're just the fallbacks I guess.



Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
---
diff --git a/src/gallium/auxiliary/util/u_atomic.h
b/src/gallium/auxiliary/util/u_atomic.h
index 2f2b42b..08cadb4 100644
--- a/src/gallium/auxiliary/util/u_atomic.h
+++ b/src/gallium/auxiliary/util/u_atomic.h
@@ -183,7 +183,7 @@ p_atomic_cmpxchg(int32_t *v, int32_t old, int32_t _new)
  
  #define p_atomic_set(_v, _i) (*(_v) = (_i))

  #define p_atomic_read(_v) (*(_v))
-#define p_atomic_dec_zero(_v) ((boolean) --(*(_v)))
+#define p_atomic_dec_zero(_v) (!(boolean) --(*(_v)))

Will this compile (with the intended result) without another set of parens?

Compiling the following in g++ and gcc seems to confirm it works correctly:

#include stdio.h

int main() {
int i = 1;

printf(dropped to 0: %i\n, (!(unsigned)--(*(i;
return 0;
}

Output:
dropped to 0: 1




@@ -324,7 +324,7 @@ p_atomic_dec_zero(int32_t *v)
  {
 uint32_t n = atomic_dec_32_nv((uint32_t *) v);
  
-   return n != 0;

+   return n == 0;
  }

This looks good, but there are more variants which look similarly broken?

Is this intended to be a question or a statement? The other p_atomic_dec_zero 
definitions look fine afaict.

~Maarten
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] gallium/util: fix p_atomic_dec_zero macros

2014-06-12 Thread Maarten Lankhorst 

I'm pretty sure that p_atomic_dec_zero should return 1 if the count drops to 
zero.

Cc: 10.2 10.1 10.0 mesa-sta...@lists.freedesktop.org
Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
---
diff --git a/src/gallium/auxiliary/util/u_atomic.h 
b/src/gallium/auxiliary/util/u_atomic.h
index 2f2b42b..08cadb4 100644
--- a/src/gallium/auxiliary/util/u_atomic.h
+++ b/src/gallium/auxiliary/util/u_atomic.h
@@ -183,7 +183,7 @@ p_atomic_cmpxchg(int32_t *v, int32_t old, int32_t _new)
 
 #define p_atomic_set(_v, _i) (*(_v) = (_i))

 #define p_atomic_read(_v) (*(_v))
-#define p_atomic_dec_zero(_v) ((boolean) --(*(_v)))
+#define p_atomic_dec_zero(_v) (!(boolean) --(*(_v)))
 #define p_atomic_inc(_v) ((void) (*(_v))++)
 #define p_atomic_dec(_v) ((void) (*(_v))--)
 #define p_atomic_cmpxchg(_v, old, _new) (*(_v) == old ? *(_v) = (_new) : *(_v))
@@ -324,7 +324,7 @@ p_atomic_dec_zero(int32_t *v)
 {
uint32_t n = atomic_dec_32_nv((uint32_t *) v);
 
-   return n != 0;

+   return n == 0;
 }
 
 #define p_atomic_inc(_v) atomic_inc_32((uint32_t *) _v)


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium/util: fix p_atomic_dec_zero macros

2014-06-12 Thread Michel Dänzer 
On 12.06.2014 17:00, Maarten Lankhorst wrote:
 I'm pretty sure that p_atomic_dec_zero should return 1 if the count
 drops to zero.
 
 Cc: 10.2 10.1 10.0 mesa-sta...@lists.freedesktop.org

I don't think the stable tag is justified: These bugs have been there
for more than four years. Nothing in Gallium can work properly if the
return value of p_atomic_dec_zero() is inverted, so if there was
significant use of the broken variants, we should have heard about it
long ago.


 Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
 ---
 diff --git a/src/gallium/auxiliary/util/u_atomic.h
 b/src/gallium/auxiliary/util/u_atomic.h
 index 2f2b42b..08cadb4 100644
 --- a/src/gallium/auxiliary/util/u_atomic.h
 +++ b/src/gallium/auxiliary/util/u_atomic.h
 @@ -183,7 +183,7 @@ p_atomic_cmpxchg(int32_t *v, int32_t old, int32_t _new)
  
  #define p_atomic_set(_v, _i) (*(_v) = (_i))
  #define p_atomic_read(_v) (*(_v))
 -#define p_atomic_dec_zero(_v) ((boolean) --(*(_v)))
 +#define p_atomic_dec_zero(_v) (!(boolean) --(*(_v)))

Will this compile (with the intended result) without another set of parens?


 @@ -324,7 +324,7 @@ p_atomic_dec_zero(int32_t *v)
  {
 uint32_t n = atomic_dec_32_nv((uint32_t *) v);
  
 -   return n != 0;
 +   return n == 0;
  }

This looks good, but there are more variants which look similarly broken?


-- 
Earthling Michel Dänzer|  http://www.amd.com
Libre software enthusiast  |Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev