Summary:	ASTERISK-24917: [patch] clang compilation warnings
Reporter:	Diederik de Groot (dkdegroot)	Labels:
Date Opened:	2015-03-26 10:51:10	Date Closed:	2015-05-17 15:23:09
Priority:	Major	Regression?
Status:	Closed/Complete	Components:	Core/General
Versions:	11.17.0 13.2.0	Frequency of Occurrence
Related Issues:	is related to ASTERISK-27635 [patch] app_voicemail: Avoid always true warnings with clang.
Environment:	clang	Attachments:	( 0) make.report
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. Will use this issue report to group together a number of reviewboard entries.
Comments:	By: Diederik de Groot (dkdegroot) 2015-03-26 11:55:17.895-0500 -Wabsolute-value: https://reviewboard.asterisk.org/r/4525/ Issue: using abs() instead of labs() when dealing with long instead of int values Issue: Mixing unsigned int and int resulting in taking the absolute value of an unsigned int By: Diederik de Groot (dkdegroot) 2015-03-26 12:33:44.915-0500 -Wunused-value -Wunused-variable -Wunused-const-variable https://reviewboard.asterisk.org/r/4526/ By: Diederik de Groot (dkdegroot) 2015-03-26 12:48:03.506-0500 -Wunused-function: https://reviewboard.asterisk.org/r/4527/ By: Diederik de Groot (dkdegroot) 2015-03-26 13:04:12.759-0500 -Wno-sometimes-uninitialized: https://reviewboard.asterisk.org/r/4529 By: Diederik de Groot (dkdegroot) 2015-03-26 13:13:02.399-0500 -Wgnu-variable-sized-type-not-at-end: https://reviewboard.asterisk.org/r/4530/ By: Diederik de Groot (dkdegroot) 2015-03-26 13:21:12.995-0500 -Wparentheses-equality: https://reviewboard.asterisk.org/r/4531/ By: Diederik de Groot (dkdegroot) 2015-03-26 16:58:20.758-0500 -Wtautological-compare: Comparing unsigned int > 0, which is always true. https://reviewboard.asterisk.org/r/4533/ Note: This one shows a number of occasins where comparing and enum < 0 where the function which should have returned an enum is actually trying to convey an error state using -1. Side Remark: It might be a good idea to create a generic way to convey such an error state in all enum2str and str2enum functions, either by extending the enum to include a SENTINEL / ERROR state, or by having such function also return and error state by reference. It might even be a good idea to implement three generic versions of enum2str/str2enum (one for bitfield oriented enums, one for sparse enums and one for regular ones, all using a similar format and able to return and error state (especially important for str2enum)). By: Diederik de Groot (dkdegroot) 2015-03-26 17:55:55.652-0500 -Wenum-conversion: https://reviewboard.asterisk.org/r/4535/ By: Diederik de Groot (dkdegroot) 2015-03-27 06:25:28.778-0500 -Wbitfield-constant-conversion: https://reviewboard.asterisk.org/r/4537/ By: Diederik de Groot (dkdegroot) 2015-03-27 06:41:45.046-0500 -Winitializer-overrides: https://reviewboard.asterisk.org/r/4539/ By: Diederik de Groot (dkdegroot) 2015-03-27 06:50:49.475-0500 -Wformat: https://reviewboard.asterisk.org/r/4540/ By: Diederik de Groot (dkdegroot) 2015-03-27 07:13:19.567-0500 -Wpointer-bool-conversion: https://reviewboard.asterisk.org/r/4541/ By: Diederik de Groot (dkdegroot) 2015-03-27 07:33:32.384-0500 That should be all for asterisk-13 as far as i can see. After running all these patches, clang is able to compile cleanly without any warnings on my x86_64 system. Note I have not ran any tests with the produced binary yet. Leaving that the owners of the different code segments, as they will know best if the proposed changes will/would cause any issues. By: Diederik de Groot (dkdegroot) 2015-03-27 11:03:39.737-0500 -Wself-assign: https://reviewboard.asterisk.org/r/4544/ By: Diederik de Groot (dkdegroot) 2015-03-27 11:05:48.566-0500 -Wunused-command-line-argument: https://reviewboard.asterisk.org/r/4545/ Needed to quiscence the compiler in dev-mode when creating the ".i" files By: Diederik de Groot (dkdegroot) 2015-03-27 13:00:34.024-0500 -Warray-bounds: https://reviewboard.asterisk.org/r/4546/ By: Diederik de Groot (dkdegroot) 2015-03-27 13:35:34.194-0500 braces-around-scalar-initializer: https://reviewboard.asterisk.org/r/4547/ By: Diederik de Groot (dkdegroot) 2015-03-28 11:42:47.584-0500 --dev-mode and -Wparentheses-equality: https://reviewboard.asterisk.org/r/4550/ By: Diederik de Groot (dkdegroot) 2015-03-28 11:53:38.779-0500 -Wnon-literal-null-conversion: https://reviewboard.asterisk.org/r/4551/ By: Diederik de Groot (dkdegroot) 2015-03-28 12:08:53.150-0500 -Wsometimes-uninitialized: https://reviewboard.asterisk.org/r/4552/ By: Diederik de Groot (dkdegroot) 2015-03-28 13:22:56.569-0500 -Wunused-function: https://reviewboard.asterisk.org/r/4553/ By: Diederik de Groot (dkdegroot) 2015-03-28 14:23:38.664-0500 --dev-mode and -Wunused-value: https://reviewboard.asterisk.org/r/4550/ By: Diederik de Groot (dkdegroot) 2015-03-28 14:35:27.974-0500 With all the above patches it is possible to cleanly compile using clang: ASTCFLAGS="-Wno-unused-value" make The unused-value is the last warning that either needs to be suppressed or needs fixing. The task of fixing all the warnings produced by "-Wunused-value" is a bit to much for one person though. By: Diederik de Groot (dkdegroot) 2015-03-28 18:45:21.818-0500 The one remaining compile warning after applying all the patches is the " -Wunused-value" warnining ./configure --enable-dev-mode CC=clang CXX=clang++ ASTCFLAGS="-Wno-error=unused-value" make &| grep "\[\-W" -B1 -A2 >make.report By: Diederik de Groot (dkdegroot) 2015-03-29 08:01:19.162-0500 Ran clang's static analyzer (scan-build) which found 819 of potential issues in asterisk-13 revision 433715, some of which are real bugs and of course some are also false positives. Adding both clang and scan-build to a CI environment could be quite helpfull in tracking down these bugs. The scanbuild file is pretty bif (sorry about that), but it includes the branching path that leads up to the bug. http://asterisk.chan-sccp.net:443/scanbuild-output/2015-03-29-155615-16015-1/ I hope that seeing the quality of the scan-build report, make all the work above worth it. besides the fact that some distributions / OS's are switching to llc + clang completely. By: Diederik de Groot (dkdegroot) 2015-03-29 12:20:30.204-0500 asterisk/tests need a couple of small fixes as well. https://reviewboard.asterisk.org/r/4555/ By: Diederik de Groot (dkdegroot) 2015-04-18 11:16:24.556-0500 Down to one last review. https://reviewboard.asterisk.org/r/4613/ By: Richard Mudgett (rmudgett) 2015-04-28 10:34:52.136-0500 Is there anything left on this issue that hasn't been committed? By: Diederik de Groot (dkdegroot) 2015-04-28 10:59:33.970-0500 Hi Richard, They only one left is: https://gerrit.asterisk.org/#/c/157/ The rest has all gone through. When that one is through. I will recompile asterisk-11 / 12 / 13 and trunk and see and make sure everything is complete (which it should be). By: Rusty Newton (rnewton) 2015-05-17 15:23:09.831-0500 Looks like everything has been merged here. Closing this out. If it is incomplete let us know in #asterisk-bugs on irc.freenode.net or asteriskteam@digium.com.