From 9fc299558896b4ff19c45a3e62459851e4d96cb9 Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Mon, 19 Apr 2021 13:14:41 +0100 Subject: [PATCH] gdb: remove some caching from the dwarf reader While working on some changes to 'info sources' I ran into a situation where I was seeing the same source files reported twice in the output of the 'info sources' command when using either .gdb_index or the .debug_name index. I traced the problem back to some caching in dwarf2_base_index_functions::map_symbol_filenames; when called GDB caches the set of filenames, but, filesnames are not removed as the index entries are expanded into full symtabs. As a result we can end up seeing filenames reported both from a full symtab _and_ from a (stale) previously cached index entry. Now, obviously, when seeing a problem like this the "correct" fix is to remove the stale entries from the cache, however, I ran a few experiments to see why this wasn't really hitting us anywhere, and, as far as I can tell, ::map_symbol_filenames is only called from three places: 1. The mi command -file-list-exec-source-files, 2. The 'info sources' command, and 3. Filename completion However, the result of this "bug" is that we will see duplicate filenames, and readline's completion mechanism already removes duplicates, so for case #3 we will never see any problems. Cases #1 and #2 are basically the same, and in each case, to see a problem we need to ensure we craft the test in a particular way, start up ensuring we have some unexpected symtabs, then run one of the commands to populate the cache, then expand one of the symtabs, and list the sources again. At this point you'll see duplicate entries in the results. Hardly surprising we haven't randomly hit this situation in testing. So, considering that use cases #1 and #2 are certainly not "high performance" code (i.e. I don't think these justify the need for caching) this leaves use case #3. Does this use justify the need for caching? Well the psymbol_functions::map_symbol_filenames function doesn't seem to do any extra caching, and within dwarf2_base_index_functions::map_symbol_filenames, the only expensive bit appears to be the call to dw2_get_file_names, and this already does its own caching via this_cu->v.quick->file_names. The upshot of all this analysis was that I'm not convinced the need for the additional caching is justified, and so, I propose that to fix the bug in GDB, I just remove the extra caching (for now). If we later find that the caching _was_ useful, then we can reintroduce it, but add it back such that it doesn't reintroduce this bug. As I was changing dwarf2_base_index_functions::map_symbol_filenames I replaced the use of htab_up with std::unordered_set. Tested using target_boards cc-with-debug-names and dwarf4-gdb-index. gdb/ChangeLog: * dwarf2/read.c: Add 'unordered_set' include. (dwarf2_base_index_functions::map_symbol_filenames): Replace 'visited' hash table with 'qfn_cache' unordered_set. Remove use of per_Bfd->filenames_cache cache, and use function local filenames_cache instead. Reindent. * dwarf2/read.h (struct dwarf2_per_bfd) : Delete. gdb/testsuite/ChangeLog: * gdb.base/info_sources.exp: Add new tests. --- gdb/ChangeLog | 9 ++++ gdb/dwarf2/read.c | 71 ++++++++++--------------- gdb/dwarf2/read.h | 4 -- gdb/testsuite/ChangeLog | 4 ++ gdb/testsuite/gdb.base/info_sources.exp | 17 +++--- 5 files changed, 53 insertions(+), 52 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 7e4b0bd95db..5013d730f35 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,12 @@ +2021-04-23 Andrew Burgess + + * dwarf2/read.c: Add 'unordered_set' include. + (dwarf2_base_index_functions::map_symbol_filenames): Replace + 'visited' hash table with 'qfn_cache' unordered_set. Remove use + of per_Bfd->filenames_cache cache, and use function local + filenames_cache instead. Reindent. + * dwarf2/read.h (struct dwarf2_per_bfd) : Delete. + 2021-04-22 Simon Marchi * breakpoint.c (iterate_over_bp_locations): Change callback to diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 40aeb298c14..306971ef1b7 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -88,6 +88,7 @@ #include "rust-lang.h" #include "gdbsupport/pathstuff.h" #include "count-one-bits.h" +#include /* When == 1, print basic high level tracing messages. When > 1, be more verbose. @@ -4713,58 +4714,44 @@ dwarf2_base_index_functions::map_symbol_filenames { dwarf2_per_objfile *per_objfile = get_dwarf2_per_objfile (objfile); - if (!per_objfile->per_bfd->filenames_cache) - { - per_objfile->per_bfd->filenames_cache.emplace (); - - htab_up visited (htab_create_alloc (10, - htab_hash_pointer, htab_eq_pointer, - NULL, xcalloc, xfree)); + /* Use caches to ensure we only call FUN once for each filename. */ + filename_seen_cache filenames_cache; + std::unordered_set qfn_cache; - /* The rule is CUs specify all the files, including those used - by any TU, so there's no need to scan TUs here. We can - ignore file names coming from already-expanded CUs. */ + /* The rule is CUs specify all the files, including those used by any TU, + so there's no need to scan TUs here. We can ignore file names coming + from already-expanded CUs. It is possible that an expanded CU might + reuse the file names data from a currently unexpanded CU, in this + case we don't want to report the files from the unexpanded CU. */ - for (dwarf2_per_cu_data *per_cu : per_objfile->per_bfd->all_comp_units) + for (dwarf2_per_cu_data *per_cu : per_objfile->per_bfd->all_comp_units) + { + if (per_objfile->symtab_set_p (per_cu)) { - if (per_objfile->symtab_set_p (per_cu)) - { - void **slot = htab_find_slot (visited.get (), - per_cu->v.quick->file_names, - INSERT); - - *slot = per_cu->v.quick->file_names; - } + if (per_cu->v.quick->file_names != nullptr) + qfn_cache.insert (per_cu->v.quick->file_names); } + } - for (dwarf2_per_cu_data *per_cu : per_objfile->per_bfd->all_comp_units) - { - /* We only need to look at symtabs not already expanded. */ - if (per_objfile->symtab_set_p (per_cu)) - continue; - - quick_file_names *file_data - = dw2_get_file_names (per_cu, per_objfile); - if (file_data == NULL) - continue; + for (dwarf2_per_cu_data *per_cu : per_objfile->per_bfd->all_comp_units) + { + /* We only need to look at symtabs not already expanded. */ + if (per_objfile->symtab_set_p (per_cu)) + continue; - void **slot = htab_find_slot (visited.get (), file_data, INSERT); - if (*slot) - { - /* Already visited. */ - continue; - } - *slot = file_data; + quick_file_names *file_data = dw2_get_file_names (per_cu, per_objfile); + if (file_data == nullptr + || qfn_cache.find (file_data) != qfn_cache.end ()) + continue; - for (int j = 0; j < file_data->num_file_names; ++j) - { - const char *filename = file_data->file_names[j]; - per_objfile->per_bfd->filenames_cache->seen (filename); - } + for (int j = 0; j < file_data->num_file_names; ++j) + { + const char *filename = file_data->file_names[j]; + filenames_cache.seen (filename); } } - per_objfile->per_bfd->filenames_cache->traverse ([&] (const char *filename) + filenames_cache.traverse ([&] (const char *filename) { gdb::unique_xmalloc_ptr this_real_name; diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h index 4dea86f841d..dc25ff16f51 100644 --- a/gdb/dwarf2/read.h +++ b/gdb/dwarf2/read.h @@ -235,10 +235,6 @@ public: /* The CUs we recently read. */ std::vector just_read_cus; - /* Table containing all filenames. This is an optional because the - table is lazily constructed on first access. */ - gdb::optional filenames_cache; - /* If we loaded the index from an external file, this contains the resources associated to the open file, memory mapping, etc. */ std::unique_ptr index_cache_res; diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 1a181f37475..73f95b9a719 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,7 @@ +2021-04-23 Andrew Burgess + + * gdb.base/info_sources.exp: Add new tests. + 2021-04-22 Tom Tromey * gdb.base/ptype-offsets.cc (struct empty_member): New. diff --git a/gdb/testsuite/gdb.base/info_sources.exp b/gdb/testsuite/gdb.base/info_sources.exp index 81edb3da8d7..a4f7d1966e6 100644 --- a/gdb/testsuite/gdb.base/info_sources.exp +++ b/gdb/testsuite/gdb.base/info_sources.exp @@ -28,18 +28,18 @@ if {[prepare_for_testing $testfile.exp $testfile \ # in the output. Similarly, EXPECT_SEEN_INFO_SOURCES_BASE indicates that the source file # info_sources_base.c must be seen in the output. proc test_info_sources {args expect_seen_info_sources expect_seen_info_sources_base} { - global gdb_prompt + global gdb_prompt srcfile srcfile2 set seen_info_sources 0 set seen_info_sources_base 0 set cmd [concat "info sources " $args] gdb_test_multiple $cmd $cmd { - -re "^\[^,\]*info_sources.c(, |\[\r\n\]+)" { - set seen_info_sources 1 + -re "^\[^,\]*${srcfile}(, |\[\r\n\]+)" { + incr seen_info_sources exp_continue } - -re "^\[^,\]*info_sources_base.c(, |\[\r\n\]+)" { - set seen_info_sources_base 1 + -re "^\[^,\]*${srcfile2}(, |\[\r\n\]+)" { + incr seen_info_sources_base 1 exp_continue } -re ", " { @@ -60,8 +60,13 @@ if ![runto_main] { untested $testfile.exp return -1 } -gdb_test "break some_other_func" "" +# List both files with no regexp: +with_test_prefix "in main" { + test_info_sources "" 1 1 +} + +gdb_test "break some_other_func" "" gdb_test "continue" # List both files with no regexp: -- 2.30.2