Hi Arnaldo, On Mon, 2024-11-11 at 10:42 -0300, Arnaldo Carvalho de Melo wrote: > Adding Mark to the CC list. And CC elfutils-devel so this gets more exposure to the people working on elfutils thread-safety. For those missing some context the whole thread is here: https://lore.kernel.org/dwarves/ZzIJqnFh2WfzHirN@x1/T/#u pahole source code can be found here: https://git.kernel.org/pub/scm/devel/pahole/pahole.git (See the README for build instructions.) > On Mon, Nov 11, 2024 at 01:36:52PM +0100, Jiri Olsa wrote: > > On Mon, Nov 11, 2024 at 12:01:13AM -0800, Eduard Zingerman wrote: > > > On Sun, 2024-11-10 at 03:38 -0800, Eduard Zingerman wrote: > > > > > > [...] > > > > > > > Also, it appears there is some bug either in pahole or in libdw's > > > > implementation of dwarf_getlocation(). When I try both your patch-set > > > > and my variant there is a segfault once in a while: > > > > > > > > $ for i in $(seq 1 100); \ > > > > do echo "---> $i"; \ > > > > pahole -j --skip_encoding_btf_inconsistent_proto -J --btf_encode_detached=/dev/null vmlinux ; \ > > > > done > > > > ---> 1 > > > > ... > > > > ---> 71 > > > > Segmentation fault (core dumped) > > > > ... > > > > > > > > The segfault happens only when -j (multiple threads) is passed. > > > > If pahole is built with sanitizers > > > > (passing -DCMAKE_C_FLAGS="-fsanitize=undefined,address") > > > > the stack trace looks as follows: > > > > > > Did some additional research for these SEGFAULTs. > > > Looks like all we are in trouble. > > > > > > # TLDR > > > > > > libdw is not supposed to be used in a concurrent context. > > > libdw is a part of elfutils package, the configuration flag > > > making API thread-safe is documented as experimental: > > > --enable-thread-safety enable thread safety of libraries EXPERIMENTAL > > > At-least Fedora 40 does not ship elfutils built with this flag set. > > > This colours all current parallel DWARF decoding questionable. Unfortunately true. The good news is that there is a serious effort to complete the work so that --enable-thread-safety can be turned on by default, but that work isn't complete yet. Which is why we currently have it marked as EXPERIMENTAL. It would be really nice if people did try it out though and reported any remaining issues. https://sourceware.org/bugzilla/enter_bug.cgi?product=elfutils or sent email to elfutils-devel@xxxxxxxxxxxxxx > > > # Why segfault happens > > > > > > Any references to elfutils source code are for commit [1]. > > > The dwarf_getlocation() is one of a few libdw APIs that uses memory > > > allocation internally. The function dwarf_getlocation.c:__libdw_intern_expression > > > iterates over expression encodings in DWARF and allocates > > > a set of objects of type `struct loclist` and `Dwarf_Op`. > > > Pointers to allocated objects are put to a binary tree for caching, > > > see dwarf_getlocation.c:660, the call to eu_tsearch() function. > > > The eu_tsearch() is a wrapper around libc tsearch() function. > > > This wrapper provides locking for the tree, > > > but only if --enable-thread-safety was set during elfutils configuration. > > > The SEGFAULT happens inside tsearch() call because binary tree is malformed, e.g.: > > > > > > Thread 8 "pahole" received signal SIGSEGV, Segmentation fault. > > > [Switching to Thread 0x7fffd9c006c0 (LWP 2630074)] > > > 0x00007ffff7c5d200 in maybe_split_for_insert (...) at tsearch.c:228 > > > 228 if (parentp != NULL && RED(DEREFNODEPTR(parentp))) > > > (gdb) bt > > > #0 0x00007ffff7c5d200 in maybe_split_for_insert (...) at tsearch.c:228 > > > #1 0x00007ffff7c5d466 in __GI___tsearch (...) at tsearch.c:358 > > > #2 __GI___tsearch (...) at tsearch.c:290 > > > #3 0x000000000048f096 in __interceptor_tsearch () > > > #4 0x00007ffff7f5c482 in __libdw_intern_expression (...) at dwarf_getlocation.c:660 > > > #5 0x00007ffff7f5cf51 in getlocation (...) at dwarf_getlocation.c:678 > > > #6 getlocation (...) at dwarf_getlocation.c:667 > > > #7 dwarf_getlocation (..._ at dwarf_getlocation.c:708 > > > #8 0x00000000005a2ee5 in parameter.new () > > > #9 0x00000000005a0122 in die.process_function () > > > #10 0x0000000000597efd in __die__process_tag () > > > #11 0x0000000000595ad9 in die.process_unit () > > > #12 0x0000000000595436 in die.process () > > > #13 0x00000000005b0187 in dwarf_cus.process_cu () > > > #14 0x00000000005afa38 in dwarf_cus.process_cu_thread () > > > #15 0x00000000004c7b8d in asan_thread_start(void*) () > > > #16 0x00007ffff7bda6d7 in start_thread (arg=<optimized out>) at pthread_create.c:447 > > > #17 0x00007ffff7c5e60c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:78 > > > (gdb) p parentp > > > $1 = (node *) 0x50300079d2a0 > > > (gdb) p *parentp > > > $2 = (node) 0x0 > > > > > > glibc provides a way to validate binary tree structure. > > > For this misc/tsearch.c has to be changed to define DEBUGGING variable. > > > (I used glibc 2.39 as provided by source rpm for Fedora 40 for experiments). > > > If this is done and custom glibc is used for pahole execution, > > > the following error is reported if '-j' flag is present: > > > > > > $ pahole -j --skip_encoding_btf_inconsistent_proto -J --btf_encode_detached=/home/eddy/work/tmp/my-new.btf vmlinux > > > Fatal glibc error: tsearch.c:164 (check_tree_recurse): assertion failed: d_sofar == d_total > > > Fatal glibc error: tsearch.c:164 (check_tree_recurse): assertion failed: d_sofar == d_total > > > Aborted (core dumped) > > > > > > Executing pahole using a custom-built libdw, > > > built with --enable-thread-safety resolves the issue. > > > > could we use libdw__lock around that? but I guess we use it on other > > places as well.. > > Right, if you look at the cset where I intrduced libdw__lock (at the end > of this message), its a similar case, probably we should rename it to > libdw__decl_lock and introduce a libdw__getlocation_lock? This way, for > these known to be not thread safe we use libdw in serially while not > having a libdw "big lock" getting too much in the way of pahole's > parallel operation? The trouble with a "big lock" is that it is easy to create a big deadlock, especially given that libdw provides various callbacks which are then run under the big lock and must be careful to not call back into libdw trying to reacquire the big lock. So you are currently only using that libdw__lock around the dwarf_decl_file/line calls? Is there a pahole tesstsuite which exercises the -j parallel option? Are you trying to check thread-safety of pahole with any tools like valgrind helgrind? Thanks, Mark > $ git show 1caed1c443d4a0dc5 > commit 1caed1c443d4a0dc5794f21954be7d5ae0396c90 > Author: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> > Date: Thu Jul 8 16:07:50 2021 -0300 > > dwarf_loader: Add a lock around dwarf_decl_file() and dwarf_decl_line() calls > > As this ends up racing on a tsearch() call, probably for some libdw > cache that gets updated/lookedup in concurrent pahole threads (-j N). > > This cures the following, a patch for libdw will be cooked up and sent. > > (gdb) run -j -I -F dwarf vmlinux > /dev/null > Starting program: /var/home/acme/git/pahole/build/pahole -j -I -F dwarf vmlinux > /dev/null > warning: Expected absolute pathname for libpthread in the inferior, but got .gnu_debugdata for /lib64/libpthread.so.0. > warning: File "/usr/lib64/libthread_db-1.0.so" auto-loading has been declined by your `auto-load safe-path' set to "$debugdir:$datadir/auto-load". > warning: Unable to find libthread_db matching inferior's thread library, thread debugging will not be available. > [New LWP 844789] > [New LWP 844790] > [New LWP 844791] > [New LWP 844792] > [New LWP 844793] > [New LWP 844794] > [New LWP 844795] > [New LWP 844796] > [New LWP 844797] > [New LWP 844798] > [New LWP 844799] > [New LWP 844800] > [New LWP 844801] > [New LWP 844802] > [New LWP 844803] > [New LWP 844804] > [New LWP 844805] > [New LWP 844806] > [New LWP 844807] > [New LWP 844808] > [New LWP 844809] > [New LWP 844810] > [New LWP 844811] > [New LWP 844812] > [New LWP 844813] > [New LWP 844814] > > Thread 2 "pahole" received signal SIGSEGV, Segmentation fault. > [Switching to LWP 844789] > 0x00007ffff7dfa321 in ?? () from /lib64/libc.so.6 > (gdb) bt > #0 0x00007ffff7dfa321 in ?? () from /lib64/libc.so.6 > #1 0x00007ffff7dfa4bb in ?? () from /lib64/libc.so.6 > #2 0x00007ffff7f5eaa6 in __libdw_getsrclines (dbg=0x4a7f90, debug_line_offset=10383710, comp_dir=0x7ffff3c29f01 "/var/home/acme/git/build/v5.13.0-rc6+", address_size=address_size@entry=8, linesp=linesp@entry=0x7fffcfe04ba0, filesp=filesp@entry=0x7fffcfe04ba8) > at dwarf_getsrclines.c:1129 > #3 0x00007ffff7f5ed14 in dwarf_getsrclines (cudie=cudie@entry=0x7fffd210caf0, lines=lines@entry=0x7fffd210cac0, nlines=nlines@entry=0x7fffd210cac8) at dwarf_getsrclines.c:1213 > #4 0x00007ffff7f64883 in dwarf_decl_file (die=<optimized out>) at dwarf_decl_file.c:66 > #5 0x0000000000425f24 in tag__init (tag=0x7fff0421b710, cu=0x7fffcc001e40, die=0x7fffd210cd30) at /var/home/acme/git/pahole/dwarf_loader.c:476 > #6 0x00000000004262ec in namespace__init (namespace=0x7fff0421b710, die=0x7fffd210cd30, cu=0x7fffcc001e40, conf=0x475600 <conf_load>) at /var/home/acme/git/pahole/dwarf_loader.c:576 > #7 0x00000000004263ac in type__init (type=0x7fff0421b710, die=0x7fffd210cd30, cu=0x7fffcc001e40, conf=0x475600 <conf_load>) at /var/home/acme/git/pahole/dwarf_loader.c:595 > #8 0x00000000004264d1 in type__new (die=0x7fffd210cd30, cu=0x7fffcc001e40, conf=0x475600 <conf_load>) at /var/home/acme/git/pahole/dwarf_loader.c:614 > #9 0x0000000000427ba6 in die__create_new_typedef (die=0x7fffd210cd30, cu=0x7fffcc001e40, conf=0x475600 <conf_load>) at /var/home/acme/git/pahole/dwarf_loader.c:1212 > #10 0x0000000000428df5 in __die__process_tag (die=0x7fffd210cd30, cu=0x7fffcc001e40, top_level=1, fn=0x45cee0 <__FUNCTION__.10> "die__process_unit", conf=0x475600 <conf_load>) at /var/home/acme/git/pahole/dwarf_loader.c:1823 > #11 0x0000000000428ea1 in die__process_unit (die=0x7fffd210cd30, cu=0x7fffcc001e40, conf=0x475600 <conf_load>) at /var/home/acme/git/pahole/dwarf_loader.c:1848 > #12 0x0000000000429e45 in die__process (die=0x7fffd210ce20, cu=0x7fffcc001e40, conf=0x475600 <conf_load>) at /var/home/acme/git/pahole/dwarf_loader.c:2311 > #13 0x0000000000429ecb in die__process_and_recode (die=0x7fffd210ce20, cu=0x7fffcc001e40, conf=0x475600 <conf_load>) at /var/home/acme/git/pahole/dwarf_loader.c:2326 > #14 0x000000000042a9d6 in dwarf_cus__create_and_process_cu (dcus=0x7fffffffddc0, cu_die=0x7fffd210ce20, pointer_size=8 '\b') at /var/home/acme/git/pahole/dwarf_loader.c:2644 > #15 0x000000000042ab28 in dwarf_cus__process_cu_thread (arg=0x7fffffffddc0) at /var/home/acme/git/pahole/dwarf_loader.c:2687 > #16 0x00007ffff7ed6299 in start_thread () from /lib64/libpthread.so.0 > #17 0x00007ffff7dfe353 in ?? () from /lib64/libc.so.6 > (gdb) > (gdb) fr 2 > 1085 > (gdb) list files_lines_compare > 1086 static int > 1087 files_lines_compare (const void *p1, const void *p2) > 1088 { > 1089 const struct files_lines_s *t1 = p1; > 1090 const struct files_lines_s *t2 = p2; > 1091 > 1092 if (t1->debug_line_offset < t2->debug_line_offset) > (gdb) > 1093 return -1; > 1094 if (t1->debug_line_offset > t2->debug_line_offset) > 1095 return 1; > 1096 > 1097 return 0; > 1098 } > 1099 > 1100 int > 1101 internal_function > 1102 __libdw_getsrclines (Dwarf *dbg, Dwarf_Off debug_line_offset, > (gdb) list __libdw_getsrclines > 1100 int > 1101 internal_function > 1102 __libdw_getsrclines (Dwarf *dbg, Dwarf_Off debug_line_offset, > 1103 const char *comp_dir, unsigned address_size, > 1104 Dwarf_Lines **linesp, Dwarf_Files **filesp) > 1105 { > 1106 struct files_lines_s fake = { .debug_line_offset = debug_line_offset }; > 1107 struct files_lines_s **found = tfind (&fake, &dbg->files_lines, > 1108 files_lines_compare); > 1109 if (found == NULL) > (gdb) > 1110 { > 1111 Elf_Data *data = __libdw_checked_get_data (dbg, IDX_debug_line); > 1112 if (data == NULL > 1113 || __libdw_offset_in_section (dbg, IDX_debug_line, > 1114 debug_line_offset, 1) != 0) > 1115 return -1; > 1116 > 1117 const unsigned char *linep = data->d_buf + debug_line_offset; > 1118 const unsigned char *lineendp = data->d_buf + data->d_size; > 1119 > (gdb) > 1120 struct files_lines_s *node = libdw_alloc (dbg, struct files_lines_s, > 1121 sizeof *node, 1); > 1122 > 1123 if (read_srclines (dbg, linep, lineendp, comp_dir, address_size, > 1124 &node->lines, &node->files) != 0) > 1125 return -1; > 1126 > 1127 node->debug_line_offset = debug_line_offset; > 1128 > 1129 found = tsearch (node, &dbg->files_lines, files_lines_compare); > (gdb) > > Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
