From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id BD3B03858C51 for ; Mon, 23 May 2022 17:07:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org BD3B03858C51 Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-439-L9Xd6iH-MPi6-0fxXDT02Q-1; Mon, 23 May 2022 13:07:37 -0400 X-MC-Unique: L9Xd6iH-MPi6-0fxXDT02Q-1 Received: by mail-wm1-f72.google.com with SMTP id k16-20020a7bc310000000b0038e6cf00439so5907538wmj.0 for ; Mon, 23 May 2022 10:07:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:in-reply-to:references:date :message-id:mime-version; bh=z+zBoF5LPU24OSj8nGTM2wFn2ZxnOXYoue4kK7XE5kI=; b=4oX2RgRUlG5MdU0fdWtBcPIQsL16tyvRoKhvdDHrF+nXDowBxvTxTr2jrezD8lAyvP c4tLkpG3fw8bkeXY82yx1jBuT0bKPwZ1DjAEgmkt3d5U3BjS+g4n09HRjmNnIiGDHJYl ZRjQhMbtkWDxdijiAF0EHw56DLf6AiRO8P8G1hW5Vhj0iJUAC+69D6YftJaot56vLlGg sciqkeCi7tDiMc+piu74hY4y1KUd5jSfi4hR3QCFm6gMm5QdntgfxhJXC10ksJFzfCgl Ar5t5Lei9P3PrXXph1QCj4bsn4gllpCQKGJZLuiJ4xn95wwJCnN6EqWr+B4raOCNN+Ho LlFA== X-Gm-Message-State: AOAM531BqD+VIsW7ulO9n7H6w9pbdhxvGvQWPagO+J+kmlzDDJRCR+zx 22qzBax75oUsX5cYyycj+mBHV0H+1ispjd0sEaIr0SHo5NyKMFLYy/EpJfMPYKnNh5E+3g/Shm3 VjsMMZMxeHjGVbiiNrc8Wh4uofGiwSPJc9YdGYrim94qlxDK7n/QCMuhpXrRTZORK7QOHW/16lQ == X-Received: by 2002:a05:600c:5120:b0:394:6480:d595 with SMTP id o32-20020a05600c512000b003946480d595mr10598wms.204.1653325655453; Mon, 23 May 2022 10:07:35 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz15n4N6znMa8WEbzNLSXNICJ4QTH0WAufzWetoSVGeaCRx/BQhhBOeIWhgAqxkAgZn6meoHQ== X-Received: by 2002:a05:600c:5120:b0:394:6480:d595 with SMTP id o32-20020a05600c512000b003946480d595mr10545wms.204.1653325654793; Mon, 23 May 2022 10:07:34 -0700 (PDT) Received: from localhost (host109-154-214-95.range109-154.btcentralplus.com. [109.154.214.95]) by smtp.gmail.com with ESMTPSA id q6-20020a1cf306000000b00397243d3dbcsm9446614wmq.31.2022.05.23.10.07.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 23 May 2022 10:07:34 -0700 (PDT) From: Andrew Burgess To: Ilya Leoshkevich via Gdb-patches , Tom Tromey Cc: Ulrich Weigand , Andreas Arnez , Ilya Leoshkevich , gdb-patches@sourceware.org Subject: Re: [PATCH v2] gdb: do not add const sections to the section map In-Reply-To: <20220517200308.2535419-1-iii@linux.ibm.com> References: <20220517200308.2535419-1-iii@linux.ibm.com> Date: Mon, 23 May 2022 18:07:33 +0100 Message-ID: <87mtf8i3e2.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-13.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 May 2022 17:07:41 -0000 I know there's been a little discussion of this patch on the v1 thread, but I wanted to record my thoughts, and here seemed the better place. Ilya Leoshkevich via Gdb-patches writes: > From: Ulrich Weigand > > build_objfile_section_table () creates four synthetic sections, which > significantly slow down section map sorting. This is especially > noticeable when debugging JITs that report a lot of objfiles. Since > these sections are not useful for find_pc_section (), do not add them > to the section map. This description could really be fleshed out a little more. You say "which significantly slow down section map sorting", but I'd like this to say which sort(s), in which function(s), otherwise I'm expected to either know, or go figure it out myself. You then jump to say the sections are not useful for "find_pc_section", but it's not immediately obvious how that relates to the change you're making. I think you should spell out that insert_section_p is only used by update_section_map, which updates the struct objfile_pspace_info sections table, which is only used from find_pc_section. Then you'd need to explain why non of these sections can ever be returned from find_pc_section, though it's not clear (from the discussion on the v1 thread) if the ABS section might be returned in some cases or not.. I tracked down the patch which I think originally added these synthetic sections, though I don't know if this helps much: https://sourceware.org/pipermail/gdb-patches/2013-February/100257.html > --- > v1: https://sourceware.org/pipermail/binutils/2022-May/120863.html > v1 -> v2: Fix code style, post to the correct mailing list (Andrew). > > gdb/objfiles.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/gdb/objfiles.c b/gdb/objfiles.c > index 80f68fda1c1..8a297c57530 100644 > --- a/gdb/objfiles.c > +++ b/gdb/objfiles.c > @@ -1005,6 +1005,11 @@ insert_section_p (const struct bfd *abfd, You'll need to update the comment just before this function. > if ((bfd_section_flags (section) & SEC_THREAD_LOCAL) != 0) > /* This is a TLS section. */ > return 0; > + if (bfd_is_const_section (section)) > + { > + /* This is one of the global *ABS*, *UND*, *IND*, or *COM* sections. */ > + return 0; > + } > > return 1; > } The final thing I think you need to add with this patch is some testing. We don't have much (that I'm aware of) in the way of performance testing, but what we can do, is add a mechanism by which we can gather performance data. Below you will find a patch that extends one of the existing JIT tests to gather performance data. I tried this before and after applying your patch, and I can confirm that the performance improvement with your change is significant. I think something like this should be included with this patch. I'm also tempted to wonder if we should have a whole new test added which does this: (1) Load a single JIT ELF a few times in a loop, restarting GDB each time. Time how long this takes, and get an average time. (2) Load a large number (~500 ish) JIT ELFs into GDB, and then time adding the last few (~5 ish) of these, take an average of this time. (3) If the average from step 2 is more than some multiple of step 1, then fail the test. The only problem of course is we don't really like non-deterministic tests, and whenever you're dealing with timings things can get non-deterministic. Still, it might be interesting to see if such a test could be written - we can always remove it later if it turns out to be too noisy. Thanks, Andrew --- commit 6e0318edd61be062cfb1734ddcf16941ff99bd0d Author: Andrew Burgess Date: Mon May 23 14:32:46 2022 +0100 gdb/testsuite: collect timing data for JIT ELF loading This commit updates the jit-elf.exp test so that each time an ELF containing JIT information is loaded into GDB, the time taken to load the ELF is printed. The goal of this change is to allow us to see if this process slows down as more and more ELFs are loaded. Further, the test is expanded such that when the environment variable GDB_JIT_ELF_SOLIB_COUNT is set to a number, this number of ELF files will be generated and loaded. The default, when the environment variable is not set, is to generate and load 2 ELFs, this is what the test did before this change. I've tested this script with up to 500 ELF's being generated and loaded, I see no reason why higher numbers should not work just as well, though some of the patterns generated in the test will fail if you try to create more than 9999 ELFs. The timing information is printed from the test binary itself, GDB is used to change a variable within the test binary so that the timing results are printed, we only print timing information for a single test run, so there will only be one set of results in the gdb.log file. To extract the results just use: $ grep -e "^Time taken" gdb/testsuite/gdb.log | cut -d: -f2 1,0.002640 2,0.002583 The first column is the ELF number and the second column is the time (in seconds) taken to load that ELF. As the number of ELFs increases then (obviously) the test takes longer, so I've extended the test timeouts in proportion to the number of ELFs being loaded in a couple of places. diff --git a/gdb/testsuite/gdb.base/jit-elf-main.c b/gdb/testsuite/gdb.base/jit-elf-main.c index 948530f3197..3986f885e74 100644 --- a/gdb/testsuite/gdb.base/jit-elf-main.c +++ b/gdb/testsuite/gdb.base/jit-elf-main.c @@ -27,6 +27,7 @@ #include #include #include +#include #include #include "jit-protocol.h" @@ -64,13 +65,37 @@ volatile int wait_for_gdb = ATTACH; /* The current process's PID. GDB retrieves this. */ int mypid; +/* Should timing data be printed? This is set from GDB. */ +volatile int print_timing_data_p = 0; + +void +show_time_taken (struct timeval *start, struct timeval *end) +{ + int seconds = end->tv_sec - start->tv_sec; + int useconds = end->tv_usec - start->tv_usec; + static int counter = 0; + + if (!print_timing_data_p) + return; + + while (useconds < 0 && seconds > 0) + { + seconds -= 1; + useconds += 1000000; + } + + ++counter; + printf ("Time taken: %d,%u.%06u\n", counter, seconds, useconds); +} + int MAIN (int argc, char *argv[]) { int i; - alarm (300); + alarm (300 + (5 * argc)); /* Used as backing storage for GDB to populate argv. */ - char *fake_argv[10]; + char **fake_argv = malloc (sizeof (char *) * (argc + 1)); + fake_argv[argc] = NULL; mypid = getpid (); /* gdb break here 0 */ @@ -87,6 +112,7 @@ MAIN (int argc, char *argv[]) void *load_addr = (void *) (size_t) (LOAD_ADDRESS + (i - 1) * LOAD_INCREMENT); printf ("Loading %s as JIT at %p\n", argv[i], load_addr); void *addr = load_elf (argv[i], &obj_size, load_addr); + struct timeval start_time, end_time; char name[32]; sprintf (name, "jit_function_%04d", i); @@ -106,7 +132,12 @@ MAIN (int argc, char *argv[]) /* Notify GDB. */ __jit_debug_descriptor.action_flag = JIT_REGISTER; + + gettimeofday (&start_time, NULL); __jit_debug_register_code (); + gettimeofday (&end_time, NULL); + + show_time_taken (&start_time, &end_time); if (jit_function () != 42) { diff --git a/gdb/testsuite/gdb.base/jit-elf.exp b/gdb/testsuite/gdb.base/jit-elf.exp index 4b75188a00d..27f107a10fa 100644 --- a/gdb/testsuite/gdb.base/jit-elf.exp +++ b/gdb/testsuite/gdb.base/jit-elf.exp @@ -70,11 +70,13 @@ proc clean_reattach {} { # Continue to LOCATION in the program. If REATTACH, detach and # re-attach to the program from scratch. # Return 0 if clean_reattach failed, otherwise return 1. -proc continue_to_test_location {location reattach} { +proc continue_to_test_location {location reattach timeout_factor} { global main_srcfile gdb_breakpoint [gdb_get_line_number $location $main_srcfile] - gdb_continue_to_breakpoint $location + with_timeout_factor $timeout_factor { + gdb_continue_to_breakpoint $location + } if {$reattach} { with_test_prefix "$location" { if { ![clean_reattach] } { @@ -85,7 +87,7 @@ proc continue_to_test_location {location reattach} { return 1 } -proc one_jit_test {jit_solibs_target match_str reattach} { +proc one_jit_test {jit_solibs_target match_str reattach {print_timing_data_p 0} } { set count [llength $jit_solibs_target] with_test_prefix "one_jit_test-$count" { @@ -103,6 +105,8 @@ proc one_jit_test {jit_solibs_target match_str reattach} { return } + gdb_test_no_output "set var print_timing_data_p=${print_timing_data_p}" + # Poke desired values directly into inferior instead of using "set args" # because "set args" does not work under gdbserver. incr count @@ -118,7 +122,7 @@ proc one_jit_test {jit_solibs_target match_str reattach} { gdb_continue_to_breakpoint "break here 0" - if { ![continue_to_test_location "break here 1" $reattach] } { + if { ![continue_to_test_location "break here 1" $reattach [llength $jit_solibs_target]] } { return } @@ -130,7 +134,7 @@ proc one_jit_test {jit_solibs_target match_str reattach} { gdb_test "maintenance info break" } - if { ![continue_to_test_location "break here 2" $reattach] } { + if { ![continue_to_test_location "break here 2" $reattach [llength $jit_solibs_target]] } { return } @@ -140,17 +144,34 @@ proc one_jit_test {jit_solibs_target match_str reattach} { } } +proc build_pattern { jit_solibs_target } { + set pattern_list {} + for { set i 0 } { $i < [llength $jit_solibs_target] } { incr i } { + set num [format "%04d" [expr $i + 1]] + lappend pattern_list "$::hex jit_function_${num}" + } + return join $pattern_list "\[\r\n\]+" +} + +# Figure out how many solibs we should generate and test with. +set solib_count 2 +if { [info exists env(GDB_JIT_ELF_SOLIB_COUNT)] } { + set solib_count $env(GDB_JIT_ELF_SOLIB_COUNT) +} + # Compile two shared libraries to use as JIT objects. set jit_solibs_target [compile_and_download_n_jit_so \ - $jit_solib_basename $jit_solib_srcfile 2] + $jit_solib_basename $jit_solib_srcfile ${solib_count}] if { $jit_solibs_target == -1 } { return } +set all_funcs_pattern [build_pattern $jit_solibs_target] + # Compile the main code (which loads the JIT objects). if { [compile_jit_main ${main_srcfile} ${main_binfile} {}] == 0 } { one_jit_test [lindex $jit_solibs_target 0] "${hex} jit_function_0001" 0 - one_jit_test $jit_solibs_target "${hex} jit_function_0001\[\r\n\]+${hex} jit_function_0002" 0 + one_jit_test $jit_solibs_target $all_funcs_pattern 0 1 } # Test attaching to an inferior with some JIT libraries already @@ -160,7 +181,7 @@ if {[can_spawn_for_attach]} { if { [compile_jit_main ${main_srcfile} "${main_binfile}-attach" \ {additional_flags=-DATTACH=1}] == 0 } { with_test_prefix attach { - one_jit_test $jit_solibs_target "${hex} jit_function_0001\[\r\n\]+${hex} jit_function_0002" 1 + one_jit_test $jit_solibs_target $all_funcs_pattern 1 } } } diff --git a/gdb/testsuite/lib/jit-elf-helpers.exp b/gdb/testsuite/lib/jit-elf-helpers.exp index b699917f209..b440e98705e 100644 --- a/gdb/testsuite/lib/jit-elf-helpers.exp +++ b/gdb/testsuite/lib/jit-elf-helpers.exp @@ -32,8 +32,8 @@ proc compile_jit_main {main_srcfile main_binfile options} { set options [concat \ $options \ - additional_flags=-DLOAD_ADDRESS=$jit_load_address \ - additional_flags=-DLOAD_INCREMENT=$jit_load_increment \ + additional_flags=-DLOAD_ADDRESS=${jit_load_address}ull \ + additional_flags=-DLOAD_INCREMENT=${jit_load_increment}ull \ debug] if { [gdb_compile ${main_srcfile} ${main_binfile} \