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

2015-03-29 Thread Diederik de Groot 

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

(Updated March 29, 2015, 7:20 p.m.)


Review request for Asterisk Developers.


Bugs: ASTERISK-24917
https://issues.asterisk.org/jira/browse/ASTERISK-24917


Repository: Asterisk


Description (updated)
---

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_linkedlists.c 433444 
  /branches/13/tests/test_acl.c 433444 

Diff: https://reviewboard.asterisk.org/r/4555/diff/


Testing
---


Thanks,

Diederik de Groot

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2015-03-29 Thread Diederik de Groot 

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

(Updated March 29, 2015, 7:24 p.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 (updated)
-

  /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_linkedlists.c 433444 
  /branches/13/tests/test_acl.c 433444 

Diff: https://reviewboard.asterisk.org/r/4555/diff/


Testing
---


Thanks,

Diederik de Groot

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2015-03-29 Thread Diederik de Groot 

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



/branches/13/tests/test_acl.c


Can't use the gcc nested function extension. Could have replaced it with a 
clang -block solution, but opted for the simple external function solution.



/branches/13/tests/test_strings.c


The clang compiler blows up/crashes when running ast_alloca(0) 
(ie:__builtin_alloca(0))

possible solutions:
- report issue to clang / llvm
- add check to ast_alloca and enforce return value on error
- or make sure we don't call ast_alloca with a size of 0 ever.



/branches/13/tests/test_strings.c


Woops. Debugging remnant, should not have been here.


- Diederik de Groot


On March 29, 2015, 7:24 p.m., Diederik de Groot wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4555/
> ---
> 
> (Updated March 29, 2015, 7:24 p.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_linkedlists.c 433444 
>   /branches/13/tests/test_acl.c 433444 
> 
> Diff: https://reviewboard.asterisk.org/r/4555/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Diederik de Groot
> 
>

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2015-03-29 Thread Diederik de Groot 

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

(Updated March 29, 2015, 7:26 p.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 (updated)
-

  /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_linkedlists.c 433444 
  /branches/13/tests/test_acl.c 433444 

Diff: https://reviewboard.asterisk.org/r/4555/diff/


Testing
---


Thanks,

Diederik de Groot

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2015-03-29 Thread Diederik de Groot 

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



/branches/13/tests/test_acl.c


clang can't handle gcc's nested function extension. I could have opted to 
build a clang ^block instead, but i choose the simple external function instead.



/branches/13/tests/test_strings.c


The clang compiler blows up/crashes when running ast_alloca(0) 
(ie:__builtin_alloca(0))

possible solutions:
- report issue to clang / llvm
- add check to ast_alloca and enforce return value on error
- or make sure we don't call ast_alloca with a size of 0 ever.


- Diederik de Groot


On March 29, 2015, 7:26 p.m., Diederik de Groot wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4555/
> ---
> 
> (Updated March 29, 2015, 7:26 p.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_linkedlists.c 433444 
>   /branches/13/tests/test_acl.c 433444 
> 
> Diff: https://reviewboard.asterisk.org/r/4555/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Diederik de Groot
> 
>

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2015-03-29 Thread Diederik de Groot 

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

(Updated March 29, 2015, 8:21 p.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_linkedlists.c 433444 
  /branches/13/tests/test_acl.c 433444 

Diff: https://reviewboard.asterisk.org/r/4555/diff/


Testing (updated)
---

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


=== /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.


=== /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.


File Attachments (updated)


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

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2015-03-29 Thread Diederik de Groot 

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

(Updated March 30, 2015, 12:42 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 (updated)
-

  /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_linkedlists.c 433444 
  /branches/13/tests/test_acl.c 433444 
  /branches/13/main/utils.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


=== /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.


=== /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.


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

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2015-03-29 Thread Diederik de Groot 

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

(Updated March 30, 2015, 12:43 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 (updated)
-

  /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_linkedlists.c 433444 
  /branches/13/tests/test_acl.c 433444 
  /branches/13/main/utils.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


=== /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.


=== /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.


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

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2015-03-29 Thread Diederik de Groot 

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



/branches/13/tests/test_strings.c


If buflen being 0 is allowed in these tests, then ast_escape_semicolons 
returning NULL should also be allowed to make this test a valid case



/branches/13/main/utils.c


If buflen 0 is supposed to be allowed (as suggested by the tests), then 
this should be checked and not raise a problem in the assert


- Diederik de Groot


On March 30, 2015, 12:43 a.m., Diederik de Groot wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4555/
> ---
> 
> (Updated March 30, 2015, 12:43 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_linkedlists.c 433444 
>   /branches/13/tests/test_acl.c 433444 
>   /branches/13/main/utils.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
> 
> 
> === /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.
> 
> 
> === /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.
> 
> 
> 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
> 
>

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2015-03-29 Thread Diederik de Groot 

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

(Updated March 30, 2015, 12:49 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_linkedlists.c 433444 
  /branches/13/tests/test_acl.c 433444 
  /branches/13/main/utils.c 433444 

Diff: https://reviewboard.asterisk.org/r/4555/diff/


Testing (updated)
---

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

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2015-03-31 Thread rmudgett 

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



/branches/13/main/utils.c


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.



/branches/13/tests/test_acl.c


prototype unnecessary



/branches/13/tests/test_acl.c


Guidelines: curly on its own line for start of functions

res must be changed to a pointer so the value set in res can be passed out.



/branches/13/tests/test_linkedlists.c


Revert the changes in this file.  They are for the -Wparentheses-equality 
option.



/branches/13/tests/test_stringfields.c


Use %d instead of %i.  %d is more portable since it has been around since 
K&R.



/branches/13/tests/test_strings.c


Revert this.



/branches/13/tests/test_strings.c


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.



/branches/13/tests/test_strings.c


Did you mean ast_alloca() or ast_strdupa()?



/branches/13/tests/test_strings.c


Guidelines: No C++ comments.
Delete this comment line.



/branches/13/tests/test_strings.c


} else if (test_len == 0 {
  test2 = "";
}



/branches/13/tests/test_strings.c


Revert now that the clang ast_alloca() problem is worked around.


- rmudgett


On March 29, 2015, 5:49 p.m., Diederik de Groot wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4555/
> ---
> 
> (Updated March 29, 2015, 5:49 p.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_linkedlists.c 433444 
>   /branches/13/tests/test_acl.c 433444 
>   /branches/13/main/utils.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 investigatio

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

2015-03-31 Thread Diederik de Groot 

---
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 (updated)
-

  /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

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2015-03-31 Thread Diederik de Groot 


> On April 1, 2015, 1:21 a.m., rmudgett wrote:
> > /branches/13/tests/test_strings.c, line 399
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > Revert this.


> On April 1, 2015, 1:21 a.m., rmudgett wrote:
> > /branches/13/tests/test_strings.c, lines 407-409
> > 
> >
> > 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

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

2015-03-31 Thread rmudgett 


> On March 31, 2015, 6:21 p.m., rmudgett wrote:
> > /branches/13/main/utils.c, lines 492-493
> > 
> >
> > 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.
> 
> Diederik de Groot wrote:
> 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 ?

You are changing what is returned if buflen is zero.  Before it would return 
outbuf while the change makes it return NULL.  It is just fortunate that 
nothing cares about the return value.

The test code is in error here because for a zero length it writes beyond the 
buffer boundary.


> On March 31, 2015, 6:21 p.m., rmudgett wrote:
> > /branches/13/tests/test_strings.c, lines 395-396
> > 
> >
> > 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.
> 
> Diederik de Groot wrote:
> 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

That could be an undefined area.  What is the return value of malloc(0)?  Some 
systems return NULL while other systems return a pointer that you cannot 
dereference but can later pass to free().

The problem with the original test_semi() code is that with a zero length we 
dereference any returned pointer to put a '\0' in it.  For a zero length buffer 
that is beyond the bounds of the buffer and will corrupt the stack.  The code 
also reads beyond the boundary in strcmp().


- rmudgett


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


On March 31, 2015, 8:30 p.m., Diederik de Groot wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4555/
> ---
> 
> (Updated March 31, 2015, 8:30 p.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

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

2015-03-31 Thread rmudgett 

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



/branches/13/tests/test_strings.c


De red blobs.


- rmudgett


On March 31, 2015, 8:30 p.m., Diederik de Groot wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4555/
> ---
> 
> (Updated March 31, 2015, 8:30 p.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
> 
>

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2015-03-31 Thread Diederik de Groot 

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

(Updated April 1, 2015, 4:42 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 (updated)
-

  /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

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2015-03-31 Thread rmudgett 

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

Ship it!


Ship It!

- rmudgett


On March 31, 2015, 9:42 p.m., Diederik de Groot wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4555/
> ---
> 
> (Updated March 31, 2015, 9:42 p.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
> 
>

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2015-03-31 Thread Diederik de Groot 

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

(Updated April 1, 2015, 4:46 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 (updated)
-

  /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

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2015-03-31 Thread Diederik de Groot 

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

(Updated April 1, 2015, 4:47 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 (updated)
-

  /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

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2015-03-31 Thread Diederik de Groot 


> On April 1, 2015, 1:21 a.m., rmudgett wrote:
> > /branches/13/main/utils.c, lines 492-493
> > 
> >
> > 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.
> 
> Diederik de Groot wrote:
> 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 ?
> 
> rmudgett wrote:
> You are changing what is returned if buflen is zero.  Before it would 
> return outbuf while the change makes it return NULL.  It is just fortunate 
> that nothing cares about the return value.
> 
> The test code is in error here because for a zero length it writes beyond 
> the buffer boundary.

Ok Agreed.


> On April 1, 2015, 1:21 a.m., rmudgett wrote:
> > /branches/13/tests/test_strings.c, lines 395-396
> > 
> >
> > 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.
> 
> Diederik de Groot wrote:
> 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
> 
> rmudgett wrote:
> That could be an undefined area.  What is the return value of malloc(0)?  
> Some systems return NULL while other systems return a pointer that you cannot 
> dereference but can later pass to free().
> 
> The problem with the original test_semi() code is that with a zero length 
> we dereference any returned pointer to put a '\0' in it.  For a zero length 
> buffer that is beyond the bounds of the buffer and will corrupt the stack.  
> The code also reads beyond the boundary in strcmp().

Wrote a little tester.c

#include 
#include 
#include 
int main() {
int *test1 = malloc(0);
int *test2 = __builtin_alloca(0);
printf("%p / %p\n", test1, test2);
free(test1);
}

And without any compiler flags, clang and gcc seem to be happy and running this 
without a problem. Will have to figure out which compiler flags the Makefile 
compounds to see if that makes any difference.


- Diederik


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


On April 1, 2015, 4:47 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, 4:47 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 *

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

2015-04-11 Thread Diederik de Groot 

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

(Updated April 11, 2015, 10:26 a.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


Changes
---

Committed in revision 434705


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

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev