[Buildroot] [PATCH 03/21] toolchain: add BR2_TOOLCHAIN_HAS_SYNC_x hidden booleans
Thomas Petazzoni
thomas.petazzoni at free-electrons.com
Wed Jan 27 21:46:37 UTC 2016
Yann,
On Mon, 25 Jan 2016 19:27:37 +0100, Yann E. MORIN wrote:
> Thomas, All,
>
> [Warning: pedantic-english mode engaged. ;-) But with real, interesting
> comments, too! ]
Thanks for the detailed review! I'll skip over the english related
comments (which I've taken into account, of course!) and focus on the
cases where some additional reply is needed.
> > +# - 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 bytes __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 bytes __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.
> > +#
> > +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
>
> I had a bit of difficulty parsing that one. It was not obvious from the
> commit log that gcc >= 4.7 was required, while the comment above makes
> it really explicit. Here's how I understood the commit log:
>
> - for armv7, with gcc-4.7+, it's OK
> - for armv6-, it needs glibc and there is no gcc condition.
>
> But the comment here states it differently, and matches the code.
>
> Can you just confirm that I misunderstand the commit log?
Well, the commit log has:
__sync __atomic gcc
ARM Y Y Y X Y Y Y Y 4.8, 4.7
ARM Y Y Y - 4.5
So, as you can see:
- With a gcc 4.5 toolchain, there is no __sync for 8-byte types at all.
- Starting with gcc 4.7, there *might* be a __sync for 8-byte types,
but only if you're using glibc *or* if you're using ARMv7.
Hence the conditions:
depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_7
depends on BR2_TOOLCHAIN_USES_GLIBC || BR2_ARM_CPU_ARMV7A
So I believe the code, the comment and the commit log are matching
here. Am I missing something ?
> Well, I'd prefer we have a simpler conditional block, maybe something
> like:
>
> config BR2_TOOLCHAIN_i386_HAS_SYNC_8
> bool
> default y
> depends on BR2_i386
> depends on !BR2_x86_i386
> depends on !BR2_x86_i486
> depends on !BR2_x86_c3
> depends on !BR2_x86_winchip_c6
> depends on !BR2_x86_winchip2
>
> config BR2_TOOLCHAIN_HAS_SYNC_8
> bool
> default y if BR2_ARCH_IS_64
> default y if BR2_TOOLCHAIN_i386_HAS_SYNC_8
> default y if BR2_TOOLCHAIN_ARM_HAS_SYNC_8
Adopted.
> Also, I'm not sure about the 'default y if BR2_ARCH_IS_64' and I think
> we should just list those architectures (4 of them: AArch64, mips64,
> sparc64 and x86_64, with that last one already covered with i386).
I beg to disagree here. I don't see why a 64 bits architecture would
not provide the 8-byte intrinsics. I would like to suggest to keep it
as is for now, and revisit when/if we get a 64 bits architecture that
doesn't provide those 8-byte intrinsics.
Thanks!
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
More information about the buildroot
mailing list