fix: #1858 by changing the wrong logic in update_dbg_info #1884

yihong0618 · 2024-02-08T09:20:08Z

by changing the wrong logic in update_dbg_info

backtrace(need set follow-fork-mode child)

__memmove_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:457
457	../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S: No such file or directory.
#0  __memmove_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:457
#1  0x00007ffff7c8de6c in __GI__IO_default_xsputn (n=<optimized out>, data=<optimized out>, f=<optimized out>) at ./libio/genops.c:386
#2  __GI__IO_default_xsputn (f=0x7fffffff7a10, data=<optimized out>, n=174) at ./libio/genops.c:370
#3  0x00007ffff7c74918 in outstring_func (done=0, length=174, 
    string=0x55555c1634d0 "F: 723bd tensorflow._api.v2.experimental.numpy.random.<module>\nL: 1 /home/yihong/.local/lib/python3.10/site-packages/tensorflow/_api/v2/experimental/numpy/random/__init__.py\n", s=0x7fffffff7440) at ../libio/libioP.h:947
#4  printf_positional (s=s@entry=0x7fffffff7a10, format=format@entry=0x7ffff62d1911 "%s", readonly_format=<optimized out>, readonly_format@entry=0, ap=ap@entry=0x7fffffff7b90, 
    ap_savep=ap_savep@entry=0x7fffffff75a8, done=<optimized out>, done@entry=0, nspecs_done=<optimized out>, lead_str_end=<optimized out>, work_buffer=<optimized out>, 
    save_errno=<optimized out>, grouping=<optimized out>, thousands_sep=<optimized out>, mode_flags=<optimized out>) at ./stdio-common/vfprintf-internal.c:1927
#5  0x00007ffff7c75336 in __vfprintf_internal (s=s@entry=0x7fffffff7a10, format=format@entry=0x7ffff62d1911 "%s", ap=ap@entry=0x7fffffff7b90, mode_flags=mode_flags@entry=0)
    at ./stdio-common/vfprintf-internal.c:1602
#6  0x00007ffff7c8849a in __vsnprintf_internal (string=0x7ffff4ffffeb "", maxlen=<optimized out>, format=0x7ffff62d1911 "%s", args=args@entry=0x7fffffff7b90, mode_flags=mode_flags@entry=0)
    at ./libio/vsnprintf.c:114
#7  0x00007ffff7c60856 in __GI___snprintf (s=s@entry=0x7ffff4ffffeb "", maxlen=maxlen@entry=175, format=format@entry=0x7ffff62d1911 "%s") at ./stdio-common/snprintf.c:31
#8  0x00007ffff62cc1be in snprintf (__fmt=0x7ffff62d1911 "%s", __n=175, __s=0x7ffff4ffffeb "") at /usr/include/x86_64-linux-gnu/bits/stdio2.h:71
#9  update_dbg_info (line=<optimized out>, file=<optimized out>, addr=<optimized out>, name=<optimized out>) at /home/yihong/repos/uftrace/python/trace-python.c:403
#10 convert_function_addr (is_pyfunc=<optimized out>, args=<optimized out>, frame=<optimized out>) at /home/yihong/repos/uftrace/python/trace-python.c:943
#11 uftrace_trace_python (self=<optimized out>, args=<optimized out>) at /home/yihong/repos/uftrace/python/trace-python.c:976

honggyukim · 2024-02-08T12:43:48Z

Thanks very much for the fix! Could you please update the commit message following our commit message convention? The current commit title is too long and we tend to make the title and commit message short. Please read the discussion at https://stackoverflow.com/questions/2290016/git-commit-messages-50-72-formatting.

yihong0618 · 2024-02-08T13:07:25Z

Thanks very much for the fix! Could you please update the commit message following our commit message convention? The current commit title is too long and we tend to make the title and commit message short. Please read the discussion at https://stackoverflow.com/questions/2290016/git-commit-messages-50-72-formatting.

Done, and do we need to add comments to the line of #define UFTRACE_PYTHON_SYMTAB_SIZE (2 * 1024 * 1024)
I did search the code base, there's no such comments like that, so I did not add it.

honggyukim · 2024-02-08T14:11:27Z

Thanks. But I just found that the root cause is in different reason. The following would be more proper fix.

diff --git a/python/trace-python.c b/python/trace-python.c
index a0636b35..f6adc490 100644
--- a/python/trace-python.c
+++ b/python/trace-python.c
@@ -382,7 +382,7 @@ static void update_dbg_info(const char *name, uint64_t addr, const char *file, i
                old_hdr.val = tmp_hdr.val;
        }

-       if (new_hdr.offset >= uftrace_symtab_size) {
+       if (new_hdr.offset >= uftrace_dbginfo_size) {
                unsigned new_dbginfo_size = uftrace_dbginfo_size + UFTRACE_PYTHON_SYMTAB_SIZE;

                pr_dbg("try to increase the shared memory for %s (new size=%uMB)\n",

I think the original code at update_dbg_info was incorrectly written because it got confused with get_new_sym_addr.

honggyukim · 2024-02-08T14:13:23Z

We also need to be sure that entry_size + 1 is less than UFTRACE_PYTHON_SYMTAB_SIZE.

yihong0618 · 2024-02-08T14:27:13Z

uftrace_dbginfo_size

nice catch, will change the root cause.

yihong0618 · 2024-02-08T14:38:34Z

We also need to be sure that entry_size + 1 is less than UFTRACE_PYTHON_SYMTAB_SIZE.

I think we do not need to consider it seems always less than UFTRACE_PYTHON_SYMTAB_SIZE

	int entry_size = strlen(name) + 20; /* addr(16) + spaces(2) + type(1) + newline(1) */

yihong0618 · 2024-02-08T14:53:57Z

@honggyukim you are right, the root cause it the logic, also change the logic from if to while

honggyukim · 2024-02-08T14:56:02Z

I think we do not need to consider it seems always less than UFTRACE_PYTHON_SYMTAB_SIZE

Yeah, we won't need it for both get_new_sym_addr and update_dbg_info.

int entry_size = xasprintf(&buf, "F: %" PRIx64 " %s\nL: %d %s\n", addr, name, line, file);

So please apply the uftrace_dbginfo_size change only and make the commit message as follows.

python: Fix incorrect comparison in update_dbg_info

The logic to increase uftrace_dbginfo_size should be compared
with uftrace_dbginfo_size instead of uftrace_symtab_size.

This looks like a mistake due to confusion with get_new_sym_addr.

Fixed: #1858
Signed-off-by: Yi Hong <zouzou0208@gmail.com>
Signed-off-by: Honggyu Kim <honggyu.kp@gmail.com>

honggyukim · 2024-02-08T14:56:37Z

python/trace-python.c

@@ -261,7 +261,7 @@ static uint32_t get_new_sym_addr(const char *name, bool is_libcall)
 		old_hdr.val = tmp_hdr.val;
 	}

-	if (new_hdr.offset >= uftrace_symtab_size) {
+	while (new_hdr.offset >= uftrace_symtab_size) {


I don't think we need to use while. Please revert it back to if.

honggyukim · 2024-02-08T14:56:42Z

python/trace-python.c

@@ -382,7 +382,7 @@ static void update_dbg_info(const char *name, uint64_t addr, const char *file, i
 		old_hdr.val = tmp_hdr.val;
 	}

-	if (new_hdr.offset >= uftrace_symtab_size) {
+	while (new_hdr.offset >= uftrace_dbginfo_size) {


yihong0618 · 2024-02-08T14:58:54Z

I think we do not need to consider it seems always less than UFTRACE_PYTHON_SYMTAB_SIZE

Yeah, we won't need it for both get_new_sym_addr and update_dbg_info.
int entry_size = xasprintf(&buf, "F: %" PRIx64 " %s\nL: %d %s\n", addr, name, line, file);
So please apply the uftrace_dbginfo_size change only and make the commit message as follows.
python: Fix incorrect comparison in update_dbg_info

The logic to increase uftrace_dbginfo_size should be compared
with uftrace_dbginfo_size instead of uftrace_symtab_size.

This looks like a mistake due to confusion with get_new_sym_addr.

Fixed: #1858
Signed-off-by: Yi Hong <zouzou0208@gmail.com>
Signed-off-by: Honggyu Kim <honggyu.kp@gmail.com>

nice commit message, learned that.

honggyukim · 2024-02-08T15:00:17Z

FYI, Fixed: #1858 is detected by GitHub. That means if this commit is merged into master, then the issue #1858 is automatically closed.

The logic to increase uftrace_dbginfo_size should be compared with uftrace_dbginfo_size instead of uftrace_symtab_size. This looks like a mistake due to confusion with get_new_sym_addr. Fixed: namhyung#1858 Signed-off-by: Yi Hong <zouzou0208@gmail.com> Signed-off-by: Honggyu Kim <honggyu.kp@gmail.com>

yihong0618 · 2024-02-08T15:03:06Z

@honggyukim Done and thanks, I learn from your careful review every time, thanks again!

honggyukim

LGTM. Thanks very much! I will wait for @namhyung's final check.

namhyung

LGTM, thanks for the fix!

yihong0618 force-pushed the fix_issue_1858 branch 2 times, most recently from 7eae12e to c9fa998 Compare February 8, 2024 09:29

yihong0618 mentioned this pull request Feb 8, 2024

python tracing failure for a tensorflow program #1858

Closed

yihong0618 force-pushed the fix_issue_1858 branch from c9fa998 to 23fa078 Compare February 8, 2024 13:06

yihong0618 changed the title ~~fix: #1858~~ fix: #1858 by increasing UFTRACE_PYTHON_SYMTAB_SIZE default size to 2MB Feb 8, 2024

yihong0618 force-pushed the fix_issue_1858 branch from 23fa078 to 112689d Compare February 8, 2024 14:51

yihong0618 changed the title ~~fix: #1858 by increasing UFTRACE_PYTHON_SYMTAB_SIZE default size to 2MB~~ fix: #1858 by changing the wrong logic in update_dbg_info Feb 8, 2024

honggyukim reviewed Feb 8, 2024

View reviewed changes

yihong0618 force-pushed the fix_issue_1858 branch from 112689d to cda6bbe Compare February 8, 2024 15:01

honggyukim approved these changes Feb 8, 2024

View reviewed changes

namhyung approved these changes Feb 9, 2024

View reviewed changes

namhyung merged commit ecca1ec into namhyung:master Feb 9, 2024
3 checks passed

yihong0618 deleted the fix_issue_1858 branch February 12, 2024 12:56

yihong0618 mentioned this pull request Feb 18, 2024

Segmentation Fault Occured #1651

Open

chenrui333 mentioned this pull request Apr 25, 2024

uftrace 0.16 Homebrew/homebrew-core#170060

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: #1858 by changing the wrong logic in update_dbg_info #1884

fix: #1858 by changing the wrong logic in update_dbg_info #1884

yihong0618 commented Feb 8, 2024 •

edited

honggyukim commented Feb 8, 2024

yihong0618 commented Feb 8, 2024 •

edited

honggyukim commented Feb 8, 2024

honggyukim commented Feb 8, 2024 •

edited

yihong0618 commented Feb 8, 2024

yihong0618 commented Feb 8, 2024

yihong0618 commented Feb 8, 2024

honggyukim commented Feb 8, 2024

honggyukim Feb 8, 2024

honggyukim Feb 8, 2024

yihong0618 commented Feb 8, 2024

honggyukim commented Feb 8, 2024

yihong0618 commented Feb 8, 2024

honggyukim left a comment

namhyung left a comment

fix: #1858 by changing the wrong logic in update_dbg_info #1884

fix: #1858 by changing the wrong logic in update_dbg_info #1884

Conversation

yihong0618 commented Feb 8, 2024 • edited

honggyukim commented Feb 8, 2024

yihong0618 commented Feb 8, 2024 • edited

honggyukim commented Feb 8, 2024

honggyukim commented Feb 8, 2024 • edited

yihong0618 commented Feb 8, 2024

yihong0618 commented Feb 8, 2024

yihong0618 commented Feb 8, 2024

honggyukim commented Feb 8, 2024

honggyukim Feb 8, 2024

Choose a reason for hiding this comment

honggyukim Feb 8, 2024

Choose a reason for hiding this comment

yihong0618 commented Feb 8, 2024

honggyukim commented Feb 8, 2024

yihong0618 commented Feb 8, 2024

honggyukim left a comment

Choose a reason for hiding this comment

namhyung left a comment

Choose a reason for hiding this comment

yihong0618 commented Feb 8, 2024 •

edited

yihong0618 commented Feb 8, 2024 •

edited

honggyukim commented Feb 8, 2024 •

edited