[PATCH v2] gdb: do not add const sections to the section map

[PATCH v2] gdb: do not add const sections to the section map

[Ilya Leoshkevich]

From: Ulrich Weigand <ulrich.weigand@de.ibm.com>

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.
---
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,
   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;
 }
-- 
2.35.1

Re: [PATCH v2] gdb: do not add const sections to the section map

[Andrew Burgess]

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 <gdb-patches@sourceware.org> writes:

> From: Ulrich Weigand <ulrich.weigand@de.ibm.com>
>
> 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 <aburgess@redhat.com>
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 <string.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
+#include <sys/time.h>
 #include <unistd.h>
 
 #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} \

Re: [PATCH v2] gdb: do not add const sections to the section map

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

> 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 <gdb-patches@sourceware.org> writes:
>
>> From: Ulrich Weigand <ulrich.weigand@de.ibm.com>
>>
>> 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.

FYI, here's a graph of the performance change I'm seeing when loading
500 JIT ELF files:

  https://ibb.co/kQrGjtb

Thanks,
Andrew

Re: [PATCH v2] gdb: do not add const sections to the section map

[Pedro Alves]

On 2022-05-23 18:07, Andrew Burgess via Gdb-patches wrote:

> 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.

Did you see the gdb.perf/ testsuite?

Re: [PATCH v2] gdb: do not add const sections to the section map

[Andrew Burgess]

Pedro Alves <pedro@palves.net> writes:

> On 2022-05-23 18:07, Andrew Burgess via Gdb-patches wrote:
>
>> 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.
>
> Did you see the gdb.perf/ testsuite?

No I didn't.  Thanks for pointing that out.

I don't think there's any JIT testing in there, I guess Ilya (or maybe
me) should add the JIT performance test in there.

Thanks,
Andrew

Re: [PATCH v2] gdb: do not add const sections to the section map

[Ilya Leoshkevich]

On Tue, 2022-05-24 at 09:17 +0100, Andrew Burgess wrote:
> Pedro Alves <pedro@palves.net> writes:
> 
> > On 2022-05-23 18:07, Andrew Burgess via Gdb-patches wrote:
> > 
> > > 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.
> > 
> > Did you see the gdb.perf/ testsuite?
> 
> No I didn't.  Thanks for pointing that out.
> 
> I don't think there's any JIT testing in there, I guess Ilya (or
> maybe
> me) should add the JIT performance test in there.
> 
> Thanks,
> Andrew
> 

I'll give it a try, thanks for all the advice!