| Message ID | 1453986515-9505-1-git-send-email-casantos@datacom.ind.br |
|---|---|
| State | Rejected |
| Headers | show |
Carlos, On Thu, 28 Jan 2016 11:08:35 -0200, Carlos Santos wrote: > From: Henrique Marks <henrique.marks@datacom.ind.br> > > Signed-off-by: Henrique Marks <henrique.marks@datacom.ind.br> > Signed-off-by: Carlos Santos <casantos@datacom.ind.br> > --- > 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
Hello 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" The macro GOOGLE_PROTOBUF_ATOMICOPS_ERROR expands to "another" pre-processor directive (#error "Message"). So if the Macro gets expanded, it will generate a strange message "error: #error ..." 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). Thanks for the analysis ----- Mensagem original ----- > De: "Thomas Petazzoni" <thomas.petazzoni@free-electrons.com> > Para: "Carlos Santos" <casantos@datacom.ind.br> > Cc: buildroot@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, > > On Thu, 28 Jan 2016 11:08:35 -0200, Carlos Santos wrote: >> From: Henrique Marks <henrique.marks@datacom.ind.br> >> >> Signed-off-by: Henrique Marks <henrique.marks@datacom.ind.br> >> Signed-off-by: Carlos Santos <casantos@datacom.ind.br> >> --- >> 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@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Hello Henrique, On Fri, 5 Feb 2016 09:04:53 -0200 (BRST), Henrique Marks wrote: > 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. > 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. > 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
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 I guess this is enough. Thanks ----- Mensagem original ----- > De: "Thomas Petazzoni" <thomas.petazzoni@free-electrons.com> > Para: "DATACOM" <henrique.marks@datacom.ind.br> > Cc: "Carlos Santos" <casantos@datacom.ind.br>, buildroot@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, > > On Fri, 5 Feb 2016 09:04:53 -0200 (BRST), Henrique Marks wrote: > >> 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. > >> 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. > >> 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
Hello, (Please try to avoid top-posting, this is considered bad practice on most mailing list) On Fri, 5 Feb 2016 11:22:58 -0200 (BRST), Henrique Marks wrote: > 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 Sounds good. There is only one gotcha/limitation introduced by the atomic series: the fact that building protobuf with gcc 4.7 will not be allowed, while in fact it seems to be possible. On PowerPC, the __atomic_*() built-ins for 1-byte, 2-byte and 4-byte types are available built-in, without the need for libatomic. This means that the __atomic_*() built-ins for those sizes are available in gcc 4.7. 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. But in fact, protobuf most likely only uses the 1, 2 and 4-byte variants, so it *could* technically build with gcc 4.7. But oh, well, it's probably not a big deal, and we can live with requiring gcc 4.8 on PowerPC to build protobuf. Is that OK for you? If we want to do a more fine-grained selection, we would have to introduce multiple BR2_TOOLCHAIN_HAS_ATOMIC_<x> options, like I've done for the __sync_*() built-ins. It's possible, but a big annoying especially since gcc 4.8 has everything needed. Best regards, Thomas
Dear Henrique Marks, On Fri, 5 Feb 2016 11:22:58 -0200 (BRST), Henrique Marks wrote: > 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 The atomic series has been merged in master. Can you respin this protobuf patch, taking into account the comments I made? Thanks! Thomas
----- Original Message ----- > From: "Thomas Petazzoni" <thomas.petazzoni@free-electrons.com> > To: "Carlos Santos" <casantos@datacom.ind.br> > Cc: buildroot@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, > > On Thu, 28 Jan 2016 11:08:35 -0200, Carlos Santos wrote: >> From: Henrique Marks <henrique.marks@datacom.ind.br> >> >> Signed-off-by: Henrique Marks <henrique.marks@datacom.ind.br> >> Signed-off-by: Carlos Santos <casantos@datacom.ind.br> >> --- >> 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 These architecture checks would be superseded by the BR2_TOOLCHAIN_HAS_ATOMIC boolean. > 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. > What do you think ? Ok, I will send a new patch. Carlos Santos (Casantos) DATACOM, P&D
Dear Carlos Santos, On Wed, 10 Feb 2016 13:25:49 -0200 (BRST), Carlos Santos wrote: > > 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. > > 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"). Best regards, Thomas
----- Original Message ----- > From: "Thomas Petazzoni" <thomas.petazzoni@free-electrons.com> > To: "Carlos Santos" <casantos@datacom.ind.br> > Cc: buildroot@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, > > On Wed, 10 Feb 2016 13:25:49 -0200 (BRST), Carlos Santos wrote: > >> > 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. > >> > 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. Carlos Santos (Casantos) DATACOM, P&D
Dear Carlos Santos, On Wed, 10 Feb 2016 14:32:06 -0200 (BRST), Carlos Santos wrote: > 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) Yes, looks good. > Without the patch protoc is not linked to libatomic.so.1. Indeed. And so, what's the problem ? Thanks, Thomas
> From: "Thomas Petazzoni" <thomas.petazzoni@free-electrons.com> > To: "Carlos Santos" <casantos@datacom.ind.br> > Cc: buildroot@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, > > On Wed, 10 Feb 2016 14:32:06 -0200 (BRST), Carlos Santos wrote: > >> 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) > > Yes, looks good. > >> Without the patch protoc is not linked to libatomic.so.1. > > Indeed. > > And so, what's the problem ? No problem, IMO. It's a solution. :-) Carlos Santos (Casantos) DATACOM, P&D
> From: "Thomas Petazzoni" <thomas.petazzoni@free-electrons.com> > To: "Carlos Santos" <casantos@datacom.ind.br> > Cc: buildroot@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, > > On Wed, 10 Feb 2016 14:32:06 -0200 (BRST), Carlos Santos wrote: > >> 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) > > Yes, looks good. > >> 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. This is not big deal, IMO, so if you are satisfied with the patch then go ahead and apply it. Carlos Santos (Casantos) DATACOM, P&D
Carlos, On Wed, 10 Feb 2016 16:30:19 -0200 (BRST), Carlos Santos wrote: > 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. Best regards, Thomas
> From: "Thomas Petazzoni" <thomas.petazzoni@free-electrons.com> > To: "Carlos Santos" <casantos@datacom.ind.br> > Cc: buildroot@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: > >> 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. I tried. It worked on SPARC (with the libatomic patch) but failed on SPARC64 due to a missing declaration of "Atomic64". I guess their SPARC code aims Solaris, not Linux/buildroot. Carlos Santos (Casantos) DATACOM, P&D
diff --git a/package/protobuf/0001-PowerPC-Support.patch b/package/protobuf/0001-PowerPC-Support.patch new file mode 100644 index 0000000..aee3717 --- /dev/null +++ b/package/protobuf/0001-PowerPC-Support.patch @@ -0,0 +1,54 @@ +From d56c6b19b18dc459c1ea6b720ef015afe72757ea Mon Sep 17 00:00:00 2001 +From: Henrique Marks <henrique.marks@datacom.ind.br> +Date: Fri, 28 Aug 2015 18:55:49 -0300 +Subject: [PATCH 1/1] Syntax Error Patch + +Signed-off-by: Henrique Marks <henrique.marks@datacom.ind.br> +--- + src/google/protobuf/stubs/atomicops.h | 10 +++++----- + 1 file changed, 5 insertions(+), 5 deletions(-) + +diff --git a/src/google/protobuf/stubs/atomicops.h b/src/google/protobuf/stubs/atomicops.h +index b1336e3..a130b38 100644 +--- a/src/google/protobuf/stubs/atomicops.h ++++ b/src/google/protobuf/stubs/atomicops.h +@@ -162,7 +162,7 @@ Atomic64 Release_Load(volatile const Atomic64* ptr); + + // Include our platform specific implementation. + #define GOOGLE_PROTOBUF_ATOMICOPS_ERROR \ +-#error "Atomic operations are not supported on your platform" ++"Atomic operations are not supported on your platform" + + // ThreadSanitizer, http://clang.llvm.org/docs/ThreadSanitizer.html. + #if defined(THREAD_SANITIZER) +@@ -172,7 +172,7 @@ Atomic64 Release_Load(volatile const Atomic64* ptr); + #if defined(GOOGLE_PROTOBUF_ARCH_IA32) || defined(GOOGLE_PROTOBUF_ARCH_X64) + #include <google/protobuf/stubs/atomicops_internals_x86_msvc.h> + #else +-GOOGLE_PROTOBUF_ATOMICOPS_ERROR ++#error GOOGLE_PROTOBUF_ATOMICOPS_ERROR + #endif + + // Solaris +@@ -203,15 +203,15 @@ GOOGLE_PROTOBUF_ATOMICOPS_ERROR + #if __has_extension(c_atomic) + #include <google/protobuf/stubs/atomicops_internals_generic_gcc.h> + #else +-GOOGLE_PROTOBUF_ATOMICOPS_ERROR ++#error GOOGLE_PROTOBUF_ATOMICOPS_ERROR + #endif + #else +-GOOGLE_PROTOBUF_ATOMICOPS_ERROR ++#error GOOGLE_PROTOBUF_ATOMICOPS_ERROR + #endif + + // Unknown. + #else +-GOOGLE_PROTOBUF_ATOMICOPS_ERROR ++#error GOOGLE_PROTOBUF_ATOMICOPS_ERROR + #endif + + // On some platforms we need additional declarations to make AtomicWord +-- +1.9.1 + diff --git a/package/protobuf/Config.in b/package/protobuf/Config.in index 9ee7e7d..3899ac1 100644 --- a/package/protobuf/Config.in +++ b/package/protobuf/Config.in @@ -3,8 +3,7 @@ config BR2_PACKAGE_PROTOBUF depends on BR2_INSTALL_LIBSTDCPP depends on BR2_TOOLCHAIN_HAS_THREADS # See src/google/protobuf/stubs/platform_macros.h for supported archs. - # PowerPC doesn't actually work. - depends on BR2_arm || BR2_i386 || BR2_mipsel || BR2_x86_64 + depends on BR2_arm || BR2_i386 || BR2_mipsel || BR2_x86_64 || BR2_powerpc # host-protobuf only builds on certain architectures depends on BR2_HOSTARCH = "x86_64" || BR2_HOSTARCH = "x86" depends on !BR2_STATIC_LIBS @@ -17,5 +16,5 @@ config BR2_PACKAGE_PROTOBUF comment "protobuf needs a toolchain w/ C++, threads, dynamic library" depends on !BR2_INSTALL_LIBSTDCPP || !BR2_TOOLCHAIN_HAS_THREADS \ || BR2_STATIC_LIBS - depends on BR2_arm || BR2_i386 || BR2_mipsel || BR2_x86_64 + depends on BR2_arm || BR2_i386 || BR2_mipsel || BR2_x86_64 || BR2_powerpc depends on BR2_HOSTARCH = "x86_64" || BR2_HOSTARCH = "x86"