diff mbox series

eal: fix build of external apps with clang on armv8

Message ID 20190114161442.8277-1-i.maximets@samsung.com (mailing list archive)
State Accepted, archived
Headers
Series eal: fix build of external apps with clang on armv8 |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Ilya Maximets Jan. 14, 2019, 4:14 p.m. UTC 
  In case DPDK built using GCC, RTE_TOOLCHAIN_CLANG is not defined.
But 'rte_atomic.h' is a generic header that included to the
external apps like OVS while building with DPDK. As a result,
clang build of OVS fails on ARMv8 if DPDK built using gcc:

    include/generic/rte_atomic.h:215:9: error:
            implicit declaration of function '__atomic_exchange_2'
            is invalid in C99
    include/generic/rte_atomic.h:494:9: error:
            implicit declaration of function '__atomic_exchange_4'
            is invalid in C99
    include/generic/rte_atomic.h:772:9: error:
            implicit declaration of function '__atomic_exchange_8'
            is invalid in C99

We need to check for current compiler, not the compiler used for
DPDK build.

Fixes: 7bdccb93078e ("eal: fix ARM build with clang")
Cc: stable@dpdk.org

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 lib/librte_eal/common/include/generic/rte_atomic.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Thomas Monjalon Jan. 14, 2019, 4:27 p.m. UTC | #1 
14/01/2019 17:14, Ilya Maximets:
> In case DPDK built using GCC, RTE_TOOLCHAIN_CLANG is not defined.
> But 'rte_atomic.h' is a generic header that included to the
> external apps like OVS while building with DPDK. As a result,
> clang build of OVS fails on ARMv8 if DPDK built using gcc:
> 
>     include/generic/rte_atomic.h:215:9: error:
>             implicit declaration of function '__atomic_exchange_2'
>             is invalid in C99
>     include/generic/rte_atomic.h:494:9: error:
>             implicit declaration of function '__atomic_exchange_4'
>             is invalid in C99
>     include/generic/rte_atomic.h:772:9: error:
>             implicit declaration of function '__atomic_exchange_8'
>             is invalid in C99
> 
> We need to check for current compiler, not the compiler used for
> DPDK build.

Right, API cannot rely on internal build system configuration.

> Fixes: 7bdccb93078e ("eal: fix ARM build with clang")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>

Acked-by: Thomas Monjalon <thomas@monjalon.net>
Bruce Richardson Jan. 14, 2019, 4:46 p.m. UTC | #2 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ilya Maximets
> Sent: Monday, January 14, 2019 4:15 PM
> To: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>
> Cc: Pavan Nikhilesh <pbhagavatula@marvell.com>; Ilya Maximets
> <i.maximets@samsung.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH] eal: fix build of external apps with clang on
> armv8
> 
> In case DPDK built using GCC, RTE_TOOLCHAIN_CLANG is not defined.
> But 'rte_atomic.h' is a generic header that included to the external apps
> like OVS while building with DPDK. As a result, clang build of OVS fails
> on ARMv8 if DPDK built using gcc:
> 
>     include/generic/rte_atomic.h:215:9: error:
>             implicit declaration of function '__atomic_exchange_2'
>             is invalid in C99
>     include/generic/rte_atomic.h:494:9: error:
>             implicit declaration of function '__atomic_exchange_4'
>             is invalid in C99
>     include/generic/rte_atomic.h:772:9: error:
>             implicit declaration of function '__atomic_exchange_8'
>             is invalid in C99
> 
> We need to check for current compiler, not the compiler used for DPDK
> build.
> 
> Fixes: 7bdccb93078e ("eal: fix ARM build with clang")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
>  lib/librte_eal/common/include/generic/rte_atomic.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/generic/rte_atomic.h
> b/lib/librte_eal/common/include/generic/rte_atomic.h
> index b99ba4688..d0c464fb1 100644
> --- a/lib/librte_eal/common/include/generic/rte_atomic.h
> +++ b/lib/librte_eal/common/include/generic/rte_atomic.h
> @@ -212,7 +212,7 @@ rte_atomic16_exchange(volatile uint16_t *dst, uint16_t
> val);  static inline uint16_t  rte_atomic16_exchange(volatile uint16_t
> *dst, uint16_t val)  { -#if defined(RTE_ARCH_ARM64) &&
> defined(RTE_TOOLCHAIN_CLANG)
> +#if defined(RTE_ARCH_ARM64) && defined(__clang__)
>  	return __atomic_exchange_n(dst, val, __ATOMIC_SEQ_CST);  #else
>  	return __atomic_exchange_2(dst, val, __ATOMIC_SEQ_CST); @@ -495,7
> +495,7 @@ rte_atomic32_exchange(volatile uint32_t *dst, uint32_t val);
> static inline uint32_t  rte_atomic32_exchange(volatile uint32_t *dst,
> uint32_t val)  { -#if defined(RTE_ARCH_ARM64) &&
> defined(RTE_TOOLCHAIN_CLANG)
> +#if defined(RTE_ARCH_ARM64) && defined(__clang__)
>  	return __atomic_exchange_n(dst, val, __ATOMIC_SEQ_CST);  #else
>  	return __atomic_exchange_4(dst, val, __ATOMIC_SEQ_CST); @@ -777,7
> +777,7 @@ rte_atomic64_exchange(volatile uint64_t *dst, uint64_t val);
> static inline uint64_t  rte_atomic64_exchange(volatile uint64_t *dst,
> uint64_t val)  { -#if defined(RTE_ARCH_ARM64) &&
> defined(RTE_TOOLCHAIN_CLANG)
> +#if defined(RTE_ARCH_ARM64) && defined(__clang__)
>  	return __atomic_exchange_n(dst, val, __ATOMIC_SEQ_CST);  #else
>  	return __atomic_exchange_8(dst, val, __ATOMIC_SEQ_CST);
> --
> 2.17.1

Is this really architecture-specific? Would the same issue not occur on e.g. x86 or PPC?
Honnappa Nagarahalli Jan. 14, 2019, 4:50 p.m. UTC | #3 
> 
> In case DPDK built using GCC, RTE_TOOLCHAIN_CLANG is not defined.
> But 'rte_atomic.h' is a generic header that included to the external apps like
> OVS while building with DPDK. As a result, clang build of OVS fails on ARMv8
                                                                                                                               ^^^^^^
Typo, should be armv8.
Commit '40fd87486799' should have caught the typo issue. I don't want to do this for every patch 😊 Does 'check-git-log.sh' run automatically?

> if DPDK built using gcc:
> 
>     include/generic/rte_atomic.h:215:9: error:
>             implicit declaration of function '__atomic_exchange_2'
>             is invalid in C99
>     include/generic/rte_atomic.h:494:9: error:
>             implicit declaration of function '__atomic_exchange_4'
>             is invalid in C99
>     include/generic/rte_atomic.h:772:9: error:
>             implicit declaration of function '__atomic_exchange_8'
>             is invalid in C99
> 
> We need to check for current compiler, not the compiler used for DPDK build.
> 
> Fixes: 7bdccb93078e ("eal: fix ARM build with clang")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
>  lib/librte_eal/common/include/generic/rte_atomic.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/generic/rte_atomic.h
> b/lib/librte_eal/common/include/generic/rte_atomic.h
> index b99ba4688..d0c464fb1 100644
> --- a/lib/librte_eal/common/include/generic/rte_atomic.h
> +++ b/lib/librte_eal/common/include/generic/rte_atomic.h
> @@ -212,7 +212,7 @@ rte_atomic16_exchange(volatile uint16_t *dst,
> uint16_t val);  static inline uint16_t  rte_atomic16_exchange(volatile uint16_t
> *dst, uint16_t val)  { -#if defined(RTE_ARCH_ARM64) &&
> defined(RTE_TOOLCHAIN_CLANG)
> +#if defined(RTE_ARCH_ARM64) && defined(__clang__)
>  	return __atomic_exchange_n(dst, val, __ATOMIC_SEQ_CST);  #else
>  	return __atomic_exchange_2(dst, val, __ATOMIC_SEQ_CST); @@ -
> 495,7 +495,7 @@ rte_atomic32_exchange(volatile uint32_t *dst, uint32_t
> val);  static inline uint32_t  rte_atomic32_exchange(volatile uint32_t *dst,
> uint32_t val)  { -#if defined(RTE_ARCH_ARM64) &&
> defined(RTE_TOOLCHAIN_CLANG)
> +#if defined(RTE_ARCH_ARM64) && defined(__clang__)
>  	return __atomic_exchange_n(dst, val, __ATOMIC_SEQ_CST);  #else
>  	return __atomic_exchange_4(dst, val, __ATOMIC_SEQ_CST); @@ -
> 777,7 +777,7 @@ rte_atomic64_exchange(volatile uint64_t *dst, uint64_t
> val);  static inline uint64_t  rte_atomic64_exchange(volatile uint64_t *dst,
> uint64_t val)  { -#if defined(RTE_ARCH_ARM64) &&
> defined(RTE_TOOLCHAIN_CLANG)
> +#if defined(RTE_ARCH_ARM64) && defined(__clang__)
>  	return __atomic_exchange_n(dst, val, __ATOMIC_SEQ_CST);  #else
>  	return __atomic_exchange_8(dst, val, __ATOMIC_SEQ_CST);
> --
> 2.17.1
Thomas Monjalon Jan. 14, 2019, 6:47 p.m. UTC | #4 
14/01/2019 17:50, Honnappa Nagarahalli:
> > 
> > In case DPDK built using GCC, RTE_TOOLCHAIN_CLANG is not defined.
> > But 'rte_atomic.h' is a generic header that included to the external apps like
> > OVS while building with DPDK. As a result, clang build of OVS fails on ARMv8
>                                                                                                                                ^^^^^^
> Typo, should be armv8.
> Commit '40fd87486799' should have caught the typo issue. I don't want to do this for every patch 😊 Does 'check-git-log.sh' run automatically?

We are supposed to run it before submitting.
And tree maintainers run it before applying.

Anyway, it does check the title but not the message body.
Thomas Monjalon Jan. 14, 2019, 6:49 p.m. UTC | #5 
14/01/2019 17:27, Thomas Monjalon:
> 14/01/2019 17:14, Ilya Maximets:
> > In case DPDK built using GCC, RTE_TOOLCHAIN_CLANG is not defined.
> > But 'rte_atomic.h' is a generic header that included to the
> > external apps like OVS while building with DPDK. As a result,
> > clang build of OVS fails on ARMv8 if DPDK built using gcc:
> > 
> >     include/generic/rte_atomic.h:215:9: error:
> >             implicit declaration of function '__atomic_exchange_2'
> >             is invalid in C99
> >     include/generic/rte_atomic.h:494:9: error:
> >             implicit declaration of function '__atomic_exchange_4'
> >             is invalid in C99
> >     include/generic/rte_atomic.h:772:9: error:
> >             implicit declaration of function '__atomic_exchange_8'
> >             is invalid in C99
> > 
> > We need to check for current compiler, not the compiler used for
> > DPDK build.
> 
> Right, API cannot rely on internal build system configuration.
> 
> > Fixes: 7bdccb93078e ("eal: fix ARM build with clang")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> 
> Acked-by: Thomas Monjalon <thomas@monjalon.net>

Applied (if armv8 typo), thanks
Thomas Monjalon Jan. 14, 2019, 7:06 p.m. UTC | #6 
14/01/2019 19:49, Thomas Monjalon:
> 14/01/2019 17:27, Thomas Monjalon:
> > 14/01/2019 17:14, Ilya Maximets:
> > > In case DPDK built using GCC, RTE_TOOLCHAIN_CLANG is not defined.
> > > But 'rte_atomic.h' is a generic header that included to the
> > > external apps like OVS while building with DPDK. As a result,
> > > clang build of OVS fails on ARMv8 if DPDK built using gcc:
> > > 
> > >     include/generic/rte_atomic.h:215:9: error:
> > >             implicit declaration of function '__atomic_exchange_2'
> > >             is invalid in C99
> > >     include/generic/rte_atomic.h:494:9: error:
> > >             implicit declaration of function '__atomic_exchange_4'
> > >             is invalid in C99
> > >     include/generic/rte_atomic.h:772:9: error:
> > >             implicit declaration of function '__atomic_exchange_8'
> > >             is invalid in C99
> > > 
> > > We need to check for current compiler, not the compiler used for
> > > DPDK build.
> > 
> > Right, API cannot rely on internal build system configuration.
> > 
> > > Fixes: 7bdccb93078e ("eal: fix ARM build with clang")
> > > Cc: stable@dpdk.org
> > > 
> > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> > 
> > Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 
> Applied (if armv8 typo), thanks

correction: with armv8 typo fixed
Ilya Maximets Jan. 15, 2019, 11:32 a.m. UTC | #7 
On 14.01.2019 19:46, Richardson, Bruce wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ilya Maximets
>> Sent: Monday, January 14, 2019 4:15 PM
>> To: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>
>> Cc: Pavan Nikhilesh <pbhagavatula@marvell.com>; Ilya Maximets
>> <i.maximets@samsung.com>; stable@dpdk.org
>> Subject: [dpdk-dev] [PATCH] eal: fix build of external apps with clang on
>> armv8
>>
>> In case DPDK built using GCC, RTE_TOOLCHAIN_CLANG is not defined.
>> But 'rte_atomic.h' is a generic header that included to the external apps
>> like OVS while building with DPDK. As a result, clang build of OVS fails
>> on ARMv8 if DPDK built using gcc:
>>
>>     include/generic/rte_atomic.h:215:9: error:
>>             implicit declaration of function '__atomic_exchange_2'
>>             is invalid in C99
>>     include/generic/rte_atomic.h:494:9: error:
>>             implicit declaration of function '__atomic_exchange_4'
>>             is invalid in C99
>>     include/generic/rte_atomic.h:772:9: error:
>>             implicit declaration of function '__atomic_exchange_8'
>>             is invalid in C99
>>
>> We need to check for current compiler, not the compiler used for DPDK
>> build.
>>
>> Fixes: 7bdccb93078e ("eal: fix ARM build with clang")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
>>  lib/librte_eal/common/include/generic/rte_atomic.h | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/include/generic/rte_atomic.h
>> b/lib/librte_eal/common/include/generic/rte_atomic.h
>> index b99ba4688..d0c464fb1 100644
>> --- a/lib/librte_eal/common/include/generic/rte_atomic.h
>> +++ b/lib/librte_eal/common/include/generic/rte_atomic.h
>> @@ -212,7 +212,7 @@ rte_atomic16_exchange(volatile uint16_t *dst, uint16_t
>> val);  static inline uint16_t  rte_atomic16_exchange(volatile uint16_t
>> *dst, uint16_t val)  { -#if defined(RTE_ARCH_ARM64) &&
>> defined(RTE_TOOLCHAIN_CLANG)
>> +#if defined(RTE_ARCH_ARM64) && defined(__clang__)
>>  	return __atomic_exchange_n(dst, val, __ATOMIC_SEQ_CST);  #else
>>  	return __atomic_exchange_2(dst, val, __ATOMIC_SEQ_CST); @@ -495,7
>> +495,7 @@ rte_atomic32_exchange(volatile uint32_t *dst, uint32_t val);
>> static inline uint32_t  rte_atomic32_exchange(volatile uint32_t *dst,
>> uint32_t val)  { -#if defined(RTE_ARCH_ARM64) &&
>> defined(RTE_TOOLCHAIN_CLANG)
>> +#if defined(RTE_ARCH_ARM64) && defined(__clang__)
>>  	return __atomic_exchange_n(dst, val, __ATOMIC_SEQ_CST);  #else
>>  	return __atomic_exchange_4(dst, val, __ATOMIC_SEQ_CST); @@ -777,7
>> +777,7 @@ rte_atomic64_exchange(volatile uint64_t *dst, uint64_t val);
>> static inline uint64_t  rte_atomic64_exchange(volatile uint64_t *dst,
>> uint64_t val)  { -#if defined(RTE_ARCH_ARM64) &&
>> defined(RTE_TOOLCHAIN_CLANG)
>> +#if defined(RTE_ARCH_ARM64) && defined(__clang__)
>>  	return __atomic_exchange_n(dst, val, __ATOMIC_SEQ_CST);  #else
>>  	return __atomic_exchange_8(dst, val, __ATOMIC_SEQ_CST);
>> --
>> 2.17.1
> 
> Is this really architecture-specific? Would the same issue not occur on e.g. x86 or PPC?
> 

Yes. I looked a bit deeper and found that x86 build is also broken.
We have not encountered this problem earlier on other platforms because
CONFIG_RTE_FORCE_INTRINSICS enabled by default only for armv8.

I've sent a patch:
  http://patches.dpdk.org/patch/49829/

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/librte_eal/common/include/generic/rte_atomic.h b/lib/librte_eal/common/include/generic/rte_atomic.h
index b99ba4688..d0c464fb1 100644
--- a/lib/librte_eal/common/include/generic/rte_atomic.h
+++ b/lib/librte_eal/common/include/generic/rte_atomic.h
@@ -212,7 +212,7 @@  rte_atomic16_exchange(volatile uint16_t *dst, uint16_t val);
 static inline uint16_t
 rte_atomic16_exchange(volatile uint16_t *dst, uint16_t val)
 {
-#if defined(RTE_ARCH_ARM64) && defined(RTE_TOOLCHAIN_CLANG)
+#if defined(RTE_ARCH_ARM64) && defined(__clang__)
 	return __atomic_exchange_n(dst, val, __ATOMIC_SEQ_CST);
 #else
 	return __atomic_exchange_2(dst, val, __ATOMIC_SEQ_CST);
@@ -495,7 +495,7 @@  rte_atomic32_exchange(volatile uint32_t *dst, uint32_t val);
 static inline uint32_t
 rte_atomic32_exchange(volatile uint32_t *dst, uint32_t val)
 {
-#if defined(RTE_ARCH_ARM64) && defined(RTE_TOOLCHAIN_CLANG)
+#if defined(RTE_ARCH_ARM64) && defined(__clang__)
 	return __atomic_exchange_n(dst, val, __ATOMIC_SEQ_CST);
 #else
 	return __atomic_exchange_4(dst, val, __ATOMIC_SEQ_CST);
@@ -777,7 +777,7 @@  rte_atomic64_exchange(volatile uint64_t *dst, uint64_t val);
 static inline uint64_t
 rte_atomic64_exchange(volatile uint64_t *dst, uint64_t val)
 {
-#if defined(RTE_ARCH_ARM64) && defined(RTE_TOOLCHAIN_CLANG)
+#if defined(RTE_ARCH_ARM64) && defined(__clang__)
 	return __atomic_exchange_n(dst, val, __ATOMIC_SEQ_CST);
 #else
 	return __atomic_exchange_8(dst, val, __ATOMIC_SEQ_CST);