[Buildroot] [PATCH 03/21] toolchain: add BR2_TOOLCHAIN_HAS_SYNC_x hidden booleans

[Thomas Petazzoni <thomas.petazzoni@free-electrons.com>]

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