[asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value

[Diederik de Groot]

> On April 1, 2015, 12:30 a.m., rmudgett wrote:
> > I dislike this warning.  It is more nuisance than helpful.  I once worked with compiler that had a similar warning.  The lengths you sometimes have to go to eradicate the the warning can be extream.
> > 
> > There are several places in this patch that change code behavior and introduce bugs.


Hi rmudgett,

I agree this would be the biggest amount of work of the whole 'clang' project. It does not really matter from a performance standpoint i think.

It can be helpfull to find refcount issues where a previously used refcounted pointer was unreffed and might vanish at any moment. If we where to always require that you have to use the return value from any function including XXX_unref, then we would be setting that that pointer to NULL and therefor the static compiler would be able to detect the null pointer dereference for any use after this location. There are other ways to deal with this, for example adding __attribute(context)__ and using sparse to find these (like the linux-kernel does) instead of depending on this little warning.

Is it worth the work ? I don't know. I leave that decision up to you guys. 

You might want to have a look at:
http://asterisk.chan-sccp.net:443/scanbuild-output/2015-03-29-155615-16015-1/

To see how usefull this static compiler can really be (Don't mind the "Cast from non-struct type to struct type" for now).

FYI : I know my patch/fix was certainly not complete nor quite sure not bug free, it was more a sample case of how this could be handled, sorry that that was not clear from the patch description.


- Diederik


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


On March 29, 2015, 1:44 a.m., Diederik de Groot wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4554/
> -----------------------------------------------------------
> 
> (Updated March 29, 2015, 1:44 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24917
>     https://issues.asterisk.org/jira/browse/ASTERISK-24917
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. 
> 
> clang compiler warning:-Wunused-value
> 
> Changes:
> apps/app_agent_pool.c
> multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used.
> return value from AST_LIST_REMOVE should be checked.
> Possible issues with leaked logged reference (added remark)
> 
> channels/chan_iax2.c
> return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer
> return value from AST_SCHED_DEL should be used to update the scheduled variable
> return value from AST_LIST_REMOVE should be checked.
> 
> channels/iax2/parser.c
> return value from AST_LIST_REMOVE should be checked.
> 
> include/asterisk/stringfields.h
> Quick fix to send the returned value from ast_string_field_ptr_set into the void.
> 
> Work in Progress:
> There are too many sources issues with "-Wunused-value" for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on.
> 
> Should we suppress "-Wunused-value" ? :
> I think some of the issues pointed out by clang could be very helpfull, for example the "unref/ref/AST_LIST_REMOVE" changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes.
> 
> 
> Diffs
> -----
> 
>   /branches/13/include/asterisk/stringfields.h 433444 
>   /branches/13/channels/iax2/parser.c 433444 
>   /branches/13/channels/chan_iax2.c 433444 
>   /branches/13/apps/app_agent_pool.c 433444 
> 
> Diff: https://reviewboard.asterisk.org/r/4554/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> ASTCFLAGS="-Wno-error=unused-value" make &| grep "[-W" -B1 -A2
>   https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report
> 
> 
> Thanks,
> 
> Diederik de Groot
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150401/2c91f439/attachment.html>