include/boilerplate/compiler.h breaks clang 3.9's arm_acle.h

include/boilerplate/compiler.h breaks clang 3.9's arm_acle.h

[Giulio Moro]

I have clang 3.9.1 (on Debian Stretch) on armv7.

the below minimal program clz-test.c:

```
#include <stdlib.h>
#include <arm_acle.h>

int main()
{
    return __clz(0);
}
```

will fail to build when compiled with 

`clang -I/usr/xenomai/include/cobalt -I/usr/xenomai/include clz-test.c` , as follows:
```
In file included from test-clz.c:2:
/usr/lib/llvm-3.9/bin/../lib/clang/3.9.1/include/arm_acle.h:142:1: error: expected identifier or '('
__clz(uint32_t __t) {
^
/usr/xenomai/include/boilerplate/compiler.h:87:3: note: expanded from macro '__clz'
        ({                                                              \
         ^
In file included from test-clz.c:2:
/usr/lib/llvm-3.9/bin/../lib/clang/3.9.1/include/arm_acle.h:142:1: error: expected ')'
/usr/xenomai/include/boilerplate/compiler.h:87:3: note: expanded from macro '__clz'
        ({                                                              \
         ^
/usr/lib/llvm-3.9/bin/../lib/clang/3.9.1/include/arm_acle.h:142:1: note: to match this '('
/usr/xenomai/include/boilerplate/compiler.h:87:2: note: expanded from macro '__clz'
        ({                                                              \
        ^
2 errors generated.
```

(compiler.h is included indirectly by `stdlib.h`)

This is because clang provides a definition of __clz as an inline function in /usr/lib/llvm-3.9/lib/clang/3.9.1/include/arm_acle.h :

static __inline__ uint32_t __attribute__((__always_inline__, __nodebug__))
__clz(uint32_t __t) {
  return __builtin_clz(__t);
}

however, xenomai-3/include/boilerplate/compiler.h defines __clz() as a macro, hence the issue.

Note that compiler.h similarly defines a macro for __ctz , but that is not defined in clang's arm_acle.h, so no issue with that one.

The patch below addresses the issue, leveraging clang's `__has_builtin()` feature, while keeping things the same for gcc:

diff --git a/include/boilerplate/compiler.h b/include/boilerplate/compiler.h
index bcef7d4cc..8697a24b0 100644
--- a/include/boilerplate/compiler.h
+++ b/include/boilerplate/compiler.h
@@ -83,6 +83,15 @@ void __invalid_operand_size(void);
                __ret;                                                  \
        })

+#ifdef __clang__
+#if __has_builtin(__builtin_clz)
+#define DONT_DEF_CLZ
+#endif /* has_builtin */
+#endif /* __clang_ */
+
+#ifdef DONT_DEF_CLZ
+#undef DONT_DEF_CLZ
+#else /* DONT_DEF_CLZ */
 #define __clz(__v)                                                     \
        ({                                                              \
                int __ret;                                              \
@@ -101,6 +110,7 @@ void __invalid_operand_size(void);
                        }                                               \
                __ret;                                                  \
        })
+#endif /* DONT_DEF_CLZ */

 #ifdef __cplusplus
 }

Re: include/boilerplate/compiler.h breaks clang 3.9's arm_acle.h

[Jan Kiszka]

On 17.11.18 01:38, Giulio Moro via Xenomai wrote:
> I have clang 3.9.1 (on Debian Stretch) on armv7.
> 
> the below minimal program clz-test.c:
> 
> ```
> #include <stdlib.h>
> #include <arm_acle.h>
> 
> int main()
> {
>      return __clz(0);
> }
> ```
> 
> will fail to build when compiled with
> 
> `clang -I/usr/xenomai/include/cobalt -I/usr/xenomai/include clz-test.c` , as follows:
> ```
> In file included from test-clz.c:2:
> /usr/lib/llvm-3.9/bin/../lib/clang/3.9.1/include/arm_acle.h:142:1: error: expected identifier or '('
> __clz(uint32_t __t) {
> ^
> /usr/xenomai/include/boilerplate/compiler.h:87:3: note: expanded from macro '__clz'
>          ({                                                              \
>           ^
> In file included from test-clz.c:2:
> /usr/lib/llvm-3.9/bin/../lib/clang/3.9.1/include/arm_acle.h:142:1: error: expected ')'
> /usr/xenomai/include/boilerplate/compiler.h:87:3: note: expanded from macro '__clz'
>          ({                                                              \
>           ^
> /usr/lib/llvm-3.9/bin/../lib/clang/3.9.1/include/arm_acle.h:142:1: note: to match this '('
> /usr/xenomai/include/boilerplate/compiler.h:87:2: note: expanded from macro '__clz'
>          ({                                                              \
>          ^
> 2 errors generated.
> ```
> 
> (compiler.h is included indirectly by `stdlib.h`)
> 
> This is because clang provides a definition of __clz as an inline function in /usr/lib/llvm-3.9/lib/clang/3.9.1/include/arm_acle.h :
> 
> static __inline__ uint32_t __attribute__((__always_inline__, __nodebug__))
> __clz(uint32_t __t) {
>    return __builtin_clz(__t);
> }
> 
> however, xenomai-3/include/boilerplate/compiler.h defines __clz() as a macro, hence the issue.
> 
> Note that compiler.h similarly defines a macro for __ctz , but that is not defined in clang's arm_acle.h, so no issue with that one.
> 
> The patch below addresses the issue, leveraging clang's `__has_builtin()` feature, while keeping things the same for gcc:
> 
> diff --git a/include/boilerplate/compiler.h b/include/boilerplate/compiler.h
> index bcef7d4cc..8697a24b0 100644
> --- a/include/boilerplate/compiler.h
> +++ b/include/boilerplate/compiler.h
> @@ -83,6 +83,15 @@ void __invalid_operand_size(void);
>                  __ret;                                                  \
>          })
> 
> +#ifdef __clang__
> +#if __has_builtin(__builtin_clz)
> +#define DONT_DEF_CLZ
> +#endif /* has_builtin */
> +#endif /* __clang_ */
> +
> +#ifdef DONT_DEF_CLZ
> +#undef DONT_DEF_CLZ
> +#else /* DONT_DEF_CLZ */
>   #define __clz(__v)                                                     \
>          ({                                                              \
>                  int __ret;                                              \
> @@ -101,6 +110,7 @@ void __invalid_operand_size(void);
>                          }                                               \
>                  __ret;                                                  \
>          })
> +#endif /* DONT_DEF_CLZ */
> 
>   #ifdef __cplusplus
>   }
> 
> 

We should do

#define __clz	__clz

in the clang case and then

#ifndef __clz

That should save some boilerplate code. Could you send a proper patch (see 
CONTRIBUTING.md)?

Thanks!
Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: include/boilerplate/compiler.h breaks clang 3.9's arm_acle.h

[Giulio Moro]

right, digging deeper into the issue, ARM says that if the __ARM_ACLE macro is defined by the compiler, then it's safer to include `arm_acle.h`, which will define __clz() as a function, so a better approach could be perhaps:

#ifdef __ARM_ACLE
#include <arm_acle.h>
#else
#define __clz(__v)    \
      // .... currently defined code
#endif

Happy to prepare a patch for the above. On the other hand, I am not sure what the current purpose of this definition of __clz() in a public header file is: 

$ grep -RI __clz .
./lib/copperplate/heapobj-pshared.c:		log2size = sizeof(size) * 8 - 1 - __clz(size);
./include/boilerplate/compiler.h:#define __clz(__v)

so it's used only in lib/copperplate/heapobj-pshared.c (similarly for __ctz()) . Even if they was used in multiple places, why should they be defined in a public header, risking to cause issues when user code includes stdlib.h?

It seems to me that we can safely remove 
#include <boilerplate/compiler.h>

from include/cobalt/wrappers.h in order to fix the above issue for the Cobalt API. Possibly something similar can be done for the other APIs?

Giulio

Re: include/boilerplate/compiler.h breaks clang 3.9's arm_acle.h

[Philippe Gerum]

On 11/19/18 12:59 PM, Giulio Moro via Xenomai wrote:
> right, digging deeper into the issue, ARM says that if the __ARM_ACLE macro is defined by the compiler, then it's safer to include `arm_acle.h`, which will define __clz() as a function, so a better approach could be perhaps:
> 
> #ifdef __ARM_ACLE
> #include <arm_acle.h>
> #else
> #define __clz(__v)    \
>       // .... currently defined code
> #endif
> 
> Happy to prepare a patch for the above. On the other hand, I am not sure what the current purpose of this definition of __clz() in a public header file is: 
> 
> $ grep -RI __clz .
> ./lib/copperplate/heapobj-pshared.c:		log2size = sizeof(size) * 8 - 1 - __clz(size);
> ./include/boilerplate/compiler.h:#define __clz(__v)
> 
> so it's used only in lib/copperplate/heapobj-pshared.c (similarly for __ctz()) . Even if they was used in multiple places, why should they be defined in a public header, risking to cause issues when user code includes stdlib.h?
> 
> It seems to me that we can safely remove 
> #include <boilerplate/compiler.h>
> 
> from include/cobalt/wrappers.h in order to fix the above issue for the Cobalt API. Possibly something similar can be done for the other APIs?
> 
> Giulio
> 
> 

Why not solving the namespace clash the simplest way by renaming the
offending stuff in boilerplate/compiler.h? __clz and friends are
definitely not part of the public interface, so you only have to care
about in-tree users. __clz could be renamed as count_leading_zeroes()
without affecting the API.

-- 
Philippe.

Re: include/boilerplate/compiler.h breaks clang 3.9's arm_acle.h

[Giulio Moro]

> Why not solving the namespace clash the simplest way by renaming the
> offending stuff in boilerplate/compiler.h? __clz and friends are
> definitely not part of the public interface, so you only have to care
> about in-tree users. __clz could be renamed as count_leading_zeroes()
> without affecting the API.

If you want to go down this path, I'd go for xenomai_count_leading_zeros(), to give some sort of protection against further namespace conflicts.
However, I don't see why this header, and its defines, should be included with `stdlib.h` (or any other system include, fwiw) at all. Granted, I don't know much about the ins and outs of libcobalt, but, at first sight, there doesn't seem to be anything in copperplate/compiler.h that should be made available to the user.

Best,
Giulio

Re: include/boilerplate/compiler.h breaks clang 3.9's arm_acle.h

[Philippe Gerum]

On 11/20/18 7:05 PM, Giulio Moro wrote:
>> Why not solving the namespace clash the simplest way by renaming the
>> offending stuff in boilerplate/compiler.h? __clz and friends are
>> definitely not part of the public interface, so you only have to care
>> about in-tree users. __clz could be renamed as count_leading_zeroes()
>> without affecting the API.
> 
> If you want to go down this path, I'd go for xenomai_count_leading_zeros(), to give some sort of protection against further namespace conflicts.
> However, I don't see why this header, and its defines, should be included with `stdlib.h` (or any other system include, fwiw) at all. Granted, I don't know much about the ins and outs of libcobalt, but, at first sight, there doesn't seem to be anything in copperplate/compiler.h that should be made available to the user.

container_of() must be.

This said, this is about fixing the whole issue: make the Xenomai stack
(in userland) and applications all buildable with clang. Maybe this
change won't suffice wrt to the former, but at least making the proper
change would be a step forward this goal.

-- 
Philippe.