[asterisk-dev] [Code Review] 3488: RAII_VAR: nested functions aren't portable. Adapting RAII_VAR to use clang/llvm blocks to get the same/similar functionality.

Sun Jan 25 23:14:26 CST 2015

> On Jan. 25, 2015, 11:25 p.m., Matt Jordan wrote:
> > A few remarks before noting some additional issues:
> > 
> > First, thanks again for providing the patch to get Asterisk to compile under clang. It's definitely a large first step in the right direction.
> > 
> > Second, sorry for not replying again to this issue. Unfortunately, it took me some time to really dig into what the patch here was doing, and then I had to spend some serious time testing this out on my system. In the process of doing so, I discovered a number of other issues, some of which you alluded to on the original ASTERISK issue. While the intent here was simply to get Asterisk closer to compiling under clang, I went a bit further and cleaned up a few other issues in the build system that you had noted.
> > 
> > In addition to the findings below, there are a few other things that I think should be cleaned up with this patch:
> > (1) Our check for trampolines will unfortunately still succeed even when clang is the compiler. This will cause clang to fail to compile under --enable-dev-mode, as it doesn't understand what the 'Wtrampolines' option is for. Making the configure check treat warnings as errors will cause it to fail when clang is used, and allows clang to compile in dev-mode. (Note that it won't get far, as clang's static analysis seems to be much better than gcc's. That's a patch for another day.)
> > (2) As noted on the ASTERISK issue and in the review description, the AST_INLINE_API macro will cause clang to think functions are defined twice. This is due to clang's interpretation of the 'inline' keyword, which follows the C99 standard (see http://clang.llvm.org/compatibility.html#inline). Since clang only views inline as a mild suggestion, I think we should treat the presence of the clang compiler the same as the LOW_MEMORY option, and just let it pick and choose what it wants to inline. That's not too hard to do, and is probably worth doing to make people's lives easier.
> > 
> > Since it's been plenty of time since this review was posted, I've gone ahead and posted an updated review of this patch here with the findings addressed:
> > 
> > https://reviewboard.asterisk.org/r/4370
> > 
> > I'm going to move that we close this review in favour of that one. I'll take ownership of cleaning up any additional findings others may have.

Hi Matt,

Thanks for reopening this case and having an indepth look into the issues. The point about clangs static analysis being better, should be the biggest incentive in getting this project off the ground and eventually finished. 

1. Not completely sure what Wtrampolines does anymore, have to look that one up.
2. I see the point about inlining. I think there might be a way to force them using gcc_inline, which it seems to obey:
    #define forceinline __inline__ __attribute__((always_inline))
    #define ensure_forceinline __attribute__((always_inline)) // inline or die

Would really like to compile asterisk using both compilers in the future. Even if gcc maybe faster because of the inlining, it would be worth it having to compilers to do static analysis at least.

Have you guys been looking at sanitizers like: 
-fsanitize=undefined -fsanitize=shift -fsanitize=integer-divide-by-zero -fsanitize=unreachable -fsanitize=vla-bound  -fsanitize=null -fsanitize=signed-integer-overflow -fsanitize=return
They could be a great help in debunking tiny errors.

(Question why does it take such a long time for reviews to be taken up ?)

- Diederik

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3488/#review14277
-----------------------------------------------------------

On May 15, 2014, 10:57 a.m., Diederik de Groot wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3488/
> -----------------------------------------------------------
> 
> (Updated May 15, 2014, 10:57 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-20850
>     https://issues.asterisk.org/jira/browse/ASTERISK-20850
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> RAII_VAR: nested functions aren't portable. Adapting RAII_VAR to use clang/llvm blocks to get the same/similar functionality.
> Making it possible again to use clang as a compiler, instead of depending on gcc completely.
> 
> Compile instructions:
> ================
> ./bootstrap.sh
> CC=clang CXX=clang++ ./configure --enable-dev-mode
> Needed to set DISABLE_INLINE to get passed the double declaration error in api-inline.h, i guess someone needs to figure out how to get this passed clang, correctly
> make menuselect.makeopts
> menuselect/menuselect --enable DISABLE_INLINE
> Needed to suppresse some of the warnings to get clang to compile (for now), clang can be a little panicky, but for a good reason.
> 
>     -Wno-unknown-warning-option. When gcc doesn't know a compiler option it returns NON-ZERO errorlevel, clang returns ZERO errorlevel, which is annoying. Even the linux kernel developers group complained about this. Will be fixed/changed (hopefully soon). For now, when checking clang compiler options, you would need to grep and parse the error output
>     -Wno-error needed to quite down clang being panicky (Standard asterisk -Werror is a good idea in general, but not when compiling against a 'new' compiler )
> 
> ASTCFLAGS="-Wno-unknown-warning-option -Wno-error" make
> make install
> RAII_VAR seems to work, but i guess there is still a bit of work before clang support for the rest of asterisk can be announced.
> 
> 
> Diffs
> -----
> 
>   /trunk/makeopts.in 413043 
>   /trunk/main/Makefile 413043 
>   /trunk/include/asterisk/utils.h 413043 
>   /trunk/configure.ac 413043 
>   /trunk/configure UNKNOWN 
>   /trunk/Makefile 413043 
> 
> Diff: https://reviewboard.asterisk.org/r/3488/diff/
> 
> 
> Testing
> -------
> 
> Just a proof of concept, to show how asterisk could be made clang compatible again.
> 
> 
> Thanks,
> 
> Diederik de Groot
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150126/9e2968d6/attachment.html>