* [PATCH][gdb/symtab] Don't create duplicate psymtab for forward-imported CU
@ 2020-04-08 10:19 Tom de Vries
2020-04-08 10:22 ` Tom de Vries
0 siblings, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2020-04-08 10:19 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
Hi,
Consider the executable generated for test-case gdb.dwarf2/imported-unit.exp.
When loading the executable using various tracing:
...
$ gdb \
outputs/gdb.dwarf2/imported-unit/imported-unit \
-batch \
-iex "set verbose on" \
-iex "set debug symtab-create 1"
...
Created psymtab 0x213f380 for module <artificial>@0xc7.
Created psymtab 0x20e7b00 for module imported_unit.c.
Created psymtab 0x215da20 for module imported_unit.c.
Created psymtab 0x2133630 for module elf-init.c.
Created psymtab 0x215b910 for module ../sysdeps/x86_64/crtn.S.
...
we notice that there are two psymtabs generated for imported_unit.c.
This is due to the following: in dwarf2_build_psymtabs_hard we loop over CUs
and generate partial symtabs for those, and if we encounter an import of
another CU, we also generate a partial symtab for that one, unless already
created.
This works well with backward import references:
- the imported CU is read
- then the importing CU is read
- the import is encountered, but the imported CU is already read, so
we're done.
But with forward import references, we have instead:
- the importing CU is read
- the import is encountered, and the imported CU is read
- the imported CU is read once more
Fix this by skipping already created psymtabs in the loop in
dwarf2_build_psymtabs_hard.
Build in combination with the tentative fix for PR25718 - "[ada] symbol in
imported unit not found"
( https://sourceware.org/pipermail/gdb-patches/2020-March/167152.html ) to
prevent this regression:
...
FAIL: gdb.ada/char_enum.exp: ptype pck.Global_Enum_Type
...
Tested on x86_64-linux, with native and target board
unix/-flto/-O0/-flto-partition=none/-ffat-lto-objects.
This causes this regression with the target board:
...
FAIL: gdb.ada/dgopt.exp: list x.adb:16, 16
...
which I consider a seperate PR, filed as PR25801 - "Filename of shared psymtab
is ignored".
OK for trunk?
Thanks,
- Tom
[gdb/symtab] Don't create duplicate psymtab for forward-imported CU
gdb/ChangeLog:
2020-04-08 Tom de Vries <tdevries@suse.de>
PR symtab/25700
* dwarf2/read.c (dwarf2_build_psymtabs_hard): Don't create psymtab for
CU if already created.
gdb/testsuite/ChangeLog:
2020-04-08 Tom de Vries <tdevries@suse.de>
PR symtab/25700
* gdb.dwarf2/imported-unit.exp: Verify that there's only one partial
symtab for imported_unit.c.
---
gdb/dwarf2/read.c | 7 ++++++-
gdb/testsuite/gdb.dwarf2/imported-unit.exp | 18 ++++++++++++++++++
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index c0b5e58b17..8b761e891e 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -7781,7 +7781,12 @@ dwarf2_build_psymtabs_hard (struct dwarf2_per_objfile *dwarf2_per_objfile)
addrmap_create_mutable (&temp_obstack));
for (dwarf2_per_cu_data *per_cu : dwarf2_per_objfile->all_comp_units)
- process_psymtab_comp_unit (per_cu, false, language_minimal);
+ {
+ if (per_cu->v.psymtab != NULL)
+ /* In case a forward DW_TAG_imported_unit has read the CU already. */
+ continue;
+ process_psymtab_comp_unit (per_cu, false, language_minimal);
+ }
/* This has to wait until we read the CUs, we need the list of DWOs. */
process_skeletonless_type_units (dwarf2_per_objfile);
diff --git a/gdb/testsuite/gdb.dwarf2/imported-unit.exp b/gdb/testsuite/gdb.dwarf2/imported-unit.exp
index 41a7505459..d7b3d4c539 100644
--- a/gdb/testsuite/gdb.dwarf2/imported-unit.exp
+++ b/gdb/testsuite/gdb.dwarf2/imported-unit.exp
@@ -168,6 +168,24 @@ if { $psymtabs_p } {
} else {
unsupported $test
}
+
+# Verify that there's only one partial symtab for imported_unit.c. Test-case
+# for PR25700.
+set test "no duplicate psymtab for imported_unit.c"
+if { $psymtabs_p } {
+ set line "Partial symtab for source file imported_unit.c"
+ gdb_test_multiple "maint print psymbols" $test {
+ -re -wrap "$line.*$line.*" {
+ fail $gdb_test_name
+ }
+ -re -wrap "$line.*" {
+ pass $gdb_test_name
+ }
+ }
+} else {
+ unsupported $test
+}
+
# Sanity check
gdb_test "ptype main" "= int \\(void\\)"
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][gdb/symtab] Don't create duplicate psymtab for forward-imported CU
2020-04-08 10:19 [PATCH][gdb/symtab] Don't create duplicate psymtab for forward-imported CU Tom de Vries
@ 2020-04-08 10:22 ` Tom de Vries
2020-04-22 6:13 ` Tom de Vries
0 siblings, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2020-04-08 10:22 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
On 08-04-2020 12:19, Tom de Vries wrote:
> Hi,
>
> Consider the executable generated for test-case gdb.dwarf2/imported-unit.exp.
>
> When loading the executable using various tracing:
> ...
> $ gdb \
> outputs/gdb.dwarf2/imported-unit/imported-unit \
> -batch \
> -iex "set verbose on" \
> -iex "set debug symtab-create 1"
> ...
> Created psymtab 0x213f380 for module <artificial>@0xc7.
> Created psymtab 0x20e7b00 for module imported_unit.c.
> Created psymtab 0x215da20 for module imported_unit.c.
> Created psymtab 0x2133630 for module elf-init.c.
> Created psymtab 0x215b910 for module ../sysdeps/x86_64/crtn.S.
> ...
> we notice that there are two psymtabs generated for imported_unit.c.
>
> This is due to the following: in dwarf2_build_psymtabs_hard we loop over CUs
> and generate partial symtabs for those, and if we encounter an import of
> another CU, we also generate a partial symtab for that one, unless already
> created.
>
> This works well with backward import references:
> - the imported CU is read
> - then the importing CU is read
> - the import is encountered, but the imported CU is already read, so
> we're done.
>
> But with forward import references, we have instead:
> - the importing CU is read
> - the import is encountered, and the imported CU is read
> - the imported CU is read once more
>
> Fix this by skipping already created psymtabs in the loop in
> dwarf2_build_psymtabs_hard.
>
> Build in combination with the tentative fix for PR25718 - "[ada] symbol in
> imported unit not found"
> ( https://sourceware.org/pipermail/gdb-patches/2020-March/167152.html ) to
> prevent this regression:
> ...
> FAIL: gdb.ada/char_enum.exp: ptype pck.Global_Enum_Type
> ...
>
> Tested on x86_64-linux, with native and target board
> unix/-flto/-O0/-flto-partition=none/-ffat-lto-objects.
>
> This causes this regression with the target board:
> ...
> FAIL: gdb.ada/dgopt.exp: list x.adb:16, 16
> ...
> which I consider a seperate PR, filed as PR25801 - "Filename of shared psymtab
> is ignored".
>
[ Forgot to mention ]
Using a 2.8GB libxul.so debug file from opensuse tumbleweed, build with
lto, we have:
...
$ time.sh gdb -batch -iex "maint set dwarf max-cache-age 1000" -iex "set
language c" usr/lib/debug/usr/lib64/firefox/libxul.so-74.0-1.1.x86_64.debug
maxmem: 10417196
real: 116.62
user: 114.85
system: 2.37
...
and with this patch:
...
$ time.sh gdb -batch -iex "maint set dwarf max-cache-age 1000" -iex "set
language c" usr/lib/debug/usr/lib64/firefox/libxul.so-74.0-1.1.x86_64.debug
maxmem: 10436372
real: 77.42
user: 75.54
system: 2.46
OK for trunk?
Thanks,
- Tom
> [gdb/symtab] Don't create duplicate psymtab for forward-imported CU
>
> gdb/ChangeLog:
>
> 2020-04-08 Tom de Vries <tdevries@suse.de>
>
> PR symtab/25700
> * dwarf2/read.c (dwarf2_build_psymtabs_hard): Don't create psymtab for
> CU if already created.
>
> gdb/testsuite/ChangeLog:
>
> 2020-04-08 Tom de Vries <tdevries@suse.de>
>
> PR symtab/25700
> * gdb.dwarf2/imported-unit.exp: Verify that there's only one partial
> symtab for imported_unit.c.
>
> ---
> gdb/dwarf2/read.c | 7 ++++++-
> gdb/testsuite/gdb.dwarf2/imported-unit.exp | 18 ++++++++++++++++++
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index c0b5e58b17..8b761e891e 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -7781,7 +7781,12 @@ dwarf2_build_psymtabs_hard (struct dwarf2_per_objfile *dwarf2_per_objfile)
> addrmap_create_mutable (&temp_obstack));
>
> for (dwarf2_per_cu_data *per_cu : dwarf2_per_objfile->all_comp_units)
> - process_psymtab_comp_unit (per_cu, false, language_minimal);
> + {
> + if (per_cu->v.psymtab != NULL)
> + /* In case a forward DW_TAG_imported_unit has read the CU already. */
> + continue;
> + process_psymtab_comp_unit (per_cu, false, language_minimal);
> + }
>
> /* This has to wait until we read the CUs, we need the list of DWOs. */
> process_skeletonless_type_units (dwarf2_per_objfile);
> diff --git a/gdb/testsuite/gdb.dwarf2/imported-unit.exp b/gdb/testsuite/gdb.dwarf2/imported-unit.exp
> index 41a7505459..d7b3d4c539 100644
> --- a/gdb/testsuite/gdb.dwarf2/imported-unit.exp
> +++ b/gdb/testsuite/gdb.dwarf2/imported-unit.exp
> @@ -168,6 +168,24 @@ if { $psymtabs_p } {
> } else {
> unsupported $test
> }
> +
> +# Verify that there's only one partial symtab for imported_unit.c. Test-case
> +# for PR25700.
> +set test "no duplicate psymtab for imported_unit.c"
> +if { $psymtabs_p } {
> + set line "Partial symtab for source file imported_unit.c"
> + gdb_test_multiple "maint print psymbols" $test {
> + -re -wrap "$line.*$line.*" {
> + fail $gdb_test_name
> + }
> + -re -wrap "$line.*" {
> + pass $gdb_test_name
> + }
> + }
> +} else {
> + unsupported $test
> +}
> +
> # Sanity check
> gdb_test "ptype main" "= int \\(void\\)"
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][gdb/symtab] Don't create duplicate psymtab for forward-imported CU
2020-04-08 10:22 ` Tom de Vries
@ 2020-04-22 6:13 ` Tom de Vries
2020-04-22 13:45 ` Simon Marchi
0 siblings, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2020-04-22 6:13 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
On 08-04-2020 12:22, Tom de Vries wrote:
> On 08-04-2020 12:19, Tom de Vries wrote:
>> Hi,
>>
>> Consider the executable generated for test-case gdb.dwarf2/imported-unit.exp.
>>
>> When loading the executable using various tracing:
>> ...
>> $ gdb \
>> outputs/gdb.dwarf2/imported-unit/imported-unit \
>> -batch \
>> -iex "set verbose on" \
>> -iex "set debug symtab-create 1"
>> ...
>> Created psymtab 0x213f380 for module <artificial>@0xc7.
>> Created psymtab 0x20e7b00 for module imported_unit.c.
>> Created psymtab 0x215da20 for module imported_unit.c.
>> Created psymtab 0x2133630 for module elf-init.c.
>> Created psymtab 0x215b910 for module ../sysdeps/x86_64/crtn.S.
>> ...
>> we notice that there are two psymtabs generated for imported_unit.c.
>>
>> This is due to the following: in dwarf2_build_psymtabs_hard we loop over CUs
>> and generate partial symtabs for those, and if we encounter an import of
>> another CU, we also generate a partial symtab for that one, unless already
>> created.
>>
>> This works well with backward import references:
>> - the imported CU is read
>> - then the importing CU is read
>> - the import is encountered, but the imported CU is already read, so
>> we're done.
>>
>> But with forward import references, we have instead:
>> - the importing CU is read
>> - the import is encountered, and the imported CU is read
>> - the imported CU is read once more
>>
>> Fix this by skipping already created psymtabs in the loop in
>> dwarf2_build_psymtabs_hard.
>>
>> Build in combination with the tentative fix for PR25718 - "[ada] symbol in
>> imported unit not found"
>> ( https://sourceware.org/pipermail/gdb-patches/2020-March/167152.html ) to
>> prevent this regression:
>> ...
>> FAIL: gdb.ada/char_enum.exp: ptype pck.Global_Enum_Type
>> ...
>>
>> Tested on x86_64-linux, with native and target board
>> unix/-flto/-O0/-flto-partition=none/-ffat-lto-objects.
>>
>> This causes this regression with the target board:
>> ...
>> FAIL: gdb.ada/dgopt.exp: list x.adb:16, 16
>> ...
>> which I consider a seperate PR, filed as PR25801 - "Filename of shared psymtab
>> is ignored".
>>
>
> [ Forgot to mention ]
>
> Using a 2.8GB libxul.so debug file from opensuse tumbleweed, build with
> lto, we have:
> ...
> $ time.sh gdb -batch -iex "maint set dwarf max-cache-age 1000" -iex "set
> language c" usr/lib/debug/usr/lib64/firefox/libxul.so-74.0-1.1.x86_64.debug
> maxmem: 10417196
> real: 116.62
> user: 114.85
> system: 2.37
> ...
> and with this patch:
> ...
> $ time.sh gdb -batch -iex "maint set dwarf max-cache-age 1000" -iex "set
> language c" usr/lib/debug/usr/lib64/firefox/libxul.so-74.0-1.1.x86_64.debug
> maxmem: 10436372
> real: 77.42
> user: 75.54
> system: 2.46
>
> OK for trunk?
>
Committed (
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=3d5afab3393064e563305bc27264fde5a7598635
).
Thanks,
- Tom
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][gdb/symtab] Don't create duplicate psymtab for forward-imported CU
2020-04-22 6:13 ` Tom de Vries
@ 2020-04-22 13:45 ` Simon Marchi
2020-04-22 13:55 ` Tom de Vries
0 siblings, 1 reply; 6+ messages in thread
From: Simon Marchi @ 2020-04-22 13:45 UTC (permalink / raw)
To: Tom de Vries, gdb-patches; +Cc: Tom Tromey
On 2020-04-22 2:13 a.m., Tom de Vries wrote:
>> [ Forgot to mention ]
>>
>> Using a 2.8GB libxul.so debug file from opensuse tumbleweed, build with
>> lto, we have:
>> ...
>> $ time.sh gdb -batch -iex "maint set dwarf max-cache-age 1000" -iex "set
>> language c" usr/lib/debug/usr/lib64/firefox/libxul.so-74.0-1.1.x86_64.debug
>> maxmem: 10417196
>> real: 116.62
>> user: 114.85
>> system: 2.37
>> ...
>> and with this patch:
>> ...
>> $ time.sh gdb -batch -iex "maint set dwarf max-cache-age 1000" -iex "set
>> language c" usr/lib/debug/usr/lib64/firefox/libxul.so-74.0-1.1.x86_64.debug
>> maxmem: 10436372
>> real: 77.42
>> user: 75.54
>> system: 2.46
>>
>> OK for trunk?
I just glanced at this and... wow, that's a big difference.
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][gdb/symtab] Don't create duplicate psymtab for forward-imported CU
2020-04-22 13:45 ` Simon Marchi
@ 2020-04-22 13:55 ` Tom de Vries
2020-05-27 15:09 ` Tom Tromey
0 siblings, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2020-04-22 13:55 UTC (permalink / raw)
To: Simon Marchi, gdb-patches; +Cc: Tom Tromey
On 22-04-2020 15:45, Simon Marchi wrote:
> On 2020-04-22 2:13 a.m., Tom de Vries wrote:
>>> [ Forgot to mention ]
>>>
>>> Using a 2.8GB libxul.so debug file from opensuse tumbleweed, build with
>>> lto, we have:
>>> ...
>>> $ time.sh gdb -batch -iex "maint set dwarf max-cache-age 1000" -iex "set
>>> language c" usr/lib/debug/usr/lib64/firefox/libxul.so-74.0-1.1.x86_64.debug
>>> maxmem: 10417196
>>> real: 116.62
>>> user: 114.85
>>> system: 2.37
>>> ...
>>> and with this patch:
>>> ...
>>> $ time.sh gdb -batch -iex "maint set dwarf max-cache-age 1000" -iex "set
>>> language c" usr/lib/debug/usr/lib64/firefox/libxul.so-74.0-1.1.x86_64.debug
>>> maxmem: 10436372
>>> real: 77.42
>>> user: 75.54
>>> system: 2.46
>>>
>>> OK for trunk?
>
> I just glanced at this and... wow, that's a big difference.
>
Yeah :)
Not as big though as the difference of changing dwarf max-cache-age from
default 5 to 1000 (
https://sourceware.org/bugzilla/show_bug.cgi?id=25703#c1 )
Thanks,
- Tom
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][gdb/symtab] Don't create duplicate psymtab for forward-imported CU
2020-04-22 13:55 ` Tom de Vries
@ 2020-05-27 15:09 ` Tom Tromey
0 siblings, 0 replies; 6+ messages in thread
From: Tom Tromey @ 2020-05-27 15:09 UTC (permalink / raw)
To: Tom de Vries; +Cc: Simon Marchi, gdb-patches, Tom Tromey
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
Tom> Not as big though as the difference of changing dwarf max-cache-age from
Tom> default 5 to 1000 (
Tom> https://sourceware.org/bugzilla/show_bug.cgi?id=25703#c1 )
Maybe this number should default to being relative to available memory
rather than just be an arbitrary constant. I suspect that when "5" was
chosen, inter-CU references weren't common; and I'm certain this
predates the existence of dwz.
Tom
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-27 15:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-08 10:19 [PATCH][gdb/symtab] Don't create duplicate psymtab for forward-imported CU Tom de Vries
2020-04-08 10:22 ` Tom de Vries
2020-04-22 6:13 ` Tom de Vries
2020-04-22 13:45 ` Simon Marchi
2020-04-22 13:55 ` Tom de Vries
2020-05-27 15:09 ` Tom Tromey
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).