protobuf: apply patch to compile for PowerPC

From: Henrique Marks <[email protected]>

Signed-off-by Henrique Marks <[email protected]>

Signed-off-by Carlos Santos <[email protected]>

---
 package/protobuf/0001-PowerPC-Support.patch | 54 +++++++++++++++++++++++++++++
 package/protobuf/Config.in                  |  5 ++-
 2 files changed, 56 insertions(+), 3 deletions(-)
 create mode 100644 package/protobuf/0001-PowerPC-Support.patch

De: "Thomas Petazzoni" <[email protected]>
Para: "Carlos Santos" <[email protected]>
Cc: buildroot at buildroot.org
Enviadas: Quinta-feira, 4 de fevereiro de 2016 21:06:13
Assunto: Re: [Buildroot] [PATCH 1/1] protobuf: apply patch to compile for PowerPC

Carlos,

Quoted 9 lines · Carlos Santos

From: Henrique Marks <[email protected]>

Signed-off-by Henrique Marks <[email protected]>

Signed-off-by Carlos Santos <[email protected]>

---
 package/protobuf/0001-PowerPC-Support.patch | 54 +++++++++++++++++++++++++++++
 package/protobuf/Config.in                  |  5 ++-
 2 files changed, 56 insertions(+), 3 deletions(-)
 create mode 100644 package/protobuf/0001-PowerPC-Support.patch

This patch doesn't actually work. First there are a number of problems:

- The patch you apply has technically nothing to do with enabling the PowerPC architecture. It seems more related to supporting old compilers.

- The patch you apply is already applied upstream, so in this case, we prefer to use the upstream patch directly.

- You change the architecture dependencies in protobuf/Config.in, but forget to propagate this change to the reverse dependencies of protobuf, namely the mosh and ola packages. To make this easier, I've changed protobuf/Config.in to provide a BR2_PACKAGE_PROTOBUF_ARCH_SUPPORTS variable, and changed mosh and ola to use it. See commit
https://git.busybox.net/buildroot/commit/?id=abdc56006bf253bec393066f96f69f0a6246b896.

But then, despite those issues, your patch still doesn't build on PowerPC with the following defconfig for example:

BR2_powerpc=y
BR2_powerpc_8548=y
BR2_TOOLCHAIN_EXTERNAL=y
BR2_INIT_NONE=y
BR2_SYSTEM_BIN_SH_NONE=y
# BR2_PACKAGE_BUSYBOX is not set
BR2_PACKAGE_OLA=y
BR2_PACKAGE_MOSH=y
# BR2_TARGET_ROOTFS_TAR is not set

It fails with:

In file included from ./google/protobuf/stubs/once.h:81:0,
from google/protobuf/stubs/common.cc:34:
./google/protobuf/stubs/atomicops.h:209:2: error: #error GOOGLE_PROTOBUF_ATOMICOPS_ERROR
In file included from ./google/protobuf/stubs/once.h:81:0,
from google/protobuf/stubs/once.cc:38:
./google/protobuf/stubs/atomicops.h:209:2: error: #error GOOGLE_PROTOBUF_ATOMICOPS_ERROR
In file included from google/protobuf/stubs/atomicops_internals_x86_msvc.cc:37:0:
./google/protobuf/stubs/atomicops.h:209:2: error: #error GOOGLE_PROTOBUF_ATOMICOPS_ERROR
In file included from google/protobuf/stubs/atomicops_internals_x86_gcc.cc:36:0:
./google/protobuf/stubs/atomicops.h:209:2: error: #error GOOGLE_PROTOBUF_ATOMICOPS_ERROR

Looking at atomicops.h, I can read:

#elif defined(__GNUC__)
#if defined(GOOGLE_PROTOBUF_ARCH_IA32) || defined(GOOGLE_PROTOBUF_ARCH_X64) #include <google/protobuf/stubs/atomicops_internals_x86_gcc.h> #elif defined(GOOGLE_PROTOBUF_ARCH_ARM) && defined(__linux__) #include <google/protobuf/stubs/atomicops_internals_arm_gcc.h> #elif defined(GOOGLE_PROTOBUF_ARCH_AARCH64)
#include <google/protobuf/stubs/atomicops_internals_arm64_gcc.h> #elif defined(GOOGLE_PROTOBUF_ARCH_ARM_QNX)
#include <google/protobuf/stubs/atomicops_internals_arm_qnx.h> #elif defined(GOOGLE_PROTOBUF_ARCH_MIPS) || defined(GOOGLE_PROTOBUF_ARCH_MIPS64) #include <google/protobuf/stubs/atomicops_internals_mips_gcc.h> #elif defined(__native_client__)
#include <google/protobuf/stubs/atomicops_internals_pnacl.h>
#elif (((__GNUC__ == 4) && (__GNUC_MINOR__ >= 7)) || (__GNUC__ > 4)) #include <google/protobuf/stubs/atomicops_internals_generic_gcc.h> #elif defined(__clang__)
#if __has_extension(c_atomic)
#include <google/protobuf/stubs/atomicops_internals_generic_gcc.h> #else
#error GOOGLE_PROTOBUF_ATOMICOPS_ERROR
#endif
#else
#error GOOGLE_PROTOBUF_ATOMICOPS_ERROR
#endif

So it means that on i386, on x86-64, on ARM and on MIPS and MIPS64, there are built-in implementation for the atomic operations.

For all other architectures, it relies on atomicops_internals_generic_gcc.h, only when gcc >= 4.7. And indeed my tested toolchain only has gcc 4.5.

In fact atomicops_internals_generic_gcc.h uses the __atomic_*() built-ins of the compiler, which indeed are only introduced in gcc 4.7. But on some architectures, they require linking with -latomic. See my atomic patch series at
http://lists.busybox.net/pipermail/buildroot/2016-February/151672.html,
and especially patch
http://lists.busybox.net/pipermail/buildroot/2016-February/151674.html
which has all the gory details.

And in fact, with those atomics, not only PowerPC can be supported, but any other architecture (except if protobuf has other architecture-specific dependencies elsewhere).

So, once my atomic patch series is merged, we could do:

config BR2_PACKAGE_PROTOBUF_ARCH_SUPPORTS
bool
default y if BR2_arm
default y if BR2_i386
default y if BR2_mipsel
default y if BR2_x86_64
default y if BR2_TOOLCHAIN_HAS_ATOMIC
depends on BR2_HOSTARCH = "x86_64" || BR2_HOSTARCH = "x86"

*and* ensure protobuf gets linked with -latomic.

What do you think ?

Best regards,

Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
_______________________________________________
buildroot mailing list
buildroot at busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot

I agree with you, but let me say that the original patch just corrects a "syntax error" in the code !

+ #define GOOGLE_PROTOBUF_ATOMICOPS_ERROR \
+-#error "Atomic operations are not supported on your platform" ++"Atomic operations are not supported on your platform"

This syntax error correction was submitted upstream, but wasnt applied to protobuf 2.6.1, just to protobuf 3x series. As we cannot drive the push to protobuf 3.x series right now (but we can in the near future, as soon as the first 3.x appears), we submitted a patch to buildroot.

Despite of this, the solution using the atomic patch you sent seems ok. We are using the patch we submmited for six months now, internally, and we try to send upstream everything, as soon as possible, so that we can keep up with the upstream tree. When your solution goes in, we can remove our internal patch (it is submmited in protobuf upstream 3.x anyway).

De: "Thomas Petazzoni" <[email protected]>
Para: "DATACOM" <[email protected]>
Cc: "Carlos Santos" <[email protected]>, buildroot at buildroot.org
Enviadas: Sexta-feira, 5 de fevereiro de 2016 11:09:09
Assunto: Re: [Buildroot] [PATCH 1/1] protobuf: apply patch to compile for PowerPC

Hello Henrique,

Quoted 6 lines · Henrique Marks

I agree with you, but let me say that the original patch just corrects a "syntax error" in the code !

+ #define GOOGLE_PROTOBUF_ATOMICOPS_ERROR \
+-#error "Atomic operations are not supported on your platform" ++"Atomic operations are not supported on your platform"

Correct. In fact I thought it was only with recent compilers, but it is probably incorrect regardless of the gcc version, and it doesn't fail in all cases because we ensure that protobuf is only built on architecture on which protobuf has built-in support for atomic operations. And therefore you don't fall into the #else cases where this bogus error macro is used.

Quoted 5 lines

This syntax error correction was submitted upstream, but wasnt applied to protobuf 2.6.1, just to protobuf 3x series. As we cannot drive the push to protobuf 3.x series right now (but we can in the near future, as soon as the first 3.x appears), we submitted a patch to buildroot.

Sure. But in this case, we prefer the patch to be a backport from upstream, so that it is clearer when bumping that the patch can be dropped.

And also, when a patch has been accepted by upstream, we have a higher confidence that the patch is correct.

Quoted 6 lines

Despite of this, the solution using the atomic patch you sent seems ok. We are using the patch we submmited for six months now, internally, and we try to send upstream everything, as soon as possible, so that we can keep up with the upstream tree. When your solution goes in, we can remove our internal patch (it is submmited in protobuf upstream 3.x anyway).

OK. Could you work on a proper patch series on the atomic series is merged in master ?

Thanks!

Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Yes, once the atomic series enter master branch, we are going to proceed on with this patch:

- Change protobuf, as you stated.
- Change Dependent Packages, it is four or five last time i checked out.
- Build on powerpc these packages, with gcc > 4.8

Yes, once the atomic series enter master branch, we are going to proceed on with this patch:

- Change protobuf, as you stated.
- Change Dependent Packages, it is four or five last time i checked out.
- Build on powerpc these packages, with gcc > 4.8

From: "Thomas Petazzoni" <[email protected]>
To: "Carlos Santos" <[email protected]>
Cc: buildroot at buildroot.org
Sent: Thursday, February 4, 2016 9:06:13 PM
Subject: Re: [Buildroot] [PATCH 1/1] protobuf: apply patch to compile for PowerPC

Carlos,

Quoted 9 lines · Carlos Santos

From: Henrique Marks <[email protected]>

Signed-off-by Henrique Marks <[email protected]>

Signed-off-by Carlos Santos <[email protected]>

---
 package/protobuf/0001-PowerPC-Support.patch | 54 +++++++++++++++++++++++++++++
 package/protobuf/Config.in                  |  5 ++-
 2 files changed, 56 insertions(+), 3 deletions(-)
 create mode 100644 package/protobuf/0001-PowerPC-Support.patch

This patch doesn't actually work. First there are a number of problems:

- The patch you apply has technically nothing to do with enabling the PowerPC architecture. It seems more related to supporting old compilers.

- The patch you apply is already applied upstream, so in this case, we prefer to use the upstream patch directly.

- You change the architecture dependencies in protobuf/Config.in, but forget to propagate this change to the reverse dependencies of protobuf, namely the mosh and ola packages. To make this easier, I've changed protobuf/Config.in to provide a BR2_PACKAGE_PROTOBUF_ARCH_SUPPORTS variable, and changed mosh and ola to use it. See commit
https://git.busybox.net/buildroot/commit/?id=abdc56006bf253bec393066f96f69f0a6246b896.

But then, despite those issues, your patch still doesn't build on PowerPC with the following defconfig for example:

BR2_powerpc=y
BR2_powerpc_8548=y
BR2_TOOLCHAIN_EXTERNAL=y
BR2_INIT_NONE=y
BR2_SYSTEM_BIN_SH_NONE=y
# BR2_PACKAGE_BUSYBOX is not set
BR2_PACKAGE_OLA=y
BR2_PACKAGE_MOSH=y
# BR2_TARGET_ROOTFS_TAR is not set

It fails with:

In file included from ./google/protobuf/stubs/once.h:81:0,
from google/protobuf/stubs/common.cc:34:
./google/protobuf/stubs/atomicops.h:209:2: error: #error GOOGLE_PROTOBUF_ATOMICOPS_ERROR
In file included from ./google/protobuf/stubs/once.h:81:0,
from google/protobuf/stubs/once.cc:38:
./google/protobuf/stubs/atomicops.h:209:2: error: #error GOOGLE_PROTOBUF_ATOMICOPS_ERROR
In file included from google/protobuf/stubs/atomicops_internals_x86_msvc.cc:37:0:
./google/protobuf/stubs/atomicops.h:209:2: error: #error GOOGLE_PROTOBUF_ATOMICOPS_ERROR
In file included from google/protobuf/stubs/atomicops_internals_x86_gcc.cc:36:0:
./google/protobuf/stubs/atomicops.h:209:2: error: #error GOOGLE_PROTOBUF_ATOMICOPS_ERROR

Looking at atomicops.h, I can read:

#elif defined(__GNUC__)
#if defined(GOOGLE_PROTOBUF_ARCH_IA32) || defined(GOOGLE_PROTOBUF_ARCH_X64) #include <google/protobuf/stubs/atomicops_internals_x86_gcc.h> #elif defined(GOOGLE_PROTOBUF_ARCH_ARM) && defined(__linux__) #include <google/protobuf/stubs/atomicops_internals_arm_gcc.h> #elif defined(GOOGLE_PROTOBUF_ARCH_AARCH64)
#include <google/protobuf/stubs/atomicops_internals_arm64_gcc.h> #elif defined(GOOGLE_PROTOBUF_ARCH_ARM_QNX)
#include <google/protobuf/stubs/atomicops_internals_arm_qnx.h> #elif defined(GOOGLE_PROTOBUF_ARCH_MIPS) || defined(GOOGLE_PROTOBUF_ARCH_MIPS64) #include <google/protobuf/stubs/atomicops_internals_mips_gcc.h> #elif defined(__native_client__)
#include <google/protobuf/stubs/atomicops_internals_pnacl.h>
#elif (((__GNUC__ == 4) && (__GNUC_MINOR__ >= 7)) || (__GNUC__ > 4)) #include <google/protobuf/stubs/atomicops_internals_generic_gcc.h> #elif defined(__clang__)
#if __has_extension(c_atomic)
#include <google/protobuf/stubs/atomicops_internals_generic_gcc.h> #else
#error GOOGLE_PROTOBUF_ATOMICOPS_ERROR
#endif
#else
#error GOOGLE_PROTOBUF_ATOMICOPS_ERROR
#endif

So it means that on i386, on x86-64, on ARM and on MIPS and MIPS64, there are built-in implementation for the atomic operations.

For all other architectures, it relies on atomicops_internals_generic_gcc.h, only when gcc >= 4.7. And indeed my tested toolchain only has gcc 4.5.

In fact atomicops_internals_generic_gcc.h uses the __atomic_*() built-ins of the compiler, which indeed are only introduced in gcc 4.7. But on some architectures, they require linking with -latomic. See my atomic patch series at
http://lists.busybox.net/pipermail/buildroot/2016-February/151672.html,
and especially patch
http://lists.busybox.net/pipermail/buildroot/2016-February/151674.html
which has all the gory details.

And in fact, with those atomics, not only PowerPC can be supported, but any other architecture (except if protobuf has other architecture-specific dependencies elsewhere).

So, once my atomic patch series is merged, we could do:

config BR2_PACKAGE_PROTOBUF_ARCH_SUPPORTS
bool
default y if BR2_arm
default y if BR2_i386
default y if BR2_mipsel
default y if BR2_x86_64

	default y if BR2_TOOLCHAIN_HAS_ATOMIC
       depends on BR2_HOSTARCH = "x86_64" || BR2_HOSTARCH = "x86"

*and* ensure protobuf gets linked with -latomic.

What do you think ?

Quoted 6 lines

config BR2_PACKAGE_PROTOBUF_ARCH_SUPPORTS
bool
default y if BR2_arm
default y if BR2_i386
default y if BR2_mipsel
default y if BR2_x86_64

These architecture checks would be superseded by the BR2_TOOLCHAIN_HAS_ATOMIC boolean.

Quoted 4 lines

	default y if BR2_TOOLCHAIN_HAS_ATOMIC
       depends on BR2_HOSTARCH = "x86_64" || BR2_HOSTARCH = "x86"

*and* ensure protobuf gets linked with -latomic.

Hum, this requires a patch on configure.ac that that would hardly be accepted upstream.

From: "Thomas Petazzoni" <[email protected]>
To: "Carlos Santos" <[email protected]>
Cc: buildroot at buildroot.org
Sent: Wednesday, February 10, 2016 1:57:00 PM
Subject: Re: [Buildroot] [PATCH 1/1] protobuf: apply patch to compile for PowerPC

Dear Carlos Santos,

Quoted 9 lines · Carlos Santos

Quoted 6 lines

config BR2_PACKAGE_PROTOBUF_ARCH_SUPPORTS
bool
default y if BR2_arm
default y if BR2_i386
default y if BR2_mipsel
default y if BR2_x86_64

These architecture checks would be superseded by the BR2_TOOLCHAIN_HAS_ATOMIC boolean.

Indeed.

Quoted 7 lines

Quoted 4 lines

	default y if BR2_TOOLCHAIN_HAS_ATOMIC
       depends on BR2_HOSTARCH = "x86_64" || BR2_HOSTARCH = "x86"

*and* ensure protobuf gets linked with -latomic.

Hum, this requires a patch on configure.ac that that would hardly be accepted upstream.

Why? If they use __atomic_*() built-ins, then they must link with libatomic if it exists, since __atomic_*() built-ins are not guaranteed to be available on all architectures if you don't link with libatomic.

Look for example at the Android NDK documentation, which says that you should link with libatomic when using <atomic> in C++ :

http://developer.android.com/ndk/guides/cpp-support.html

(search for "atomic support").

The problem is the same you found on mpd (commit 84533029c70a4dffb2cd2e4f05e3903fd1b8fcd9) in branch master. With the patch in configure.ac I get this:

$ owerpc-e500v2-linux-gnuspe-ldd --root target/ target/usr/bin/protoc
        libprotobuf.so.9 => /usr/lib/libprotobuf.so.9 (0xdeadbeef)
        libatomic.so.1 => /lib/libatomic.so.1 (0xdeadbeef)
        libpthread.so.0 => /lib/libpthread.so.0 (0xdeadbeef)
        libc.so.6 => /lib/libc.so.6 (0xdeadbeef)
        ld.so.1 => /lib/ld.so.1 (0xdeadbeef)
        libz.so.1 => /usr/lib/libz.so.1 (0xdeadbeef)
        libstdc++.so.6 => /usr/lib/libstdc++.so.6 (0xdeadbeef)
        libm.so.6 => /lib/libm.so.6 (0xdeadbeef)
        libgcc_s.so.1 => /lib/libgcc_s.so.1 (0xdeadbeef)
        libprotoc.so.9 => /usr/lib/libprotoc.so.9 (0xdeadbeef)

Without the patch protoc is not linked to libatomic.so.1.

From: "Thomas Petazzoni" <[email protected]>
To: "Carlos Santos" <[email protected]>
Cc: buildroot at buildroot.org
Sent: Wednesday, February 10, 2016 2:44:25 PM
Subject: Re: [Buildroot] [PATCH 1/1] protobuf: apply patch to compile for PowerPC

Dear Carlos Santos,

Quoted 4 lines · Carlos Santos

The problem is the same you found on mpd (commit 84533029c70a4dffb2cd2e4f05e3903fd1b8fcd9) in branch master. With the patch in configure.ac I get this:

Quoted 11 lines

$ owerpc-e500v2-linux-gnuspe-ldd --root target/ target/usr/bin/protoc
        libprotobuf.so.9 => /usr/lib/libprotobuf.so.9 (0xdeadbeef)
        libatomic.so.1 => /lib/libatomic.so.1 (0xdeadbeef)
        libpthread.so.0 => /lib/libpthread.so.0 (0xdeadbeef)
        libc.so.6 => /lib/libc.so.6 (0xdeadbeef)
        ld.so.1 => /lib/ld.so.1 (0xdeadbeef)
        libz.so.1 => /usr/lib/libz.so.1 (0xdeadbeef)
        libstdc++.so.6 => /usr/lib/libstdc++.so.6 (0xdeadbeef)
        libm.so.6 => /lib/libm.so.6 (0xdeadbeef)
        libgcc_s.so.1 => /lib/libgcc_s.so.1 (0xdeadbeef)
        libprotoc.so.9 => /usr/lib/libprotoc.so.9 (0xdeadbeef)

Yes, looks good.

Quoted 1 line

Without the patch protoc is not linked to libatomic.so.1.

Indeed.

And so, what's the problem ?

From: "Thomas Petazzoni" <[email protected]>
To: "Carlos Santos" <[email protected]>
Cc: buildroot at buildroot.org
Sent: Wednesday, February 10, 2016 2:44:25 PM
Subject: Re: [Buildroot] [PATCH 1/1] protobuf: apply patch to compile for PowerPC

Dear Carlos Santos,

Quoted 4 lines · Carlos Santos

The problem is the same you found on mpd (commit 84533029c70a4dffb2cd2e4f05e3903fd1b8fcd9) in branch master. With the patch in configure.ac I get this:

Quoted 11 lines

$ owerpc-e500v2-linux-gnuspe-ldd --root target/ target/usr/bin/protoc
        libprotobuf.so.9 => /usr/lib/libprotobuf.so.9 (0xdeadbeef)
        libatomic.so.1 => /lib/libatomic.so.1 (0xdeadbeef)
        libpthread.so.0 => /lib/libpthread.so.0 (0xdeadbeef)
        libc.so.6 => /lib/libc.so.6 (0xdeadbeef)
        ld.so.1 => /lib/ld.so.1 (0xdeadbeef)
        libz.so.1 => /usr/lib/libz.so.1 (0xdeadbeef)
        libstdc++.so.6 => /usr/lib/libstdc++.so.6 (0xdeadbeef)
        libm.so.6 => /lib/libm.so.6 (0xdeadbeef)
        libgcc_s.so.1 => /lib/libgcc_s.so.1 (0xdeadbeef)
        libprotoc.so.9 => /usr/lib/libprotoc.so.9 (0xdeadbeef)

Yes, looks good.

Quoted 1 line

Without the patch protoc is not linked to libatomic.so.1.

Indeed.

And so, what's the problem ?

Henrique just warned me (offline) about the misunderstanding. The "problem" here is that the patch on configure.ac would not be accepted upstream, since their detection of the atomic operations already works.

From: "Thomas Petazzoni" <[email protected]>
To: "Carlos Santos" <[email protected]>
Cc: buildroot at buildroot.org
Sent: Wednesday, February 10, 2016 6:13:16 PM
Subject: Re: [Buildroot] [PATCH 1/1] protobuf: apply patch to compile for PowerPC

Carlos,

On Wed, 10 Feb 2016 16:30:19 -0200 (BRST), Carlos Santos wrote:

Quoted 4 lines

Henrique just warned me (offline) about the misunderstanding. The "problem" here is that the patch on configure.ac would not be accepted upstream, since their detection of the atomic operations already works.

What? It certainly cannot work if they don't link against libatomic. Try on an architecture like SPARC, and you will see that if you don't link with libatomic, the build will fail.

package/protobuf/0002-configure.ac-check-if-libatomic-is-needed.patch new file +4 -0

new file mode 100644
index 0000000..237bc71
@@ -0,0 +1,34 @@
+From 0883fa19d59ece19eec30937c65fd10162ef57b0 Mon Sep 17 00:00:00 2001
+From: Carlos Santos <[email protected]>
+Date: Wed, 10 Feb 2016 12:54:43 -0200
+Subject: [PATCH] configure.ac: check if libatomic is needed

From: "Thomas Petazzoni" <[email protected]>
To: "Carlos Santos" <[email protected]>
Cc: buildroot at buildroot.org, "henrique marks" <[email protected]>
Sent: Wednesday, February 10, 2016 1:50:37 PM
Subject: Re: [PATCH 1/1] protobuf: fix detection of __atomic_*() built-ins

Hello,

Thanks, this looks good, with one nit, see below.

Quoted 19 lines · Carlos Santos

diff --git a/package/protobuf/0002-configure.ac-check-if-libatomic-is-needed.patch b/package/protobuf/0002-configure.ac-check-if-libatomic-is-needed.patch new file mode 100644
index 0000000..237bc71

package/protobuf/0002-configure.ac-check-if-libatomic-is-needed.patch new file +11 -0

@@ -0,0 +1,34 @@
+From 0883fa19d59ece19eec30937c65fd10162ef57b0 Mon Sep 17 00:00:00 2001
+From: Carlos Santos <[email protected]>
+Date: Wed, 10 Feb 2016 12:54:43 -0200
+Subject: [PATCH] configure.ac: check if libatomic is needed
+
+In Buildroot, to simplify things, we've decided to simply require gcc 4.8
+as soon as the architectures has at least one __atomic_*() built-in
+variant that requires libatomic.
+
+Since protobuf most likely only uses the 1, 2 and 4-byte variants, it
+*could* technically build with gcc 4.7. This is probably not a big deal,

package/protobuf/0002-configure.ac-check-if-libatomic-is-needed.patch line 14

Quoted 3 lines

package/protobuf/0002-configure.ac-check-if-libatomic-is-needed.patch new file +3 -0

+and we can live with requiring gcc 4.8 on PowerPC to build protobuf.
+
+Signed-off-by: Carlos Santos <[email protected]>

The patch description should not mention Buildroot and not mention Buildroot specific choices. It should be written as if you were going to submit it upstream, i.e with a proper justification as to why linking with libatomic may be needed.

And in fact, I'm even going to ask you to submit this patch upstream :-)

[Thanks, Zimbra, for messing rearranging the messages in the inbox, so I answer them in the wrong order].

Quoted 31 lines

From: "Thomas Petazzoni" <[email protected]>
To: "Carlos Santos" <[email protected]>
Cc: buildroot at buildroot.org, "henrique marks" <[email protected]>
Sent: Wednesday, February 10, 2016 1:50:37 PM
Subject: Re: [PATCH 1/1] protobuf: fix detection of __atomic_*() built-ins

Hello,

Thanks, this looks good, with one nit, see below.

Quoted 19 lines · Carlos Santos

diff --git a/package/protobuf/0002-configure.ac-check-if-libatomic-is-needed.patch b/package/protobuf/0002-configure.ac-check-if-libatomic-is-needed.patch new file mode 100644
index 0000000..237bc71

package/protobuf/0002-configure.ac-check-if-libatomic-is-needed.patch new file +11 -0

@@ -0,0 +1,34 @@
+From 0883fa19d59ece19eec30937c65fd10162ef57b0 Mon Sep 17 00:00:00 2001
+From: Carlos Santos <[email protected]>
+Date: Wed, 10 Feb 2016 12:54:43 -0200
+Subject: [PATCH] configure.ac: check if libatomic is needed
+
+In Buildroot, to simplify things, we've decided to simply require gcc 4.8
+as soon as the architectures has at least one __atomic_*() built-in
+variant that requires libatomic.
+
+Since protobuf most likely only uses the 1, 2 and 4-byte variants, it
+*could* technically build with gcc 4.7. This is probably not a big deal,

Quoted 8 lines

package/protobuf/0002-configure.ac-check-if-libatomic-is-needed.patch line 14

Quoted 3 lines

package/protobuf/0002-configure.ac-check-if-libatomic-is-needed.patch new file +3 -0

+and we can live with requiring gcc 4.8 on PowerPC to build protobuf.
+
+Signed-off-by: Carlos Santos <[email protected]>

The patch description should not mention Buildroot and not mention Buildroot specific choices. It should be written as if you were going to submit it upstream, i.e with a proper justification as to why linking with libatomic may be needed.

This patch only exists to appease Buildroot but, anyway, I can rewrite the comment.

Quoted 1 line

And in fact, I'm even going to ask you to submit this patch upstream :-)

They don't need this. Their detection of the atomic built-ins already works without additional help.

package/protobuf/Config.in +11 -0

+#
+# On PowerPC, the __atomic_*() built-ins for 1-byte, 2-byte and 4-byte
+# types are available built-in. However, the __atomic_*() built-ins for
+# 8-byte types is implemented via libatomic, so only available since gcc
+# 4.8.
+#
+# In Buildroot, to simplify things, we've decided to simply require gcc
+# 4.8 as soon as the architectures has at least one __atomic_*() built-in
+# variant that requires libatomic.
+#
+# Since protobuf most likely only uses the 1, 2 and 4-byte variants, it

package/protobuf/Config.in +1 -0

+	default y if BR2_TOOLCHAIN_HAS_ATOMIC
 	depends on BR2_HOSTARCH = "x86_64" || BR2_HOSTARCH = "x86"

From: "Arnout Vandecappelle" <[email protected]>
To: "Carlos Santos" <[email protected]>, buildroot at buildroot.org
Cc: "thomas petazzoni" <[email protected]>
Sent: Wednesday, February 10, 2016 6:00:16 PM
Subject: Re: [Buildroot] [PATCH 1/1] protobuf: fix detection of __atomic_*() built-ins

On 10-02-16 16:33, Carlos Santos wrote:
[snip]

Quoted 17 lines

package/protobuf/Config.in +14 -0

+#
+# On PowerPC, the __atomic_*() built-ins for 1-byte, 2-byte and 4-byte
+# types are available built-in. However, the __atomic_*() built-ins for
+# 8-byte types is implemented via libatomic, so only available since gcc
+# 4.8.
+#
+# In Buildroot, to simplify things, we've decided to simply require gcc
+# 4.8 as soon as the architectures has at least one __atomic_*() built-in
+# variant that requires libatomic.
+#
+# Since protobuf most likely only uses the 1, 2 and 4-byte variants, it
+# *could* technically build with gcc 4.7. This is probably not a big deal,
+# and we can live with requiring gcc 4.8 on PowerPC to build protobuf.
+#
 # host-protobuf only builds on certain architectures
 config BR2_PACKAGE_PROTOBUF_ARCH_SUPPORTS
 	bool

package/protobuf/Config.in line 9

Quoted 4 lines

package/protobuf/Config.in +0 -4

-	default y if BR2_arm
-	default y if BR2_i386
-	default y if BR2_mipsel
-	default y if BR2_x86_64

Sorry for my confusion, but why do you remove these? It will still work with gcc 4.7 on arm, i386, mipsel and x86_64 because it will use its custom atomic implementation, no?

- Use the recently introduced BR2_TOOLCHAIN_HAS_ATOMIC boolean.

- Import an upstream patch to fix error handling when atomic operations are not detected. Without this patch the build fails due to a syntax error instead of showing the proper message.

- Add a patch to configure.ac to check if libatomic is needed and force linking to it (we will attempt to submit this upstream).

- Disable build for SPARC64 because it fails due to a missing definition of Atomic64.

On PowerPC, the __atomic_*() built-ins for 1-byte, 2-byte and 4-byte types are available built-in. The corresponding built-ins for 8-byte types, however, are implemented via libatomic, so requiring gcc >= 4.8.

In Buildroot, to simplify things, it was decided to require gcc 4.8 as soon as the architectures has at least one __atomic_*() built-in variant that requires libatomic.

Since protobuf most likely only uses the 1, 2 and 4-byte variants, it *could* technically build with gcc 4.7. This is probably not a big deal, and we can live with requiring gcc 4.8 on PowerPC to build protobuf. The same restriction applies to SPARC.

The build for SPARC64 breaks even using the master branch of protobuf due to undefined references to some NoBarrier_Atomic*() functions.

Signed-off-by Henrique Marks <[email protected]>

Signed-off-by Carlos Santos <[email protected]>

- Use the recently introduced BR2_TOOLCHAIN_HAS_ATOMIC boolean.

- Import an upstream patch to fix error handling when atomic operations are not detected. Without this patch the build fails due to a syntax error instead of showing the proper message.

- Add a patch to configure.ac to check if libatomic is needed and force linking to it (we will attempt to submit this upstream).

- Disable build for SPARC64 because it fails due to a missing definition of Atomic64.

On PowerPC, the __atomic_*() built-ins for 1-byte, 2-byte and 4-byte types are available built-in. The corresponding built-ins for 8-byte types, however, are implemented via libatomic, so requiring gcc >= 4.8.

In Buildroot, to simplify things, it was decided to require gcc 4.8 as soon as the architectures has at least one __atomic_*() built-in variant that requires libatomic.

Since protobuf most likely only uses the 1, 2 and 4-byte variants, it *could* technically build with gcc 4.7. This is probably not a big deal, and we can live with requiring gcc 4.8 on PowerPC to build protobuf. The same restriction applies to SPARC.

The build for SPARC64 breaks even using the master branch of protobuf due to undefined references to some NoBarrier_Atomic*() functions.

Signed-off-by Henrique Marks <[email protected]>

Signed-off-by Carlos Santos <[email protected]>

---
 ...GLE_PROTOBUF_ATOMICOPS_ERROR-syntax-error.patch | 61 ++++++++++++++++++++++
 ...configure.ac-check-if-libatomic-is-needed.patch | 34 ++++++++++++
 package/protobuf/Config.in                         | 21 +++++++-
 3 files changed, 115 insertions(+), 1 deletion(-)
 create mode 100644 package/protobuf/0001-Fix-GOOGLE_PROTOBUF_ATOMICOPS_ERROR-syntax-error.patch
 create mode 100644 package/protobuf/0002-configure.ac-check-if-libatomic-is-needed.patch

From: "Carlos Santos" <[email protected]>
To: buildroot at buildroot.org
Cc: "thomas petazzoni" <[email protected]>
Sent: Wednesday, February 17, 2016 3:43:00 PM
Subject: [Buildroot] [PATCH v2 0/1] protobuf: fix detection of __atomic_*() built-ins

- Clarify that we *will* attempt to submit upstream the patch in configure.ac.

- Small improvements on the wording of the commit message.

Carlos Santos (1):
protobuf: fix detection of __atomic_*() built-ins

...GLE_PROTOBUF_ATOMICOPS_ERROR-syntax-error.patch | 61 ++++++++++++++++++++++
...configure.ac-check-if-libatomic-is-needed.patch | 34 ++++++++++++
package/protobuf/Config.in                         | 21 +++++++-
3 files changed, 115 insertions(+), 1 deletion(-)

create mode 100644 package/protobuf/0001-Fix-GOOGLE_PROTOBUF_ATOMICOPS_ERROR-syntax-error.patch create mode 100644 package/protobuf/0002-configure.ac-check-if-libatomic-is-needed.patch

Quoted 5 lines

From: "Carlos Santos" <[email protected]>
To: buildroot at buildroot.org
Cc: "thomas petazzoni" <[email protected]>
Sent: Wednesday, February 17, 2016 3:43:00 PM
Subject: [Buildroot] [PATCH v2 0/1] protobuf: fix detection of __atomic_*() built-ins

Quoted 2 lines

- Clarify that we *will* attempt to submit upstream the patch in configure.ac.

In progress:

- https://goo.gl/Kzju4X
- https://goo.gl/v6bypJ