| Message ID | 20161121234340.30463-2-arnout@mind.be |
|---|---|
| State | Changes Requested |
| Headers | show |
Hello, On Tue, 22 Nov 2016 00:43:40 +0100, Arnout Vandecappelle (Essensium/Mind) wrote: > config BR2_TOOLCHAIN_ARM_HAS_SYNC_8 > bool > default y > depends on BR2_arm || BR2_armeb > - depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_7 > - depends on BR2_TOOLCHAIN_USES_GLIBC || BR2_ARM_CPU_ARMV7A > + depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 Can we make this instead: depends on (BR2_TOOLCHAIN_GCC_AT_LEAST_4_7 && BR2_TOOLCHAIN_USES_GLIBC) || BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 This way, glibc toolchains (which were never affected by the _write issue) still expose the fact that they support the __sync_8 thing. Otherwise, your change introduces a regression: a 4.7 glibc external toolchain was prior to your patch exposing the fact that it supports 8-byte sync, but no longer after your patch. Also, what about the __atomic 8 bytes? As we discussed yesterday, it's in fact affected by the same issue. How do we deal with that? Do we introduce per-size BR2_TOOLCHAIN_HAS_ATOMIC_<size> options? Best regards, Thomas
On 22-11-16 09:35, Thomas Petazzoni wrote: > Hello, > > On Tue, 22 Nov 2016 00:43:40 +0100, Arnout Vandecappelle > (Essensium/Mind) wrote: > >> config BR2_TOOLCHAIN_ARM_HAS_SYNC_8 >> bool >> default y >> depends on BR2_arm || BR2_armeb >> - depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_7 >> - depends on BR2_TOOLCHAIN_USES_GLIBC || BR2_ARM_CPU_ARMV7A >> + depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 > > Can we make this instead: > > depends on (BR2_TOOLCHAIN_GCC_AT_LEAST_4_7 && BR2_TOOLCHAIN_USES_GLIBC) || BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 > > This way, glibc toolchains (which were never affected by the _write > issue) still expose the fact that they support the __sync_8 thing. > Otherwise, your change introduces a regression: a 4.7 glibc external > toolchain was prior to your patch exposing the fact that it supports > 8-byte sync, but no longer after your patch.not-quite Now I think about it again, I think the condition should simply be depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_7 Let me explain again my reasoning. For me, the toolchain conditions that we add to hide packages are only there to cover missing features in certain compiler versions, not to cover bugs. The incorrect sync_8 implementation in gcc < 5.2 is something I consider a bug: it's something that has been implemented, works correctly, but fails to link with a specific library. Therefore, I would say that sync_8 on ARM has been available since gcc 4.7. An external non-glibc toolchain with gcc < 5.2 will not actually work, but that's due to a compiler bug. People who need one of the 3 packages that fail because of this compiler bug should update their compiler - and the best way to tell them that is to make the build fail, not to hide the option. But that's my opinion. It's clearly not the rule that is currently followed in Buildroot. Just look at the number of packages with some kind of external toolchain specific dependency. Honestly, I think those things are giving us more work for no added benefit to the user. > Also, what about the __atomic 8 bytes? As we discussed yesterday, it's > in fact affected by the same issue. How do we deal with that? Do we > introduce per-size BR2_TOOLCHAIN_HAS_ATOMIC_<size> options? If the condition for SYNC_8 is simply BR2_TOOLCHAIN_GCC_AT_LEAST_4_7, then HAS_ATOMIC is already OK. Regards, Arnout > > Best regards, > > Thomas >
Hello, On Fri, 25 Nov 2016 00:13:16 +0100, Arnout Vandecappelle wrote: > Now I think about it again, I think the condition should simply be > depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_7 > > Let me explain again my reasoning. > > For me, the toolchain conditions that we add to hide packages are only there to > cover missing features in certain compiler versions, not to cover bugs. The > incorrect sync_8 implementation in gcc < 5.2 is something I consider a bug: it's > something that has been implemented, works correctly, but fails to link with a > specific library. Therefore, I would say that sync_8 on ARM has been available > since gcc 4.7. An external non-glibc toolchain with gcc < 5.2 will not actually > work, but that's due to a compiler bug. People who need one of the 3 packages > that fail because of this compiler bug should update their compiler - and the > best way to tell them that is to make the build fail, not to hide the option. > > But that's my opinion. It's clearly not the rule that is currently followed in > Buildroot. Just look at the number of packages with some kind of external > toolchain specific dependency. Honestly, I think those things are giving us more > work for no added benefit to the user. That's fine with me, with just one question: how do we handle this from an autobuilder perspective? Add more and more exceptions? Doesn't it make more sense to have those exceptions in Buildroot itself (in the form of Config.in dependencies) so that instead of benefiting the autobuilders only, they also benefit to the user? Or do we want our users to run into build failures, to have them discover the hard way that toolchain X or Y is affected by gcc bug XYZ ? Why not instead annotate packages like we do today, to make the life our users easier ? Thomas
Hello, On Fri, 25 Nov 2016 00:13:16 +0100, Arnout Vandecappelle wrote: > > On Tue, 22 Nov 2016 00:43:40 +0100, Arnout Vandecappelle > > (Essensium/Mind) wrote: > > > >> config BR2_TOOLCHAIN_ARM_HAS_SYNC_8 > >> bool > >> default y > >> depends on BR2_arm || BR2_armeb > >> - depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_7 > >> - depends on BR2_TOOLCHAIN_USES_GLIBC || BR2_ARM_CPU_ARMV7A > >> + depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 > > > > Can we make this instead: > > > > depends on (BR2_TOOLCHAIN_GCC_AT_LEAST_4_7 && BR2_TOOLCHAIN_USES_GLIBC) || BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 > > > > This way, glibc toolchains (which were never affected by the _write > > issue) still expose the fact that they support the __sync_8 thing. > > Otherwise, your change introduces a regression: a 4.7 glibc external > > toolchain was prior to your patch exposing the fact that it supports > > 8-byte sync, but no longer after your patch.not-quite > > Now I think about it again, I think the condition should simply be > depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_7 Could you submit a new version that does this? I'll mark your patch as Changes Requested for the time being. Thanks! Thomas
diff --git a/toolchain/toolchain-common.in b/toolchain/toolchain-common.in index bf9dc7b..4699e8d 100644 --- a/toolchain/toolchain-common.in +++ b/toolchain/toolchain-common.in @@ -332,29 +332,19 @@ config BR2_TOOLCHAIN_HAS_SYNC_4 depends on !BR2_sparc depends on !(BR2_arc && !BR2_ARC_ATOMIC_EXT) -# The availability of __sync for 8-byte types on ARM is somewhat -# complicated: -# -# - It appeared in gcc starting with gcc 4.7. -# -# - On ARMv7, there is no problem, it can be directly implemented in -# userspace. -# -# - On < ARMv7, it requires help from the kernel. Unfortunately, the -# libgcc code implementing 8-byte __sync with the help from the -# kernel calls __write() when a failure occurs, which is a function -# internal to glibc, not available in uClibc and musl. This means -# that the 8-byte __sync operations are not available on < ARMv7 -# with uClibc and musl. This problem was fixed as part of gcc -# PR68059, which was backported to the gcc 5 branch, but isn't yet -# part of any gcc 5.x release. +# __sync for 8-byte types on ARM appeared in gcc starting with gcc 4.7. +# However, this implementation called the glibc-internal __write() function +# for < ARMv7, which caused link failures with uClibc and musl. This problem +# was fixed as part of gcc PR68059, which was backported to gcc 5.2.0. +# In the internal toolchain we also backported to gcc 4.8 and 4.9. To simplify +# things, and since most external toolchains are glibc, we assume the 8-byte +# __sync type is available for gcc >= 4.8. # config BR2_TOOLCHAIN_ARM_HAS_SYNC_8 bool default y depends on BR2_arm || BR2_armeb - depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_7 - depends on BR2_TOOLCHAIN_USES_GLIBC || BR2_ARM_CPU_ARMV7A + depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 # 8-byte intrinsics available on most x86 CPUs, except a few old ones config BR2_TOOLCHAIN_X86_HAS_SYNC_8
Since we patch the internal toolchain to remove the call to __write in the 8-byte __sync implementation, this now also works for uClibc and musl in gcc 4.8 and 4.9. gcc 5 and 6 were already working, but the condition on BR2_TOOLCHAIN_ARM_HAS_SYNC_8 hadn't been updated. We can simplify the condition quite a bit by just requiring gcc 4.8+. This means that gcc 4.7 + (glibc or ARMv7) is no longer considered to have 8-byte __sync, but it's not possible to build an internal toolchain with gcc 4.7, and there is no common external toolchain that uses gcc 4.7 for ARM. In addition, very few packages depend on BR2_TOOLCHAIN_ARM_HAS_SYNC_8 so nobody will miss it. Note that we assume that external toolchains with gcc 4.8 and 4.9 have been fixed. Since most external toolchains use glibc, and the musl external toolchain uses gcc 5.3.0, this is a relatively safe assumption. And again, very few packages are affected. Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> --- I guess this change is for next, not for master. But it depends on patch 1/2 (though not in the sense of causing a merge conflict). Perhaps it's best to leave it lingering in patchwork until after the release of 2016.11, or after a master->next merge. I build-tested kodi on a uClibc arm926t with the gcc patch applied. --- toolchain/toolchain-common.in | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)