[Asterisk-code-review] backtrace: Refactor ast bt get symbols so it doesn't crash (asterisk[13])

[Corey Farrell]

Corey Farrell has posted comments on this change. ( https://gerrit.asterisk.org/10602 )

Change subject: backtrace:  Refactor ast_bt_get_symbols so it doesn't crash
......................................................................


Patch Set 6: Code-Review-1

(5 comments)

https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c
File main/backtrace.c:

https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@33
PS6, Line 33: #include "asterisk/utils.h"
            : #include "asterisk/strings.h"
These includes are unused and should be removed.


https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@58
PS6, Line 58: #include "asterisk/vector.h"
My vote is to prevent vector.h from including asterisk/lock.h.  We should avoid as many Asterisk headers as possible.
/* Block include of asterisk/lock.h */
#define _ASTERISK_LOCK_H

On the other hand maybe vector.h shouldn't include asterisk/lock.h at all.  Any source which uses locking versions of vectors should directly include asterisk/lock.h.


https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@111
PS6, Line 111: /*
             :  * We CANNOT call the ast_ equivalents for pthread_
             :  * functions here so we need to undo the redirections
             :  * created by lock.h so we can call the pthread_
             :  * functions directly.
             :  */
             : #undef pthread_mutex_lock
             : #undef pthread_mutex_unlock
             : #undef pthread_mutex_t
I'd prefer this be moved to the top after including headers.  If we remove the include of lock.h from vector.h this will become unnecessary.


https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@163
PS6, Line 163: 		snprintf(data->msg, MSG_BUFF_LEN, "%s %s %s:%u %s()",
I added a 'void *address' to data, set it to addresses[stackfr], prepended "[%p] " to the format with address.  The following was printed by ast_log_backtrace:
# 0: [0x51afdb] [0x7fcb609b8af4]   chan_sip.so chan_sip.c:35545 load_module()
# 1: [0x51bc0c] [0x51afdb]   asterisk loader.c:1126 start_resource()
# 2: [0x51db7e] [0x51bc0c]   asterisk loader.c:1119 start_resource()
# 3: [0x4366fd] [0x51bc0c] i asterisk loader.c:1374 load_resource_list()
# 4: [0x7fcbcf44911b] [0x51db7e]   asterisk loader.c:1512 load_modules()
# 5: [0x43813a] [0x4366fd]   asterisk asterisk.c:4038 check_init()
# 6: [(nil)] [0x4366fd] i asterisk asterisk.c:4279 asterisk_daemon()
# 7: [(nil)] [0x4366fd] i asterisk asterisk.c:4032 main()
# 8: [(nil)] [0x7fcbcf44911b]   libc.so.6 :0 __libc_start_main()
# 9: [(nil)] [0x43813a]   asterisk :0 _start()


In this example output the first [%p] of each line is from ast_log_backtrace, the second [%p] is from right here.  Probably should just print the address from here instead of ast_log_backtrace, I don't think ast_log_backtrace can compensate for the inline frames that do not have corresponding entries in the backtrace addresses.


https://gerrit.asterisk.org/#/c/10602/6/main/backtrace.c@305
PS6, Line 305: struct ast_vector_string *__ast_bt_get_symbols(void **addresses, size_t num_frames)
             : {
             : 	return __ast_bt_get_symbols_impl(addresses, num_frames);
             : }
Why not just rename __ast_bt_get_symbols_impl in the two sections?



-- 
To view, visit https://gerrit.asterisk.org/10602
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I79d02862ddaa2423a0809caa4b3b85c128131621
Gerrit-Change-Number: 10602
Gerrit-PatchSet: 6
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Mon, 12 Nov 2018 20:51:26 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181112/ad75fdfb/attachment.html>