* Re: [PATCH v2] gdb: do not add const sections to the section map
2022-05-17 20:03 [PATCH v2] gdb: do not add const sections to the section map Ilya Leoshkevich
@ 2022-05-23 17:07 ` Andrew Burgess
2022-05-23 17:22 ` Andrew Burgess
2022-05-23 19:13 ` Pedro Alves
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Burgess @ 2022-05-23 17:07 UTC (permalink / raw)
To: Ilya Leoshkevich via Gdb-patches, Tom Tromey
Cc: Ulrich Weigand, Andreas Arnez, Ilya Leoshkevich, gdb-patches
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} \
^ permalink raw reply [flat|nested] 6+ messages in thread