[asterisk-dev] [Code Review] Out-of-tree modules can fail because of compiler flag differences.

[Mark Michelson]

> On 2010-02-16 15:14:06, qwell wrote:
> > /branches/1.4/include/asterisk/lock.h, line 106
> > <https://reviewboard.asterisk.org/r/508/diff/1/?file=8147#file8147line106>
> >
> >     In the DEBUG_THREADS case, ast_mutex_t was just a pthread_mutex_t.  Hopefully this isn't too much overhead.  It simplifies things immensely.

After taking a closer look at exactly what overhead is involved here, I am of the opinion that what you have here is acceptable. I also suspect that a good optimizer will simply do away with the extra function call that is added when DEBUG_THREADS is not enabled.


- Mark


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


On 2010-02-17 14:34:12, qwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/508/
> -----------------------------------------------------------
> 
> (Updated 2010-02-17 14:34:12)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> When compiling modules out-of-tree, there is a large potential for ABI breakage, due to changing compiler flags.  If Asterisk is compiled with DEBUG_THREADS, and an out-of-tree module is not, it should not give undefined symbol errors during load (in fact, there's no reason it shouldn't work).  The biggest problem areas seemed to be DEBUG_THREADS and DEBUG_CHANNEL_LOCKS.
> 
> This is my (mostly conceptual, for now) attempt to solve this, by keeping the API the same regardless of compiler flags.
> 
> At this point, I'm not looking for an in-depth review of the code.  More important is a design review.  Is this the right way to go about this?
> 
> 
> Note: I believe many of the functions in lock.h will need to be moved into a .c file (main/lock.c, probably).  They are currently defined as static inline, which defeats much of the purpose.  A module *would* have the information about where a lock was obtained, but since it's including lock.h without DEBUG_THREADS enabled, it wouldn't bother logging it - even though Asterisk would really like it to.
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/include/asterisk/astobj2.h 246977 
>   /branches/1.4/include/asterisk/lock.h 246977 
>   /branches/1.4/main/astobj2.c 246977 
>   /branches/1.4/main/channel.c 246977 
> 
> Diff: https://reviewboard.asterisk.org/r/508/diff
> 
> 
> Testing
> -------
> 
> Compile tests with/without DEBUG_THREADS and DEBUG_CHANNEL_LOCKS work great.  I've not tried building an external module with different flags yet.  I suspect that one would compile fine, but may not act properly, partially because of the note above.
> 
> 
> Thanks,
> 
> qwell
> 
>