[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.

[Matt Jordan]

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


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.


/trunk/configure.ac
<https://reviewboard.asterisk.org/r/3488/#comment24762>

    This will actually always fail, causing gcc to fail to compile Asterisk.
    
    The AC_LANG_PROGRAM macro will expand the second argument as the contents of int main(void). This causes the following program to be generated:
    
    int main(void)
    {
        #if defined (__clang__)
        choke
        #end if
        int main(void) { return 0 };
    
    }
    
    Since this is invalid C code even for gcc, this will cause the Clang macros to always be applied. As a result, the Makefile will always add "-fblocks" to the _ASTCFLAGS, which will cause gcc to fail to compile.
    
    Changing the AC_LANG_PROGRAM macro to:
    
    AC_LANG_PROGRAM([], [
        #if defined(__clang__)
        choke
        #endif
    ],
    ...
    
    Should resolve the issue for gcc and still allow for detection of the compiler options for Clang.



/trunk/include/asterisk/utils.h
<https://reviewboard.asterisk.org/r/3488/#comment24763>

    I'd propose changing this slightly.
    
    Clang, unfortunately, does define the __GNUC__ macro, so we can't rely on it to know for sure that we have GCC as the compiler or Clang. On the other hand, as our configure script shows, we do know that Clang will define the __clang__ macro, which we can be pretty sure that GCC will not define. As such, we can rewrite this as:
    
    #if defined(__clang__) && defined(__has_feature)
    
    #elif defined(__GNUC__)
    
    #else
        #warning
    #endif
    
    I prefer this nomenclature to a #ifndef of __has_feature simply because we (a) don't define __has_feature, which may be used a test in other places; and (b) it is a bit clearer from reading the code that we are testing explicitly for Clang. The __has_feature is a bit ambiguous, unless you are intimately familiar with the Clang provided macros.


- Matt Jordan


On May 15, 2014, 3: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, 3: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/20150125/9443a4a4/attachment-0001.html>