From 906f1dbf3a68d234dea17c314e7d1150fd9693e6 Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Sun, 8 Sep 2024 22:19:07 -0700 Subject: [PATCH 1/4] mcount: Rename to mcount_pfd The pfd historically means pipe file descriptor which is used to communicate to the uftrace process. But it's a global variable so better to have mcount_ prefix. Signed-off-by: Namhyung Kim --- libmcount/internal.h | 2 +- libmcount/mcount.c | 14 +++++++------- libmcount/misc.c | 4 ++-- libmcount/wrap.c | 6 +++--- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/libmcount/internal.h b/libmcount/internal.h index 49dc149d4..57c140950 100644 --- a/libmcount/internal.h +++ b/libmcount/internal.h @@ -196,7 +196,7 @@ extern uint64_t mcount_threshold; /* nsec */ extern unsigned mcount_minsize; extern pthread_key_t mtd_key; extern int shmem_bufsize; -extern int pfd; +extern int mcount_pfd; extern int mcount_depth; extern char *mcount_exename; extern int page_size_in_kb; diff --git a/libmcount/mcount.c b/libmcount/mcount.c index ed1e1ed90..edc3b95b1 100644 --- a/libmcount/mcount.c +++ b/libmcount/mcount.c @@ -59,7 +59,7 @@ pthread_key_t mtd_key = (pthread_key_t)-1; TLS struct mcount_thread_data mtd; /* pipe file descriptor to communite to uftrace */ -int pfd = -1; +int mcount_pfd = -1; /* maximum depth of mcount rstack */ static int mcount_rstack_max = MCOUNT_RSTACK_MAX; @@ -612,12 +612,12 @@ static void send_session_msg(struct mcount_thread_data *mtdp, const char *sess_i }; int len = sizeof(msg) + msg.len; - if (pfd < 0) + if (mcount_pfd < 0) return; mcount_memcpy4(sess.sid, sess_id, sizeof(sess.sid)); - if (writev(pfd, iov, 3) != len) { + if (writev(mcount_pfd, iov, 3) != len) { if (!mcount_should_stop()) pr_err("write tid info failed"); } @@ -640,9 +640,9 @@ static void mcount_trace_finish(bool send_msg) if (send_msg) uftrace_send_message(UFTRACE_MSG_FINISH, NULL, 0); - if (pfd != -1) { - close(pfd); - pfd = -1; + if (mcount_pfd != -1) { + close(mcount_pfd); + mcount_pfd = -1; } trace_finished = true; @@ -1979,7 +1979,7 @@ static __used void mcount_startup(void) dirname = UFTRACE_DIR_NAME; xasprintf(&channel, "%s/%s", dirname, ".channel"); - pfd = open(channel, O_WRONLY); + mcount_pfd = open(channel, O_WRONLY); free(channel); if (getenv("UFTRACE_LIST_EVENT")) { diff --git a/libmcount/misc.c b/libmcount/misc.c index 359674236..3a9f159df 100644 --- a/libmcount/misc.c +++ b/libmcount/misc.c @@ -76,11 +76,11 @@ void uftrace_send_message(int type, void *data, size_t len) }, }; - if (pfd < 0) + if (mcount_pfd < 0) return; len += sizeof(msg); - if (writev(pfd, iov, 2) != (ssize_t)len) { + if (writev(mcount_pfd, iov, 2) != (ssize_t)len) { if (!mcount_should_stop()) pr_err("writing shmem name to pipe"); } diff --git a/libmcount/wrap.c b/libmcount/wrap.c index c52e91a1d..48f4cf780 100644 --- a/libmcount/wrap.c +++ b/libmcount/wrap.c @@ -56,12 +56,12 @@ static void send_dlopen_msg(struct mcount_thread_data *mtdp, const char *sess_id }; int len = sizeof(msg) + msg.len; - if (pfd < 0) + if (mcount_pfd < 0) return; mcount_memcpy4(dlop.sid, sess_id, sizeof(dlop.sid)); - if (writev(pfd, iov, 3) != len) { + if (writev(mcount_pfd, iov, 3) != len) { if (!mcount_should_stop()) pr_err("write tid info failed"); } @@ -615,7 +615,7 @@ __visible_default int close(int fd) if (unlikely(real_close == NULL)) mcount_hook_functions(); - if (unlikely(fd == pfd)) { + if (unlikely(fd == mcount_pfd)) { return 0; } From a466183ccbb229c72c59e24046679b1383ad1e5f Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Sun, 8 Sep 2024 22:22:28 -0700 Subject: [PATCH 2/4] mcount: Update message send failure errors They had confusing error text, let's update them. Signed-off-by: Namhyung Kim --- libmcount/mcount.c | 2 +- libmcount/misc.c | 2 +- libmcount/wrap.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/libmcount/mcount.c b/libmcount/mcount.c index edc3b95b1..3e30c4e8a 100644 --- a/libmcount/mcount.c +++ b/libmcount/mcount.c @@ -619,7 +619,7 @@ static void send_session_msg(struct mcount_thread_data *mtdp, const char *sess_i if (writev(mcount_pfd, iov, 3) != len) { if (!mcount_should_stop()) - pr_err("write tid info failed"); + pr_err("send session msg failed"); } } diff --git a/libmcount/misc.c b/libmcount/misc.c index 3a9f159df..5b8c8b366 100644 --- a/libmcount/misc.c +++ b/libmcount/misc.c @@ -82,7 +82,7 @@ void uftrace_send_message(int type, void *data, size_t len) len += sizeof(msg); if (writev(mcount_pfd, iov, 2) != (ssize_t)len) { if (!mcount_should_stop()) - pr_err("writing shmem name to pipe"); + pr_err("send msg (type %d) failed", type); } } diff --git a/libmcount/wrap.c b/libmcount/wrap.c index 48f4cf780..e2cbc5f4c 100644 --- a/libmcount/wrap.c +++ b/libmcount/wrap.c @@ -63,7 +63,7 @@ static void send_dlopen_msg(struct mcount_thread_data *mtdp, const char *sess_id if (writev(mcount_pfd, iov, 3) != len) { if (!mcount_should_stop()) - pr_err("write tid info failed"); + pr_err("send dlopen msg failed"); } } From 2340f337135df5713a8ddce3ebf2b1203588af78 Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Sun, 8 Sep 2024 22:27:44 -0700 Subject: [PATCH 3/4] mcount: Rename to mcount_{rstack,auto}_rehook() The rstack restore and reset functions (and their auto counterparts) are to temporarily change the return addresses in the stack. But I found it confusing what restore and reset mean when I look at the code some time later. So let's clarify the names. Basically libmcount hooks the return address (unless -e is used) to redirect it to mcount library when the function returns. Now restore means to change the (hooked) return address to its original address (as it it's not called uftrace), and rehook means to change the *restored* return address to redirect to the libmcount again. Signed-off-by: Namhyung Kim --- libmcount/internal.h | 8 ++++---- libmcount/mcount.c | 12 ++++++------ libmcount/misc.c | 4 ++-- libmcount/plthook.c | 4 ++-- libmcount/wrap.c | 16 ++++++++-------- 5 files changed, 22 insertions(+), 22 deletions(-) diff --git a/libmcount/internal.h b/libmcount/internal.h index 57c140950..899d36e22 100644 --- a/libmcount/internal.h +++ b/libmcount/internal.h @@ -317,11 +317,11 @@ extern void uftrace_send_message(int type, void *data, size_t len); extern void build_debug_domain(char *dbg_domain_str); extern void mcount_rstack_restore(struct mcount_thread_data *mtdp); -extern void mcount_rstack_reset(struct mcount_thread_data *mtdp); -extern void mcount_rstack_reset_exception(struct mcount_thread_data *mtdp, - unsigned long frame_addr); +extern void mcount_rstack_rehook(struct mcount_thread_data *mtdp); +extern void mcount_rstack_rehook_exception(struct mcount_thread_data *mtdp, + unsigned long frame_addr); extern void mcount_auto_restore(struct mcount_thread_data *mtdp); -extern void mcount_auto_reset(struct mcount_thread_data *mtdp); +extern void mcount_auto_rehook(struct mcount_thread_data *mtdp); extern bool mcount_rstack_has_plthook(struct mcount_thread_data *mtdp); extern void prepare_shmem_buffer(struct mcount_thread_data *mtdp); diff --git a/libmcount/mcount.c b/libmcount/mcount.c index 3e30c4e8a..37995aa11 100644 --- a/libmcount/mcount.c +++ b/libmcount/mcount.c @@ -1246,7 +1246,7 @@ void mcount_exit_filter_record(struct mcount_thread_data *mtdp, struct mcount_re mtdp->filter.out_count--; if (rstack->flags & MCOUNT_FL_RECOVER) - mcount_rstack_reset(mtdp); + mcount_rstack_rehook(mtdp); } #undef FLAGS_TO_CHECK @@ -1448,7 +1448,7 @@ static int __mcount_entry(unsigned long *parent_loc, unsigned long child, struct if (frame_addr < (unsigned long)parent_loc) frame_addr = (unsigned long)(parent_loc - 1); - mcount_rstack_reset_exception(mtdp, frame_addr); + mcount_rstack_rehook_exception(mtdp, frame_addr); mtdp->in_exception = false; } @@ -1522,7 +1522,7 @@ static unsigned long __mcount_exit(long *retval) /* re-hijack return address of parent */ if (mcount_auto_recover) - mcount_auto_reset(mtdp); + mcount_auto_rehook(mtdp); __mcount_unguard_recursion(mtdp); @@ -1586,7 +1586,7 @@ static int __cygprof_entry(unsigned long parent, unsigned long child) if (frame_addr < (unsigned long)frame_ptr) frame_addr = (unsigned long)frame_ptr; - mcount_rstack_reset_exception(mtdp, frame_addr); + mcount_rstack_rehook_exception(mtdp, frame_addr); mtdp->in_exception = false; } @@ -1733,7 +1733,7 @@ static void _xray_entry(unsigned long parent, unsigned long child, struct mcount if (frame_addr < (unsigned long)frame_ptr) frame_addr = (unsigned long)frame_ptr; - mcount_rstack_reset_exception(mtdp, frame_addr); + mcount_rstack_rehook_exception(mtdp, frame_addr); mtdp->in_exception = false; } @@ -2120,7 +2120,7 @@ void __visible_default mcount_reset(void) if (unlikely(check_thread_data(mtdp))) return; - mcount_rstack_reset(mtdp); + mcount_rstack_rehook(mtdp); } void __visible_default __cyg_profile_func_enter(void *child, void *parent) diff --git a/libmcount/misc.c b/libmcount/misc.c index 5b8c8b366..7d65a9b78 100644 --- a/libmcount/misc.c +++ b/libmcount/misc.c @@ -190,7 +190,7 @@ void mcount_rstack_restore(struct mcount_thread_data *mtdp) } /* hook return address again (used after mcount_rstack_restore) */ -void mcount_rstack_reset(struct mcount_thread_data *mtdp) +void mcount_rstack_rehook(struct mcount_thread_data *mtdp) { int idx; struct mcount_ret_stack *rstack; @@ -244,7 +244,7 @@ void mcount_auto_restore(struct mcount_thread_data *mtdp) } } -void mcount_auto_reset(struct mcount_thread_data *mtdp) +void mcount_auto_rehook(struct mcount_thread_data *mtdp) { struct mcount_ret_stack *curr_rstack; struct mcount_ret_stack *prev_rstack; diff --git a/libmcount/plthook.c b/libmcount/plthook.c index 8c8ba7ead..906c2cb6f 100644 --- a/libmcount/plthook.c +++ b/libmcount/plthook.c @@ -948,7 +948,7 @@ static unsigned long __plthook_entry(unsigned long *ret_addr, unsigned long chil strcmp(pd->mod_name, mcount_exename)) { *ret_addr = rstack->parent_ip; if (mcount_auto_recover) - mcount_auto_reset(mtdp); + mcount_auto_rehook(mtdp); /* * as its return address was recovered, @@ -1078,7 +1078,7 @@ static unsigned long __plthook_exit(long *retval) /* re-hijack return address of parent */ if (mcount_auto_recover) - mcount_auto_reset(mtdp); + mcount_auto_rehook(mtdp); __mcount_unguard_recursion(mtdp); diff --git a/libmcount/wrap.c b/libmcount/wrap.c index e2cbc5f4c..b095ea60b 100644 --- a/libmcount/wrap.c +++ b/libmcount/wrap.c @@ -93,7 +93,7 @@ static int dlopen_base_callback(struct dl_phdr_info *info, size_t size, void *ar return 0; } -void mcount_rstack_reset_exception(struct mcount_thread_data *mtdp, unsigned long frame_addr) +void mcount_rstack_rehook_exception(struct mcount_thread_data *mtdp, unsigned long frame_addr) { int idx; struct mcount_ret_stack *rstack; @@ -148,7 +148,7 @@ void mcount_rstack_reset_exception(struct mcount_thread_data *mtdp, unsigned lon mtdp->idx = idx + 1; pr_dbg3("%s: exception returned to [%d]\n", __func__, mtdp->idx); - mcount_rstack_reset(mtdp); + mcount_rstack_rehook(mtdp); } static char **collect_uftrace_envp(void) @@ -305,7 +305,7 @@ __visible_default int backtrace(void **buffer, int sz) ret = real_backtrace(buffer, sz); if (!check_thread_data(mtdp)) - mcount_rstack_reset(mtdp); + mcount_rstack_rehook(mtdp); return ret; } @@ -326,7 +326,7 @@ __visible_default void __cxa_throw(void *exception, void *type, void *dest) /* * restore return addresses so that it can unwind stack * frames safely during the exception handling. - * It pairs to mcount_rstack_reset_exception(). + * It pairs to mcount_rstack_rehook_exception(). */ mcount_rstack_restore(mtdp); } @@ -350,7 +350,7 @@ __visible_default void __cxa_rethrow(void) /* * restore return addresses so that it can unwind stack * frames safely during the exception handling. - * It pairs to mcount_rstack_reset_exception() + * It pairs to mcount_rstack_rehook_exception() */ mcount_rstack_restore(mtdp); } @@ -374,7 +374,7 @@ __visible_default void _Unwind_Resume(void *exception) /* * restore return addresses so that it can unwind stack * frames safely during the exception handling. - * It pairs to mcount_rstack_reset_exception(). + * It pairs to mcount_rstack_rehook_exception(). */ mcount_rstack_restore(mtdp); } @@ -404,7 +404,7 @@ __visible_default void *__cxa_begin_catch(void *exception) if (frame_addr < (unsigned long)frame_ptr) frame_addr = (unsigned long)frame_ptr; - mcount_rstack_reset_exception(mtdp, frame_addr); + mcount_rstack_rehook_exception(mtdp, frame_addr); mtdp->in_exception = false; pr_dbg2("%s: exception caught begin on [%d]\n", __func__, mtdp->idx); } @@ -443,7 +443,7 @@ __visible_default void __cxa_guard_abort(void *guard_obj) if (frame_addr < (unsigned long)frame_ptr) frame_addr = (unsigned long)frame_ptr; - mcount_rstack_reset_exception(mtdp, frame_addr); + mcount_rstack_rehook_exception(mtdp, frame_addr); mtdp->in_exception = false; /* From 2e3b7a83f2e0c2fdb1cf196d6db91f10bcb5135e Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Mon, 9 Sep 2024 00:00:23 -0700 Subject: [PATCH 4/4] mcount: Add some more comments Describe global (shared) data in libmcount and their protection mechanism. Also add an explanation of filtering functions in terms of libmcount variants. And explain how uftrace data is actuall recorded. Signed-off-by: Namhyung Kim --- libmcount/mcount.c | 56 +++++++++++++++++++++++++++++++++++++- libmcount/record.c | 67 +++++++++++++++++++++++++++++++++++++--------- 2 files changed, 109 insertions(+), 14 deletions(-) diff --git a/libmcount/mcount.c b/libmcount/mcount.c index 37995aa11..ba29a0baa 100644 --- a/libmcount/mcount.c +++ b/libmcount/mcount.c @@ -32,6 +32,21 @@ #include "utils/utils.h" #include "version.h" +/* + * mcount global variables. + * + * These are to control various features in the libmcount. + * They are set during initialization (mcount_startup) which I believe, runs in + * a single thread. After that multiple threads (mostly) read the value so it's + * not protected by lock or something. So special care needs to be taken if you + * want to change it at runtime (like in the agent). + * + * Some are marked as 'maybe unused' because they are only used when filter + * functions are implemented. Note that libmcount is built with different + * settings like -fast and -single to be more efficient in some situation like + * when no filter is specified in the command line and/or single-thread only. + */ + /* time filter in nsec */ uint64_t mcount_threshold; @@ -55,7 +70,13 @@ unsigned long mcount_global_flags = MCOUNT_GFL_SETUP; /* TSD key to save mtd below */ pthread_key_t mtd_key = (pthread_key_t)-1; -/* thread local data to trace function execution */ +/* + * A thread local data to trace function execution. + * While this is itself TLS so ok to by accessed safely by each thread, + * mcount routines use TSD APIs to access it for performance reason. + * Also TSD provides destructor so it can release the resources when the thread + * exits. + */ TLS struct mcount_thread_data mtd; /* pipe file descriptor to communite to uftrace */ @@ -106,6 +127,10 @@ __weak void dynamic_return(void) static LIST_HEAD(mcount_watch_list); #ifdef DISABLE_MCOUNT_FILTER +/* + * These functions are only the FAST version of libmcount libraries which don't + * implement filters (other than time and size filters). + */ static void mcount_filter_init(struct uftrace_filter_setting *filter_setting, bool force) { @@ -129,6 +154,9 @@ static void mcount_watch_finish(void) } #else +/* + * Here goes the regular libmcount's filter and trigger functions. + */ /* be careful: this can be called from signal handler */ static void mcount_finish_trigger(void) @@ -581,6 +609,10 @@ static void mcount_watch_release(struct mcount_thread_data *mtdp) #endif /* DISABLE_MCOUNT_FILTER */ +/* + * These are common routines used in every libmcount libraries. + */ + static void send_session_msg(struct mcount_thread_data *mtdp, const char *sess_id) { struct uftrace_msg_sess sess = { @@ -919,6 +951,10 @@ static bool mcount_check_rstack(struct mcount_thread_data *mtdp) } #ifndef DISABLE_MCOUNT_FILTER +/* + * Again, this implements filter functionality used in !fast versions. + */ + extern void *get_argbuf(struct mcount_thread_data *, struct mcount_ret_stack *); /** @@ -1309,6 +1345,10 @@ void mcount_exit_filter_record(struct mcount_thread_data *mtdp, struct mcount_re } #else /* DISABLE_MCOUNT_FILTER */ +/* + * Here fast versions don't implement filters. + */ + enum filter_result mcount_entry_filter_check(struct mcount_thread_data *mtdp, unsigned long child, struct uftrace_trigger *tr) { @@ -2093,14 +2133,20 @@ static void mcount_cleanup(void) */ #define UFTRACE_ALIAS(_func) void uftrace_##_func(void *, void *) __alias(_func) +/* This is the historic startup routine for mcount but not used here. */ void __visible_default __monstartup(unsigned long low, unsigned long high) { } +/* This is the historic cleanup routine for mcount but not used here. */ void __visible_default _mcleanup(void) { } +/* + * This is a non-standard external function to work around some stack + * corruption problems in the past. I hope we don't need it anymore. + */ void __visible_default mcount_restore(void) { struct mcount_thread_data *mtdp; @@ -2112,6 +2158,10 @@ void __visible_default mcount_restore(void) mcount_rstack_restore(mtdp); } +/* + * This is a non-standard external function to work around some stack + * corruption problems in the past. I hope we don't need it anymore. + */ void __visible_default mcount_reset(void) { struct mcount_thread_data *mtdp; @@ -2123,6 +2173,10 @@ void __visible_default mcount_reset(void) mcount_rstack_rehook(mtdp); } +/* + * External entry points for -finstrument-functions. The alias was added to + * avoid calling them through PLT. + */ void __visible_default __cyg_profile_func_enter(void *child, void *parent) { cygprof_entry((unsigned long)parent, (unsigned long)child); diff --git a/libmcount/record.c b/libmcount/record.c index b480b6fd5..e8264ec74 100644 --- a/libmcount/record.c +++ b/libmcount/record.c @@ -240,6 +240,10 @@ static struct mcount_event *get_event_pointer(void *base, unsigned idx) } #ifndef DISABLE_MCOUNT_FILTER +/* + * These functions are for regular libmcount with filters. + */ + void *get_argbuf(struct mcount_thread_data *mtdp, struct mcount_ret_stack *rstack) { ptrdiff_t idx = rstack - mtdp->rstack; @@ -818,6 +822,10 @@ void save_watchpoint(struct mcount_thread_data *mtdp, struct mcount_ret_stack *r } #else +/* + * These are for fast libmcount libraries without filters. + */ + void *get_argbuf(struct mcount_thread_data *mtdp, struct mcount_ret_stack *rstack) { return NULL; @@ -893,7 +901,17 @@ static int record_event(struct mcount_thread_data *mtdp, struct mcount_event *ev /* * instead of set bit fields, do the bit operations manually. - * this would be good for both performance and portability. + * this would be good for both performance and portability, + * and should be equivalent to the following: + * + * struct uftrace_record *data = curr_buf->data + curr_buf->size; + * + * data->time = event->time; + * data->type = UFTRACE_EVENT; + * data->magic = RECORD_MAGIC; + * data->more = 0; + * data->depth = 0; + * data->addr = event->id; */ rec->data = UFTRACE_EVENT | RECORD_MAGIC << 3; rec->data += (uint64_t)event->id << 16; @@ -971,19 +989,19 @@ static int record_ret_stack(struct mcount_thread_data *mtdp, enum uftrace_record if (curr_buf == NULL) return mtdp->shmem.done ? 0 : -1; -#if 0 - frstack = (void *)(curr_buf->data + curr_buf->size); - - frstack->time = timestamp; - frstack->type = type; - frstack->magic = RECORD_MAGIC; - frstack->more = !!argbuf; - frstack->depth = mrstack->depth; - frstack->addr = mrstack->child_ip; -#else /* * instead of set bit fields, do the bit operations manually. - * this would be good for both performance and portability. + * this would be good for both performance and portability, + * and should be equivalent to the following: + * + * frstack = (void *)(curr_buf->data + curr_buf->size); + * + * frstack->time = timestamp; + * frstack->type = type; + * frstack->magic = RECORD_MAGIC; + * frstack->more = !!argbuf; + * frstack->depth = mrstack->depth; + * frstack->addr = mrstack->child_ip; */ rec = type | RECORD_MAGIC << 3; rec += argbuf ? 4 : 0; @@ -993,7 +1011,6 @@ static int record_ret_stack(struct mcount_thread_data *mtdp, enum uftrace_record buf = (void *)(curr_buf->data + curr_buf->size); buf[0] = timestamp; buf[1] = rec; -#endif curr_buf->size += sizeof(*frstack); mrstack->flags |= MCOUNT_FL_WRITTEN; @@ -1033,6 +1050,30 @@ static int record_ret_stack(struct mcount_thread_data *mtdp, enum uftrace_record return 0; } +/* + * For performance reasons and time filter, it doesn't record trace data one at + * a time. Instead it usually writes the data when an EXIT record is ready so + * it needs to record ENTRY data in the current and may in the parent functions. + * + * For example, if it has a time filter for 1 usec. + * + * foo() { + * bar() { + * leaf1(); // takes 0.5 usec + * leaf2(); // takes 1.2 usec + * + * Then it can start to record when leaf2 function returns (at this moment, + * mcount_ret_stack for leaf1 is gone) then it'd save the following records + * (unless ENTRY foo or bar is saved by an earlier child before leaf[12]). + * + * ENTRY (foo) + * ENTRY (bar) + * ENTRY (leaf2) + * EXIT (leaf2) + * + * Then it adds MCOUNT_FL_WRITTEN flag to parent (foo and bar) so that they + * never be written anymore by other child function. + */ int record_trace_data(struct mcount_thread_data *mtdp, struct mcount_ret_stack *mrstack, long *retval) {