[RFC 0/6] Use __sync_* instead of g_atomic_*

[RFC 0/6] Use __sync_* instead of g_atomic_*

[Lucas De Marchi]

These patches fix build with gcc >= 4.8. I'd prefer to use plain ++/-- for
these refcounts since we are not running multiple threads, but I'm preserving
the previous behavior.

Note in some of the commits that the use of atomic operations was wrong, and it
was not noticed only because there aren't multiple threads.

It's an RFC because it's only compile-tested (and 'make check' was executed as
well). Please take a look in the patches and tell me if we want
to continue with atomic operations. Then I can run some tests tomorrow.

Lucas De Marchi (6):
  gitignore: Ignore file generated by Automake 1.13
  gdbus: Use gcc builtin instead of g_atomic
  attrib: Use gcc builtin instead of g_atomic
  gobex: Use gcc builtin instead of g_atomic
  obexd: Use gcc builtin instead of g_atomic
  shared: Use gcc builtin instead of g_atomic

 .gitignore             |  4 ++++
 attrib/gatt.c          | 12 ++++++------
 attrib/gattrib.c       | 14 ++++++++------
 gdbus/client.c         | 12 ++++++------
 gobex/gobex.c          | 14 ++++++++------
 obexd/client/session.c | 12 ++++++------
 src/shared/hciemu.c    |  4 ++--
 7 files changed, 40 insertions(+), 32 deletions(-)

-- 
1.8.2

[RFC 1/6] gitignore: Ignore file generated by Automake 1.13

[Lucas De Marchi]

From: Lucas De Marchi <lucas.de.marchi@gmail.com>

These files are generated by Automake >= 1.13 when running the unit
tests with make check.
---
 .gitignore | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/.gitignore b/.gitignore
index e306f68..c6e0ae2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -25,6 +25,8 @@ ltmain.sh
 missing
 stamp-h1
 autom4te.cache
+test-driver
+test-suite.log
 
 lib/bluez.pc
 lib/bluetooth
@@ -89,3 +91,5 @@ unit/test-gobex-apparam
 unit/test-gobex-header
 unit/test-gobex-packet
 unit/test-gobex-transfer
+unit/test-*.log
+unit/test-*.trs
-- 
1.8.2

[RFC 2/6] gdbus: Use gcc builtin instead of g_atomic

[Lucas De Marchi]

g_atomic_* end up using G_STATIC_ASSERT, causing gcc 4.8 to yell due to
-Wunused-local-typedefs.

gdbus/client.c: In function ‘g_dbus_client_ref’:
/usr/include/glib-2.0/glib/gmacros.h:162:53: error: typedef ‘_GStaticAssertCompileTimeAssertion_2’ locally defined but not used [-Werror=unused-local-typedefs]
 #define G_STATIC_ASSERT(expr) typedef char G_PASTE (_GStaticAssertCompileTimeAssertion_, __COUNTER__)[(expr) ? 1 : -1]
---
 gdbus/client.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdbus/client.c b/gdbus/client.c
index 2a7d2e1..c2d2346 100644
--- a/gdbus/client.c
+++ b/gdbus/client.c
@@ -33,7 +33,7 @@
 #define METHOD_CALL_TIMEOUT (300 * 1000)
 
 struct GDBusClient {
-	gint ref_count;
+	int ref_count;
 	DBusConnection *dbus_conn;
 	char *service_name;
 	char *unique_name;
@@ -54,7 +54,7 @@ struct GDBusClient {
 };
 
 struct GDBusProxy {
-	gint ref_count;
+	int ref_count;
 	GDBusClient *client;
 	char *obj_path;
 	char *interface;
@@ -444,7 +444,7 @@ GDBusProxy *g_dbus_proxy_ref(GDBusProxy *proxy)
 	if (proxy == NULL)
 		return NULL;
 
-	g_atomic_int_inc(&proxy->ref_count);
+	__sync_fetch_and_add(&proxy->ref_count, 1);
 
 	return proxy;
 }
@@ -454,7 +454,7 @@ void g_dbus_proxy_unref(GDBusProxy *proxy)
 	if (proxy == NULL)
 		return;
 
-	if (g_atomic_int_dec_and_test(&proxy->ref_count) == FALSE)
+	if (__sync_sub_and_fetch(&proxy->ref_count, 1) > 0)
 		return;
 
 	g_hash_table_destroy(proxy->prop_list);
@@ -1265,7 +1265,7 @@ GDBusClient *g_dbus_client_ref(GDBusClient *client)
 	if (client == NULL)
 		return NULL;
 
-	g_atomic_int_inc(&client->ref_count);
+	__sync_fetch_and_add(&client->ref_count, 1);
 
 	return client;
 }
@@ -1277,7 +1277,7 @@ void g_dbus_client_unref(GDBusClient *client)
 	if (client == NULL)
 		return;
 
-	if (g_atomic_int_dec_and_test(&client->ref_count) == FALSE)
+	if (__sync_sub_and_fetch(&client->ref_count, 1) > 0)
 		return;
 
 	if (client->pending_call != NULL) {
-- 
1.8.2

[RFC 3/6] attrib: Use gcc builtin instead of g_atomic

[Lucas De Marchi]

g_atomic_* end up using G_STATIC_ASSERT, causing gcc 4.8 to yell due to
-Wunused-local-typedefs.

/usr/include/glib-2.0/glib/gmacros.h:162:53: error: typedef ‘_GStaticAssertCompileTimeAssertion_2’ locally defined but not used [-Werror=unused-local-typedefs]
 #define G_STATIC_ASSERT(expr) typedef char G_PASTE (_GStaticAssertCompileTimeAssertion_, __COUNTER__)[(expr) ? 1 : -1]

Most of the uses of atomic operations were wrong. They were fixed as
well. If we are using atomic operations, reading the variable again
later for logging is not an option, we should use the return of the
atomic function used to fetch the variable.
---
 attrib/gatt.c    | 12 ++++++------
 attrib/gattrib.c | 14 ++++++++------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/attrib/gatt.c b/attrib/gatt.c
index 44d3eb6..fa61c9e 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -79,14 +79,14 @@ static void discover_primary_free(struct discover_primary *dp)
 
 static struct included_discovery *isd_ref(struct included_discovery *isd)
 {
-	g_atomic_int_inc(&isd->refs);
+	__sync_fetch_and_add(&isd->refs, 1);
 
 	return isd;
 }
 
 static void isd_unref(struct included_discovery *isd)
 {
-	if (g_atomic_int_dec_and_test(&isd->refs) == FALSE)
+	if (__sync_sub_and_fetch(&isd->refs, 1) > 0)
 		return;
 
 	if (isd->err)
@@ -581,7 +581,7 @@ static void read_long_destroy(gpointer user_data)
 {
 	struct read_long_data *long_read = user_data;
 
-	if (g_atomic_int_dec_and_test(&long_read->ref) == FALSE)
+	if (__sync_sub_and_fetch(&long_read->ref, 1) > 0)
 		return;
 
 	if (long_read->buffer != NULL)
@@ -626,7 +626,7 @@ static void read_blob_helper(guint8 status, const guint8 *rpdu, guint16 rlen,
 				read_blob_helper, long_read, read_long_destroy);
 
 	if (id != 0) {
-		g_atomic_int_inc(&long_read->ref);
+		__sync_fetch_and_add(&long_read->ref, 1);
 		return;
 	}
 
@@ -662,7 +662,7 @@ static void read_char_helper(guint8 status, const guint8 *rpdu,
 			read_blob_helper, long_read, read_long_destroy);
 
 	if (id != 0) {
-		g_atomic_int_inc(&long_read->ref);
+		__sync_fetch_and_add(&long_read->ref, 1);
 		return;
 	}
 
@@ -698,7 +698,7 @@ guint gatt_read_char(GAttrib *attrib, uint16_t handle, GAttribResultFunc func,
 	if (id == 0)
 		g_free(long_read);
 	else {
-		g_atomic_int_inc(&long_read->ref);
+		__sync_fetch_and_add(&long_read->ref, 1);
 		long_read->id = id;
 	}
 
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 37581a3..b323617 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -147,12 +147,14 @@ static gboolean is_response(guint8 opcode)
 
 GAttrib *g_attrib_ref(GAttrib *attrib)
 {
+	int refs;
+
 	if (!attrib)
 		return NULL;
 
-	g_atomic_int_inc(&attrib->refs);
+	refs = __sync_fetch_and_add(&attrib->refs, 1);
 
-	DBG("%p: ref=%d", attrib, attrib->refs);
+	DBG("%p: ref=%d", attrib, refs + 1);
 
 	return attrib;
 }
@@ -219,16 +221,16 @@ static void attrib_destroy(GAttrib *attrib)
 
 void g_attrib_unref(GAttrib *attrib)
 {
-	gboolean ret;
+	int refs;
 
 	if (!attrib)
 		return;
 
-	ret = g_atomic_int_dec_and_test(&attrib->refs);
+	refs = __sync_sub_and_fetch(&attrib->refs, 1);
 
-	DBG("%p: ref=%d", attrib, attrib->refs);
+	DBG("%p: ref=%d", attrib, refs);
 
-	if (ret == FALSE)
+	if (refs > 0)
 		return;
 
 	attrib_destroy(attrib);
-- 
1.8.2

Re: [RFC 3/6] attrib: Use gcc builtin instead of g_atomic

[Lucas De Marchi]

On Tue, Apr 2, 2013 at 7:54 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> g_atomic_* end up using G_STATIC_ASSERT, causing gcc 4.8 to yell due to
> -Wunused-local-typedefs.
>
> /usr/include/glib-2.0/glib/gmacros.h:162:53: error: typedef ‘_GStaticAssertCompileTimeAssertion_2’ locally defined but not used [-Werror=unused-local-typedefs]
>  #define G_STATIC_ASSERT(expr) typedef char G_PASTE (_GStaticAssertCompileTimeAssertion_, __COUNTER__)[(expr) ? 1 : -1]
>
> Most of the uses of atomic operations were wrong. They were fixed as
> well. If we are using atomic operations, reading the variable again
> later for logging is not an option, we should use the return of the
> atomic function used to fetch the variable.
> ---
>  attrib/gatt.c    | 12 ++++++------
>  attrib/gattrib.c | 14 ++++++++------
>  2 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/attrib/gatt.c b/attrib/gatt.c
> index 44d3eb6..fa61c9e 100644
> --- a/attrib/gatt.c
> +++ b/attrib/gatt.c
> @@ -79,14 +79,14 @@ static void discover_primary_free(struct discover_primary *dp)
>
>  static struct included_discovery *isd_ref(struct included_discovery *isd)
>  {
> -       g_atomic_int_inc(&isd->refs);
> +       __sync_fetch_and_add(&isd->refs, 1);
>
>         return isd;
>  }
>
>  static void isd_unref(struct included_discovery *isd)
>  {
> -       if (g_atomic_int_dec_and_test(&isd->refs) == FALSE)
> +       if (__sync_sub_and_fetch(&isd->refs, 1) > 0)
>                 return;
>
>         if (isd->err)
> @@ -581,7 +581,7 @@ static void read_long_destroy(gpointer user_data)
>  {
>         struct read_long_data *long_read = user_data;
>
> -       if (g_atomic_int_dec_and_test(&long_read->ref) == FALSE)
> +       if (__sync_sub_and_fetch(&long_read->ref, 1) > 0)
>                 return;
>
>         if (long_read->buffer != NULL)
> @@ -626,7 +626,7 @@ static void read_blob_helper(guint8 status, const guint8 *rpdu, guint16 rlen,
>                                 read_blob_helper, long_read, read_long_destroy);
>
>         if (id != 0) {
> -               g_atomic_int_inc(&long_read->ref);
> +               __sync_fetch_and_add(&long_read->ref, 1);
>                 return;
>         }
>
> @@ -662,7 +662,7 @@ static void read_char_helper(guint8 status, const guint8 *rpdu,
>                         read_blob_helper, long_read, read_long_destroy);
>
>         if (id != 0) {
> -               g_atomic_int_inc(&long_read->ref);
> +               __sync_fetch_and_add(&long_read->ref, 1);
>                 return;
>         }
>
> @@ -698,7 +698,7 @@ guint gatt_read_char(GAttrib *attrib, uint16_t handle, GAttribResultFunc func,
>         if (id == 0)
>                 g_free(long_read);
>         else {
> -               g_atomic_int_inc(&long_read->ref);
> +               __sync_fetch_and_add(&long_read->ref, 1);
>                 long_read->id = id;
>         }
>
> diff --git a/attrib/gattrib.c b/attrib/gattrib.c
> index 37581a3..b323617 100644
> --- a/attrib/gattrib.c
> +++ b/attrib/gattrib.c
> @@ -147,12 +147,14 @@ static gboolean is_response(guint8 opcode)
>
>  GAttrib *g_attrib_ref(GAttrib *attrib)
>  {
> +       int refs;
> +
>         if (!attrib)
>                 return NULL;
>
> -       g_atomic_int_inc(&attrib->refs);
> +       refs = __sync_fetch_and_add(&attrib->refs, 1);
>
> -       DBG("%p: ref=%d", attrib, attrib->refs);
> +       DBG("%p: ref=%d", attrib, refs + 1);

reviewing my own patch... I should have used __sync_add_and_fetch()
above so I don't need to +1 here.

Lucas De Marchi

[RFC 4/6] gobex: Use gcc builtin instead of g_atomic

[Lucas De Marchi]

g_atomic_* end up using G_STATIC_ASSERT, causing gcc 4.8 to yell due to
-Wunused-local-typedefs.

/usr/include/glib-2.0/glib/gmacros.h:162:53: error: typedef ‘_GStaticAssertCompileTimeAssertion_2’ locally defined but not used [-Werror=unused-local-typedefs]
 #define G_STATIC_ASSERT(expr) typedef char G_PASTE (_GStaticAssertCompileTimeAssertion_, __COUNTER__)[(expr) ? 1 : -1]

Most of the uses of atomic operations were wrong. They were fixed as
well. If we are using atomic operations, reading the variable again
later for logging is not an option, we should use the return of the
atomic function used to fetch the variable.
---
 gobex/gobex.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/gobex/gobex.c b/gobex/gobex.c
index a9bcee9..790aec5 100644
--- a/gobex/gobex.c
+++ b/gobex/gobex.c
@@ -1315,25 +1315,27 @@ GObex *g_obex_new(GIOChannel *io, GObexTransportType transport_type,
 
 GObex *g_obex_ref(GObex *obex)
 {
+	int refs;
+
 	if (obex == NULL)
 		return NULL;
 
-	g_atomic_int_inc(&obex->ref_count);
+	refs = __sync_fetch_and_add(&obex->ref_count, 1);
 
-	g_obex_debug(G_OBEX_DEBUG_COMMAND, "ref %u", obex->ref_count);
+	g_obex_debug(G_OBEX_DEBUG_COMMAND, "ref %u", refs + 1);
 
 	return obex;
 }
 
 void g_obex_unref(GObex *obex)
 {
-	gboolean last_ref;
+	int refs;
 
-	last_ref = g_atomic_int_dec_and_test(&obex->ref_count);
+	refs = __sync_sub_and_fetch(&obex->ref_count, 1);
 
-	g_obex_debug(G_OBEX_DEBUG_COMMAND, "ref %u", obex->ref_count);
+	g_obex_debug(G_OBEX_DEBUG_COMMAND, "ref %u", refs);
 
-	if (!last_ref)
+	if (refs > 0)
 		return;
 
 	g_slist_free_full(obex->req_handlers, g_free);
-- 
1.8.2

[RFC 5/6] obexd: Use gcc builtin instead of g_atomic

[Lucas De Marchi]

g_atomic_* end up using G_STATIC_ASSERT, causing gcc 4.8 to yell due to
-Wunused-local-typedefs.

/usr/include/glib-2.0/glib/gmacros.h:162:53: error: typedef ‘_GStaticAssertCompileTimeAssertion_2’ locally defined but not used [-Werror=unused-local-typedefs]
 #define G_STATIC_ASSERT(expr) typedef char G_PASTE (_GStaticAssertCompileTimeAssertion_, __COUNTER__)[(expr) ? 1 : -1]

Most of the uses of atomic operations were wrong. They were fixed as
well. If we are using atomic operations, reading the variable again
later for logging is not an option, we should use the return of the
atomic function used to fetch the variable.
---
 obexd/client/session.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/obexd/client/session.c b/obexd/client/session.c
index f677bcb..c14efb4 100644
--- a/obexd/client/session.c
+++ b/obexd/client/session.c
@@ -115,9 +115,9 @@ static GQuark obex_io_error_quark(void)
 
 struct obc_session *obc_session_ref(struct obc_session *session)
 {
-	g_atomic_int_inc(&session->refcount);
+	int refs = __sync_fetch_and_add(&session->refcount, 1);
 
-	DBG("%p: ref=%d", session, session->refcount);
+	DBG("%p: ref=%d", session, refs + 1);
 
 	return session;
 }
@@ -210,13 +210,13 @@ static void session_free(struct obc_session *session)
 
 void obc_session_unref(struct obc_session *session)
 {
-	gboolean ret;
+	int refs;
 
-	ret = g_atomic_int_dec_and_test(&session->refcount);
+	refs = __sync_sub_and_fetch(&session->refcount, 1);
 
-	DBG("%p: ref=%d", session, session->refcount);
+	DBG("%p: ref=%d", session, refs);
 
-	if (ret == FALSE)
+	if (refs > 0)
 		return;
 
 	session_free(session);
-- 
1.8.2

[RFC 6/6] obexd: Use gcc builtin instead of g_atomic

[Lucas De Marchi]

g_atomic_* end up using G_STATIC_ASSERT, causing gcc 4.8 to yell due to
-Wunused-local-typedefs.

/usr/include/glib-2.0/glib/gmacros.h:162:53: error: typedef ‘_GStaticAssertCompileTimeAssertion_2’ locally defined but not used [-Werror=unused-local-typedefs]
 #define G_STATIC_ASSERT(expr) typedef char G_PASTE (_GStaticAssertCompileTimeAssertion_, __COUNTER__)[(expr) ? 1 : -1]
---
 src/shared/hciemu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/shared/hciemu.c b/src/shared/hciemu.c
index 97ea84e..60a4f47 100644
--- a/src/shared/hciemu.c
+++ b/src/shared/hciemu.c
@@ -308,7 +308,7 @@ struct hciemu *hciemu_ref(struct hciemu *hciemu)
 	if (!hciemu)
 		return NULL;
 
-	g_atomic_int_inc(&hciemu->ref_count);
+	__sync_fetch_and_add(&hciemu->ref_count, 1);
 
 	return hciemu;
 }
@@ -318,7 +318,7 @@ void hciemu_unref(struct hciemu *hciemu)
 	if (!hciemu)
 		return;
 
-	if (g_atomic_int_dec_and_test(&hciemu->ref_count) == FALSE)
+	if (__sync_sub_and_fetch(&hciemu->ref_count, 1) > 0)
 		return;
 
 	g_list_foreach(hciemu->post_command_hooks, destroy_command_hook, NULL);
-- 
1.8.2

Re: [RFC 6/6] obexd: Use gcc builtin instead of g_atomic

[Lucas De Marchi]

Hi,

On Tue, Apr 2, 2013 at 7:54 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> g_atomic_* end up using G_STATIC_ASSERT, causing gcc 4.8 to yell due to
> -Wunused-local-typedefs.
>
> /usr/include/glib-2.0/glib/gmacros.h:162:53: error: typedef ‘_GStaticAssertCompileTimeAssertion_2’ locally defined but not used [-Werror=unused-local-typedefs]
>  #define G_STATIC_ASSERT(expr) typedef char G_PASTE (_GStaticAssertCompileTimeAssertion_, __COUNTER__)[(expr) ? 1 : -1]
> ---

This patch was laying in my /tmp when I sent the patches. Please
ignore this one and use the other with subject "[RFC 5/6] obexd: Use
gcc builtin instead of g_atomic"

Sorry for the noise.

Lucas De Marchi

[RFC 6/6] shared: Use gcc builtin instead of g_atomic

[Lucas De Marchi]

g_atomic_* end up using G_STATIC_ASSERT, causing gcc 4.8 to yell due to
-Wunused-local-typedefs.

/usr/include/glib-2.0/glib/gmacros.h:162:53: error: typedef ‘_GStaticAssertCompileTimeAssertion_2’ locally defined but not used [-Werror=unused-local-typedefs]
 #define G_STATIC_ASSERT(expr) typedef char G_PASTE (_GStaticAssertCompileTimeAssertion_, __COUNTER__)[(expr) ? 1 : -1]
---
 src/shared/hciemu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/shared/hciemu.c b/src/shared/hciemu.c
index 97ea84e..60a4f47 100644
--- a/src/shared/hciemu.c
+++ b/src/shared/hciemu.c
@@ -308,7 +308,7 @@ struct hciemu *hciemu_ref(struct hciemu *hciemu)
 	if (!hciemu)
 		return NULL;
 
-	g_atomic_int_inc(&hciemu->ref_count);
+	__sync_fetch_and_add(&hciemu->ref_count, 1);
 
 	return hciemu;
 }
@@ -318,7 +318,7 @@ void hciemu_unref(struct hciemu *hciemu)
 	if (!hciemu)
 		return;
 
-	if (g_atomic_int_dec_and_test(&hciemu->ref_count) == FALSE)
+	if (__sync_sub_and_fetch(&hciemu->ref_count, 1) > 0)
 		return;
 
 	g_list_foreach(hciemu->post_command_hooks, destroy_command_hook, NULL);
-- 
1.8.2

Re: [RFC 0/6] Use __sync_* instead of g_atomic_*

[Vinicius Costa Gomes]

Hi,

On 19:54 Tue 02 Apr, Lucas De Marchi wrote:
> These patches fix build with gcc >= 4.8. I'd prefer to use plain ++/-- for
> these refcounts since we are not running multiple threads, but I'm preserving
> the previous behavior.
> 
> Note in some of the commits that the use of atomic operations was wrong, and it
> was not noticed only because there aren't multiple threads.
> 
> It's an RFC because it's only compile-tested (and 'make check' was executed as
> well). Please take a look in the patches and tell me if we want
> to continue with atomic operations. Then I can run some tests tomorrow.

Just to (publicly) clarify a doubt that I had, in case it matters to anyone
else, clang also has these built-ins.


Cheers,
-- 
Vinicius