[asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang

Tue Mar 31 20:30:27 CDT 2015

> On April 1, 2015, 1:21 a.m., rmudgett wrote:
> > /branches/13/tests/test_strings.c, line 399
> > <https://reviewboard.asterisk.org/r/4555/diff/5/?file=73351#file73351line399>
> >
> >     Did you mean ast_alloca() or ast_strdupa()?

Ooh sorry i meant ast_alloca... Typo.

> On April 1, 2015, 1:21 a.m., rmudgett wrote:
> > /branches/13/main/utils.c, lines 492-493
> > <https://reviewboard.asterisk.org/r/4555/diff/5/?file=73346#file73346line492>
> >
> >     Revert this.  This change could affect the callers of the function since you are changing the API.  However, no callers currently care about the return value.
> >     
> >     You changed this because clang had a problem in test_semi1() in tests/test_strings.c.  There is a better way.

Actually as i tried to mention in the description i changed this according to the tests being run. I was not sure which one is supposed to be leading, the test or the source. 

The test says it expects:
test_semi(";", "", 0)
To be legal. FOr it to be legal ast_alloca(0) must be allowed.

Am i really changing the API here ?

> On April 1, 2015, 1:21 a.m., rmudgett wrote:
> > /branches/13/tests/test_strings.c, lines 395-396
> > <https://reviewboard.asterisk.org/r/4555/diff/5/?file=73351#file73351line395>
> >
> >     Is clang returning a NULL pointer when passed a zero length?  If so this could be a problem througout the codebase since the code assumes that the function can never fail.

I am not sure what happens, the compiler actually segfaults when it encountered this one. I was a little shocked about it as well. I guess the __builtins are a little more scary. If you wanna find out, give it a try. I think this should at least be reported to the llvm/clang team.

I am not sure how to interpret alloca(0) either... What is the developer trying to achieve here. And what should it return. It can't allocate space of 0 length and return a pointer to it. 

A simple but ugly temp-fix would be to change 

#define ast_alloca(size) __builtin_alloca(size)

to:

#if defined(__clang)
#define ast_alloca(size) __builtin_alloca(size ? size : 1);
#else
#define ast_alloca(size) __builtin_alloca(size)
#endif

> On April 1, 2015, 1:21 a.m., rmudgett wrote:
> > /branches/13/tests/test_strings.c, line 393
> > <https://reviewboard.asterisk.org/r/4555/diff/5/?file=73351#file73351line393>
> >
> >     Revert this.

> On April 1, 2015, 1:21 a.m., rmudgett wrote:
> > /branches/13/tests/test_strings.c, lines 407-409
> > <https://reviewboard.asterisk.org/r/4555/diff/5/?file=73351#file73351line407>
> >
> >     Revert now that the clang ast_alloca() problem is worked around.

ast_alloca problem remain, but is not specifically related to this issue.

- Diederik

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

On April 1, 2015, 3:30 a.m., Diederik de Groot wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4555/
> -----------------------------------------------------------
> 
> (Updated April 1, 2015, 3:30 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. 
> 
> fixes for tests to be compiled using clang
> 
> 
> Diffs
> -----
> 
>   /branches/13/tests/test_strings.c 433444 
>   /branches/13/tests/test_stringfields.c 433444 
>   /branches/13/tests/test_sched.c 433444 
>   /branches/13/tests/test_acl.c 433444 
> 
> Diff: https://reviewboard.asterisk.org/r/4555/diff/
> 
> 
> Testing
> -------
> 
> executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) -> 
> test show results failed:
> 
> 
> =================== /main/message/ ====== 
> FAIL   test_message_queue_handler_nom /main/message/             31036ms
> [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message
> 
> Not sure if this is actually a fail or just a timeout. WIP
> 
> 
> =================== /main/strings/ ====== 
> FAIL   escape_semicolons              /main/strings/             1ms     
> [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL && outbuf != NULL (0)
> -> explainable by the change made to the source. ast_alloca(0) is not being executed -> test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes.
> 
> (With revision 5 of this code, this test now passes without a problem, had to fix both the test and the function being tested though)
> 
> 
> =================== /main/stdtime ====== 
> "test execute all" fails, caused by the /main/stdtime/ test. 
> START  /main/stdtime/ - timezone_watch 
> [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test...
> j62747*CLI> 
> CLI becomes unresponsive / no further command completion for example.
> Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct.
> 
> Seems to be caused by inotify_daemon, at least there is where the segfault happens. WIP
> 
> 
> File Attachments
> ----------------
> 
> tests results xml (except /main/stdtime)
>   https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml
> 
> 
> Thanks,
> 
> Diederik de Groot
> 
>

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