> 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
>
>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
