[PATCH][gdb/symtab] Don't create duplicate psymtab for forward-imported CU

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH][gdb/symtab] Don't create duplicate psymtab for forward-imported CU

Tom de Vries
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  <[hidden email]>

        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  <[hidden email]>

        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\\)"
 
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][gdb/symtab] Don't create duplicate psymtab for forward-imported CU

Tom de Vries
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  <[hidden email]>
>
> 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  <[hidden email]>
>
> 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\\)"
>  
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][gdb/symtab] Don't create duplicate psymtab for forward-imported CU

Tom de Vries
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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][gdb/symtab] Don't create duplicate psymtab for forward-imported CU

Simon Marchi-4
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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][gdb/symtab] Don't create duplicate psymtab for forward-imported CU

Tom de Vries
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