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

[Matt Jordan]

> On Jan. 25, 2015, 11:36 p.m., Diederik de Groot wrote:
> > /branches/11/include/asterisk/inline_api.h, lines 44-51
> > <https://reviewboard.asterisk.org/r/4370/diff/1/?file=71056#file71056line44>
> >
> >     You might want to check out:
> >         #define forceinline __inline__ __attribute__((always_inline))
> >         #define ensure_forceinline __attribute__((always_inline)) // inline or die
> >     
> >     To see if they would help forcing clang to inline
> 
> Matt Jordan wrote:
>     I gave it a shot, and clang still believes that the functions are re-defined.
>     
>     There may be a magical invocation to get clang to believe that these inlinings should occur, but I'm not sure what it is.
> 
> Diederik de Groot wrote:
>     I was intriged by the inline issue a little. There must be some solution. Declaring the function static ought to help against the multiple definition warning. Be cause the goal is to inline the function, we already expect the function content to be replicated all over the place, so the argument in the comment in include/asterisk/inline_api.h line 30 doesn't completely hold up.
>     
>     For the non-inlined version, static will not really be problematic, as the clang compiler (even in -O0 mode) will optimize the multiple declarations out (as opposed to gcc, which would not do this in -O0 mode, resulting in a bigger binary then necessary).
>     
>     So:
>     --- include/asterisk/inline_api.h    2015-02-20 14:11:49.011228989 +0100
>     +++ include/asterisk/inline_api.h~    2015-02-20 14:11:38.000000000 +0100
>     @@ -52,7 +52,7 @@
>      
>      #if !defined(AST_API_MODULE)
>      #if defined(__clang__)
>     -#define AST_INLINE_API(hdr, body) static hdr; static inline hdr body
>     +#define AST_INLINE_API(hdr, body) static hdr; static forceinline hdr body
>      #else
>      #define AST_INLINE_API(hdr, body) hdr; extern inline hdr body
>      #endif
>     
>     Seems to do the trick, as far as i can tell. Can you verify this ?
> 
> Diederik de Groot wrote:
>     Diff went a little astray:
>     Final result should have looked like:
>     #if !defined(AST_API_MODULE)
>     #if defined(__clang__)
>     #define AST_INLINE_API(hdr, body) static hdr; static inline hdr body
>     #else
>     #define AST_INLINE_API(hdr, body) hdr; extern inline hdr body
>     #endif
>     #else
>     #define AST_INLINE_API(hdr, body) hdr; hdr body
>     #endif
>

Yup, that works just fine. I haven't disassembled anything to see what it actually generated, but it certainly compiled and ran. That meets my litmus test for this patch!


- Matt


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


On Feb. 19, 2015, 10:09 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4370/
> -----------------------------------------------------------
> 
> (Updated Feb. 19, 2015, 10:09 p.m.)
> 
> 
> Review request for Asterisk Developers and Diederik de Groot.
> 
> 
> Bugs: ASTERISK-20850
>     https://issues.asterisk.org/jira/browse/ASTERISK-20850
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This is a continuation of the patch put up for review on r3488. It addresses the issues found on that review.
> 
> This patch *should* make Asterisk compile under clang. Note that compiling with --enable-dev-mode will cause Asterisk to fail to compile under clang, as it detects a number of warnings that aren't fixed under this patch.
> 
> 
> Diffs
> -----
> 
>   /branches/11/makeopts.in 432011 
>   /branches/11/main/Makefile 432011 
>   /branches/11/include/asterisk/utils.h 432011 
>   /branches/11/include/asterisk/inline_api.h 432011 
>   /branches/11/configure.ac 432011 
>   /branches/11/configure UNKNOWN 
>   /branches/11/Makefile 432011 
> 
> Diff: https://reviewboard.asterisk.org/r/4370/diff/
> 
> 
> Testing
> -------
> 
> * Compiled Asterisk with and without --enable-dev-mode using gcc. Asterisk compiles correctly.
> * Compiled Asterisk without --enable-dev-mode using clang. Asterisk compiles, links, and executes.
> 
> Note that you will need the BlocksRuntime to run Asterisk when it is compiled with clang.
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150221/7bd225da/attachment-0001.html>