Bug#739930: Please switch to -latomic atomics for wider arch portability

2014-02-23 Thread Adam Conrad 
Package: librdkafka
Version: 0.8.3-1
Severity: normal
Tags: patch
User: ubuntu-de...@lists.ubuntu.com
Usertags: origin-ubuntu trusty ubuntu-patch



In Ubuntu, the attached patch was applied to achieve the following:

  * link-atomic.patch: Switch from __sync to __atomic to support more arches.
  * Switch back to arch:any instead of the Debian maintainer's shorter list.

So, this patch should be almost upstreamable, depending on who their
intended target is.  If they need to support __sync on non-Linux,
they could wrap __sync and __atomic blocks in ifdef/ifndef magic to
test for linux or perhaps specific GCC versions.

This patch, as it is, however, should be fine for Debian.  Tested in
Ubuntu with the 6 arches we build (amd64, i386, arm64, armhf, powerpc,
and ppc64el), and all looks fine here:

https://launchpad.net/ubuntu/+source/librdkafka/0.8.3-1ubuntu1

... Adam

-- System Information:
Debian Release: jessie/sid
  APT prefers trusty-updates
  APT policy: (500, 'trusty-updates'), (500, 'trusty-security'), (500, 'trusty')
Architecture: amd64 (x86_64)
Foreign Architectures: i386

Kernel: Linux 3.13.0-11-generic (SMP w/4 CPU cores)
Locale: LANG=en_CA.UTF-8, LC_CTYPE=en_CA.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
diff -Nru librdkafka-0.8.3/debian/changelog librdkafka-0.8.3/debian/changelog
diff -Nru librdkafka-0.8.3/debian/control librdkafka-0.8.3/debian/control
--- librdkafka-0.8.3/debian/control	2014-02-17 17:22:09.0 -0700
+++ librdkafka-0.8.3/debian/control	2014-02-23 18:07:25.0 -0700
@@ -9,7 +9,7 @@
 Vcs-Browser: https://github.com/edenhill/librdkafka/tree/debian
 
 Package: librdkafka1
-Architecture: i386 amd64 x32 ia64 armel armhf arm64 ppc64 sparc64 s390x kfreebsd-i386 kfreebsd-amd64
+Architecture: any
 Depends: ${shlibs:Depends}, ${misc:Depends}
 Description: library implementing the Apache Kafka protocol
  librdkafka is a C implementation of the Apache Kafka protocol. It currently
@@ -20,7 +20,7 @@
 
 Package: librdkafka-dev
 Section: libdevel
-Architecture: i386 amd64 x32 ia64 armel armhf arm64 ppc64 sparc64 s390x kfreebsd-i386 kfreebsd-amd64
+Architecture: any
 Depends: librdkafka1 (= ${binary:Version}), ${misc:Depends}
 Description: library implementing the Apache Kafka protocol (development headers)
  librdkafka is a C implementation of the Apache Kafka protocol. It currently
@@ -34,7 +34,7 @@
 Package: librdkafka1-dbg
 Section: debug
 Priority: extra
-Architecture: i386 amd64 x32 ia64 armel armhf arm64 ppc64 sparc64 s390x kfreebsd-i386 kfreebsd-amd64
+Architecture: any
 Depends: librdkafka1 (= ${binary:Version}), ${misc:Depends}
 Description: library implementing the Apache Kafka protocol (debugging symbols)
  librdkafka is a C implementation of the Apache Kafka protocol. It currently
diff -Nru librdkafka-0.8.3/debian/patches/link-atomic.patch librdkafka-0.8.3/debian/patches/link-atomic.patch
--- librdkafka-0.8.3/debian/patches/link-atomic.patch	1969-12-31 17:00:00.0 -0700
+++ librdkafka-0.8.3/debian/patches/link-atomic.patch	2014-02-22 17:51:42.0 -0700
@@ -0,0 +1,78 @@
+Description: Switch from __sync functions to __atomic for better arch support
+Author: Adam Conrad adcon...@ubuntu.com
+
+Index: librdkafka-0.8.3/Makefile
+===
+--- librdkafka-0.8.3.orig/Makefile	2014-02-10 09:37:21.0 -0700
 librdkafka-0.8.3/Makefile	2014-02-22 17:15:42.0 -0700
+@@ -57,7 +57,7 @@
+ 		$(CC) $(LDFLAGS) \
+ 			-shared -Wl,-soname,$@ \
+ 			-Wl,--version-script=librdkafka.lds \
+-			$(OBJS) -o $@ -lpthread -lrt -lz -lc ; \
++			$(OBJS) -o $@ -lpthread -lrt -lz -lc -latomic ; \
+ 	fi)
+ 
+ $(LIBNAME).a:	$(OBJS)
+Index: librdkafka-0.8.3/examples/Makefile
+===
+--- librdkafka-0.8.3.orig/examples/Makefile	2014-02-10 09:37:21.0 -0700
 librdkafka-0.8.3/examples/Makefile	2014-02-22 17:15:57.648623936 -0700
+@@ -6,7 +6,7 @@
+ LDFLAGS +=  ../librdkafka.a
+ LDFLAGS += -lpthread -lz
+ ifeq ($(shell uname -s), Linux)
+-	LDFLAGS += -lrt
++	LDFLAGS += -lrt -latomic
+ endif
+ 
+ # Profiling
+Index: librdkafka-0.8.3/tests/Makefile
+===
+--- librdkafka-0.8.3.orig/tests/Makefile	2014-02-10 09:37:21.0 -0700
 librdkafka-0.8.3/tests/Makefile	2014-02-22 17:16:59.336621754 -0700
+@@ -8,7 +8,7 @@
+ LDFLAGS +=  -L../ -lrdkafka
+ LDFLAGS += -lpthread -lz
+ ifeq ($(shell uname -s), Linux)
+-	LDFLAGS += -lrt
++	LDFLAGS += -lrt -latomic
+ endif
+ 
+ # Profiling
+Index: librdkafka-0.8.3/README.md
+===
+--- librdkafka-0.8.3.orig/README.md	2014-02-22 17:17:42.672620221 -0700
 librdkafka-0.8.3/README.md	2014-02-22 17:18:06.864619365 -0700
+@@ -91,7 +91,7 @@
+ 
+ See [examples/rdkafka_example.c](https://github.com/edenhill/librdkafka/blob/master/examples/rdkafka_example.c) for an example producer and

Bug#739930: Please switch to -latomic atomics for wider arch portability

2014-02-23 Thread Adam Conrad 
On Sun, Feb 23, 2014 at 06:10:42PM -0700, Adam Conrad wrote:
 + 
 +-#define rd_atomic_add_prev(PTR,VAL)  __sync_fetch_and_add(PTR,VAL)
 +-#define rd_atomic_sub_prev(PTR,VAL)  __sync_fetch_and_sub(PTR,VAL)
 ++#define rd_atomic_add_prev(PTR,VAL)  
 __atmoic_fetch_add(PTR,VAL,__ATOMIC_SEQ_CST)

Except for this hilarious typo above that didn't show up in testing, as
the code never actually calls this.  Obviously, this should be __atomic,
not __atmoic.  Fixed now in the current Ubuntu version.

 ++#define rd_atomic_sub_prev(PTR,VAL)  
 __atomic_fetch_sub(PTR,VAL,__ATOMIC_SEQ_CST)
 + 
 +-#define rd_atomic_set(PTR,VAL) __sync_lock_test_and_set(PTR,VAL)
 ++#define rd_atomic_set(PTR,VAL) __atomic_test_and_set(PTR,__ATOMIC_SEQ_CST)

... Adam


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#739930: Please switch to -latomic atomics for wider arch portability

2014-02-23 Thread Faidon Liambotis 
Hi Adam,

On Sun, Feb 23, 2014 at 06:10:42PM -0700, Adam Conrad wrote:
   * link-atomic.patch: Switch from __sync to __atomic to support more arches.
   * Switch back to arch:any instead of the Debian maintainer's shorter list.
 
 So, this patch should be almost upstreamable, depending on who their
 intended target is.  If they need to support __sync on non-Linux,
 they could wrap __sync and __atomic blocks in ifdef/ifndef magic to
 test for linux or perhaps specific GCC versions.

Thanks for the patch. I had suggested the same and discussed it with
Magnus (upstream) too among other options[1], but the __atomic functions
are GCC 4.7+, which would break builds with e.g.  precise and are hence
unattractive for upstream inclusion.

Magnus has on his plans to rethink atomic operations and independently
of that switch to autoconf, so we could perhaps make this more
reasonable than a bunch of #ifdefs that would break with weird error
messages under certain combinations of GCC version/architecture :)

I'll poke him again. Thanks for your bug report.

Faidon

1: Another one would be limiting the queue size to 4GB on 32-bit
architectures (i.e. switch from uint64_t to long), which wouldn't be
such a big deal considering librdkafka's use cases.


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org