* Change in binutils-gdb[master]: [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
@ 2019-10-14 16:35 ` Tom de Vries (Code Review)
2019-10-14 16:51 ` Tom de Vries (Code Review)
` (19 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Tom de Vries (Code Review) @ 2019-10-14 16:35 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
Hello Andrew Burgess,
I'd like you to reexamine a change. Please visit
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
to look at the new patch set (#2).
Change subject: [gdb/symtab] Prefer var def over decl
......................................................................
[gdb/symtab] Prefer var def over decl
Consider the DWARF as generated by gcc with the tentative patch to fix gcc
PR91507 - "wrong debug for completed array with previous incomplete
declaration":
...
<1><f4>: Abbrev Number: 2 (DW_TAG_array_type)
<f5> DW_AT_type : <0xff>
<f9> DW_AT_sibling : <0xff>
<2><fd>: Abbrev Number: 3 (DW_TAG_subrange_type)
<2><fe>: Abbrev Number: 0
<1><ff>: Abbrev Number: 4 (DW_TAG_pointer_type)
<100> DW_AT_byte_size : 8
<101> DW_AT_type : <0x105>
<1><105>: Abbrev Number: 5 (DW_TAG_base_type)
<106> DW_AT_byte_size : 1
<107> DW_AT_encoding : 6 (signed char)
<108> DW_AT_name : (indirect string, offset: 0x19f): char
<1><10c>: Abbrev Number: 6 (DW_TAG_variable)
<10d> DW_AT_name : zzz
<111> DW_AT_decl_file : 1
<112> DW_AT_decl_line : 1
<113> DW_AT_decl_column : 14
<114> DW_AT_type : <0xf4>
<118> DW_AT_external : 1
<118> DW_AT_declaration : 1
<1><118>: Abbrev Number: 2 (DW_TAG_array_type)
<119> DW_AT_type : <0xff>
<11d> DW_AT_sibling : <0x128>
<1><12f>: Abbrev Number: 8 (DW_TAG_variable)
<130> DW_AT_specification: <0x10c>
<134> DW_AT_decl_line : 2
<135> DW_AT_decl_column : 7
<136> DW_AT_type : <0x118>
<13a> DW_AT_location : 9 byte block: 3 30 10 60 0 0 0 0 0 (DW_OP_addr: 601030)
...
The DWARF will result in two entries in the symbol table, a decl with type
char *[] and a def with type char*[2].
When trying to print the value of zzz:
...
$ gdb a.spec.out -batch -ex "p zzz"
...
the decl (rather than the def) will be found in the symbol table, which is
missing the location information, and consequently we get:
...
$1 = 0x601030 <zzz>
...
[ There is a fallback mechanism that finds the address of the variable in the
minimal symbol table, but that's not used here, because the type of the decl
does not specify a size. We could use the symbol size here to get the size
of the type, but that's currently not done: PR exp/24989. Still, fixing that
PR would not fix the generic case, where minimal symbol info is not
available. ]
Fix this by preferring defs over decls when searching in the symbol table.
Build and reg-tested on x86_64-linux.
[ The test-case is a bit simpler than the DWARF example listed above, because
the new variable varval3 that is used is not listed in the minimal symbols, so
there's no need to work around the fallback mechanism to trigger the problem. ]
gdb/ChangeLog:
2019-09-10 Tom de Vries <tdevries@suse.de>
PR symtab/24971
* block.c (best_symbol, better_symbol): New function.
(block_lookup_symbol_primary): Prefer def over decl.
gdb/testsuite/ChangeLog:
2019-09-10 Tom de Vries <tdevries@suse.de>
* gdb.dwarf2/varval.exp: Add decl before def test.
Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
---
M gdb/block.c
M gdb/testsuite/gdb.dwarf2/varval.exp
2 files changed, 55 insertions(+), 3 deletions(-)
git pull ssh://gnutoolchain-gerrit.osci.io:29418/binutils-gdb refs/changes/29/29/2
--
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 2
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-MessageType: newpatchset
^ permalink raw reply [flat|nested] 21+ messages in thread* Change in binutils-gdb[master]: [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
2019-10-14 16:35 ` Change in binutils-gdb[master]: [gdb/symtab] Prefer var def over decl Tom de Vries (Code Review)
@ 2019-10-14 16:51 ` Tom de Vries (Code Review)
2019-10-18 13:31 ` [review] " Luis Machado (Code Review)
` (18 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Tom de Vries (Code Review) @ 2019-10-14 16:51 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
Tom de Vries has posted comments on this change. ( https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29 )
Change subject: [gdb/symtab] Prefer var def over decl
......................................................................
Patch Set 2:
(2 comments)
Added missing function comments.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29/1/gdb/block.c
File gdb/block.c:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29/1/gdb/block.c@729
PS1, Line 729: best_symbol (struct symbol *a, const domain_enum domain)
> All functions should have a header comment: https://sourceware. […]
Done
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29/1/gdb/block.c@736
PS1, Line 736: better_symbol (struct symbol *a, struct symbol *b, const domain_enum domain)
> Again, needs a comment.
Done
--
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 2
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-Comment-Date: Mon, 14 Oct 2019 16:50:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 21+ messages in thread* [review] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
2019-10-14 16:35 ` Change in binutils-gdb[master]: [gdb/symtab] Prefer var def over decl Tom de Vries (Code Review)
2019-10-14 16:51 ` Tom de Vries (Code Review)
@ 2019-10-18 13:31 ` Luis Machado (Code Review)
2019-10-18 16:44 ` Tom de Vries (Code Review)
` (17 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Luis Machado (Code Review) @ 2019-10-18 13:31 UTC (permalink / raw)
To: Tom de Vries, gdb-patches; +Cc: Andrew Burgess
Luis Machado has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
Patch Set 2: Code-Review+1
(2 comments)
I've noticed a couple nits.
If PR91507 is fixed, do we need to patch GDB up? Or do we want to adapt GDB to the broken output from older GCC as well?
I'm inclined to at least add some comment to the code stating GCC's broken behavior and why we're fixing it in this particular way.
Otherwise this looks good to me.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29/2/gdb/block.c
File gdb/block.c:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29/2/gdb/block.c@737
PS2, Line 737: /* Return true if symbol A is a better match than symbol B for DOMAIN. */
The comment is a bit misleading. Maybe "Return symbol A if it is a better match than symbol B for DOMAIN. Otherwise return B."
It seems the default, if everything fails, is to return A? Maybe add that to the comment as well.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29/2/gdb/testsuite/gdb.dwarf2/varval.exp
File gdb/testsuite/gdb.dwarf2/varval.exp:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29/2/gdb/testsuite/gdb.dwarf2/varval.exp@209
PS2, Line 209:
Spurious new line?
^ permalink raw reply [flat|nested] 21+ messages in thread* [review] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (2 preceding siblings ...)
2019-10-18 13:31 ` [review] " Luis Machado (Code Review)
@ 2019-10-18 16:44 ` Tom de Vries (Code Review)
2019-10-23 16:27 ` [review v3] " Tom de Vries (Code Review)
` (16 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Tom de Vries (Code Review) @ 2019-10-18 16:44 UTC (permalink / raw)
To: gdb-patches; +Cc: Luis Machado, Andrew Burgess
Tom de Vries has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
Patch Set 2:
> Patch Set 2: Code-Review+1
>
> (2 comments)
>
> I've noticed a couple nits.
>
> If PR91507 is fixed, do we need to patch GDB up?
Yes. We need both the fix of PR91507 and this GDB patch to fix the test-case.
> Or do we want to adapt GDB to the broken output from older GCC as well?
As mentioned in the rationale, that is possible by getting the size from minimal symbol info, but I consider that a workaround rather than a fix. I've filed that as a separate PR: https://sourceware.org/bugzilla/show_bug.cgi?id=24989 .
> I'm inclined to at least add some comment to the code stating GCC's broken behavior and why we're fixing it in this particular way.
>
Ack, will add.
> Otherwise this looks good to me.
^ permalink raw reply [flat|nested] 21+ messages in thread* [review v3] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (3 preceding siblings ...)
2019-10-18 16:44 ` Tom de Vries (Code Review)
@ 2019-10-23 16:27 ` Tom de Vries (Code Review)
2019-10-23 16:29 ` Tom de Vries (Code Review)
` (15 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Tom de Vries (Code Review) @ 2019-10-23 16:27 UTC (permalink / raw)
To: Luis Machado, Andrew Burgess, gdb-patches
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
[gdb/symtab] Prefer var def over decl
Consider the DWARF as generated by gcc with the tentative patch to fix gcc
PR91507 - "wrong debug for completed array with previous incomplete
declaration":
...
<1><f4>: Abbrev Number: 2 (DW_TAG_array_type)
<f5> DW_AT_type : <0xff>
<f9> DW_AT_sibling : <0xff>
<2><fd>: Abbrev Number: 3 (DW_TAG_subrange_type)
<2><fe>: Abbrev Number: 0
<1><ff>: Abbrev Number: 4 (DW_TAG_pointer_type)
<100> DW_AT_byte_size : 8
<101> DW_AT_type : <0x105>
<1><105>: Abbrev Number: 5 (DW_TAG_base_type)
<106> DW_AT_byte_size : 1
<107> DW_AT_encoding : 6 (signed char)
<108> DW_AT_name : (indirect string, offset: 0x19f): char
<1><10c>: Abbrev Number: 6 (DW_TAG_variable)
<10d> DW_AT_name : zzz
<111> DW_AT_decl_file : 1
<112> DW_AT_decl_line : 1
<113> DW_AT_decl_column : 14
<114> DW_AT_type : <0xf4>
<118> DW_AT_external : 1
<118> DW_AT_declaration : 1
<1><118>: Abbrev Number: 2 (DW_TAG_array_type)
<119> DW_AT_type : <0xff>
<11d> DW_AT_sibling : <0x128>
<1><12f>: Abbrev Number: 8 (DW_TAG_variable)
<130> DW_AT_specification: <0x10c>
<134> DW_AT_decl_line : 2
<135> DW_AT_decl_column : 7
<136> DW_AT_type : <0x118>
<13a> DW_AT_location : 9 byte block: 3 30 10 60 0 0 0 0 0 (DW_OP_addr: 601030)
...
The DWARF will result in two entries in the symbol table, a decl with type
char *[] and a def with type char*[2].
When trying to print the value of zzz:
...
$ gdb a.spec.out -batch -ex "p zzz"
...
the decl (rather than the def) will be found in the symbol table, which is
missing the location information, and consequently we get:
...
$1 = 0x601030 <zzz>
...
[ There is a fallback mechanism that finds the address of the variable in the
minimal symbol table, but that's not used here, because the type of the decl
does not specify a size. We could use the symbol size here to get the size
of the type, but that's currently not done: PR exp/24989. Still, fixing that
PR would not fix the generic case, where minimal symbol info is not
available. ]
Fix this by preferring defs over decls when searching in the symbol table.
Build and reg-tested on x86_64-linux.
[ The test-case is a bit simpler than the DWARF example listed above, because
the new variable varval3 that is used is not listed in the minimal symbols, so
there's no need to work around the fallback mechanism to trigger the problem. ]
gdb/ChangeLog:
2019-09-10 Tom de Vries <tdevries@suse.de>
PR symtab/24971
* block.c (best_symbol, better_symbol): New function.
(block_lookup_symbol_primary): Prefer def over decl.
gdb/testsuite/ChangeLog:
2019-09-10 Tom de Vries <tdevries@suse.de>
* gdb.dwarf2/varval.exp: Add decl before def test.
Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
---
M gdb/block.c
M gdb/testsuite/gdb.dwarf2/varval.exp
2 files changed, 81 insertions(+), 3 deletions(-)
diff --git a/gdb/block.c b/gdb/block.c
index 5ba44d4..9608194 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -725,6 +725,43 @@
}
}
+/* Return true if symbol A is the best match possible for DOMAIN. */
+
+static bool
+best_symbol (struct symbol *a, const domain_enum domain)
+{
+ return (SYMBOL_DOMAIN (a) == domain
+ && SYMBOL_CLASS (a) != LOC_UNRESOLVED);
+}
+
+/* Return symbol B if it is a better match than symbol A for DOMAIN.
+ Otherwise return A. */
+
+static struct symbol *
+better_symbol (struct symbol *a, struct symbol *b, const domain_enum domain)
+{
+ if (a == NULL)
+ return b;
+ if (b == NULL)
+ return a;
+
+ if (SYMBOL_DOMAIN (a) == domain
+ && SYMBOL_DOMAIN (b) != domain)
+ return a;
+ if (SYMBOL_DOMAIN (b) == domain
+ && SYMBOL_DOMAIN (a) != domain)
+ return b;
+
+ if (SYMBOL_CLASS (a) != LOC_UNRESOLVED
+ && SYMBOL_CLASS (b) == LOC_UNRESOLVED)
+ return a;
+ if (SYMBOL_CLASS (b) != LOC_UNRESOLVED
+ && SYMBOL_CLASS (a) == LOC_UNRESOLVED)
+ return b;
+
+ return a;
+}
+
/* See block.h. */
struct symbol *
@@ -746,7 +783,34 @@
sym != NULL;
sym = mdict_iter_match_next (lookup_name, &mdict_iter))
{
- if (SYMBOL_DOMAIN (sym) == domain)
+ /* With the fix for PR gcc/debug/91507, we get for:
+ ...
+ extern char *zzz[];
+ char *zzz[ ] = {
+ "abc",
+ "cde"
+ };
+ ...
+ DWARF which will result in two entries in the symbol table, a decl
+ with type char *[] and a def with type char *[2].
+
+ If we return the decl here, we don't get the value of zzz:
+ ...
+ $ gdb a.spec.out -batch -ex "p zzz"
+ $1 = 0x601030 <zzz>
+ ...
+ because we're returning the symbol without location information, and
+ because the fallback that uses the address from the minimal symbols
+ doesn't work either because the type of the decl does not specify a
+ size.
+
+ To fix this, we prefer def over decl in best_symbol and
+ better_symbol.
+
+ In absence of the gcc fix, both def and decl have type char *[], so
+ the only option to make this work is improve the fallback to use the
+ size of the minimal symbol. Filed as PR exp/24989. */
+ if (best_symbol (sym, domain))
return sym;
/* This is a bit of a hack, but symbol_matches_domain might ignore
@@ -755,7 +819,7 @@
exactly the same domain. PR 16253. */
if (symbol_matches_domain (SYMBOL_LANGUAGE (sym),
SYMBOL_DOMAIN (sym), domain))
- other = sym;
+ other = better_symbol (other, sym, domain);
}
return other;
diff --git a/gdb/testsuite/gdb.dwarf2/varval.exp b/gdb/testsuite/gdb.dwarf2/varval.exp
index 5945910..0a1b179 100644
--- a/gdb/testsuite/gdb.dwarf2/varval.exp
+++ b/gdb/testsuite/gdb.dwarf2/varval.exp
@@ -55,7 +55,7 @@
var_b_label var_c_label var_p_label var_bad_label \
varval_label var_s_label var_untyped_label \
var_a_abstract_label var_a_concrete_label \
- varval2_label
+ varval2_label varval3_def_label varval3_decl_label
set int_size [get_sizeof "int" -1]
@@ -171,6 +171,19 @@
{DW_AT_location {DW_OP_addr [gdb_target_symbol "var_b"]} SPECIAL_expr}
}
+ varval3_decl_label: DW_TAG_variable {
+ {DW_AT_name "varval3"}
+ {DW_AT_type :${int_label}}
+ {DW_AT_external 1 DW_FORM_flag}
+ {DW_AT_declaration 1 DW_FORM_flag}
+ }
+ varval3_def_label: DW_TAG_variable {
+ {DW_AT_name "varval3"}
+ {DW_AT_external 1 DW_FORM_flag}
+ {DW_AT_type :${int_label}}
+ {DW_AT_location {DW_OP_addr [gdb_target_symbol "var_a"]} SPECIAL_expr}
+ }
+
DW_TAG_subprogram {
{MACRO_AT_func { "main" "${srcdir}/${subdir}/${srcfile}" }}
{DW_AT_type :${int_label}}
@@ -290,6 +303,7 @@
gdb_test "print varval" "= 8"
gdb_test "print varval2" "= 8"
+gdb_test "print varval3" "= 8"
gdb_test "print constval" "= 53"
gdb_test "print mixedval" "= 42"
gdb_test "print pointerval" "= \\(int \\*\\) $hex <var_b>"
^ permalink raw reply [flat|nested] 21+ messages in thread* [review v3] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (4 preceding siblings ...)
2019-10-23 16:27 ` [review v3] " Tom de Vries (Code Review)
@ 2019-10-23 16:29 ` Tom de Vries (Code Review)
2019-10-31 15:50 ` Tom Tromey (Code Review)
` (14 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Tom de Vries (Code Review) @ 2019-10-23 16:29 UTC (permalink / raw)
To: gdb-patches; +Cc: Luis Machado, Andrew Burgess
Tom de Vries has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
Patch Set 3:
(2 comments)
Thanks for the comments, updated the patch set.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29/2/gdb/block.c
File gdb/block.c:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29/2/gdb/block.c@737
PS2, Line 737: /* Return true if symbol A is a better match than symbol B for DOMAIN. */
> The comment is a bit misleading. […]
Done
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29/2/gdb/testsuite/gdb.dwarf2/varval.exp
File gdb/testsuite/gdb.dwarf2/varval.exp:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29/2/gdb/testsuite/gdb.dwarf2/varval.exp@209
PS2, Line 209:
> Spurious new line?
Done
^ permalink raw reply [flat|nested] 21+ messages in thread* [review v3] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (5 preceding siblings ...)
2019-10-23 16:29 ` Tom de Vries (Code Review)
@ 2019-10-31 15:50 ` Tom Tromey (Code Review)
2019-11-04 1:11 ` Joel Brobecker (Code Review)
` (13 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Tom Tromey (Code Review) @ 2019-10-31 15:50 UTC (permalink / raw)
To: Tom de Vries, gdb-patches; +Cc: Luis Machado, Andrew Burgess
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
Patch Set 3:
Thank you for doing this.
The patch itself seems perfectly fine to me. I appreciated the comment
and the test case -- those help a lot.
However, I wonder why gdb is even making a symbol for a declaration in the
first place. For some other kinds of declarations, gdb doesn't bother, so
I was wondering if it's possible to simply ignore them in general.
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 3
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Thu, 31 Oct 2019 15:50:37 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 21+ messages in thread* [review v3] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (6 preceding siblings ...)
2019-10-31 15:50 ` Tom Tromey (Code Review)
@ 2019-11-04 1:11 ` Joel Brobecker (Code Review)
2019-11-07 14:27 ` Tom de Vries (Code Review)
` (12 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Joel Brobecker (Code Review) @ 2019-11-04 1:11 UTC (permalink / raw)
To: Tom de Vries, gdb-patches; +Cc: Tom Tromey, Luis Machado, Andrew Burgess
Joel Brobecker has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
Patch Set 3:
> Patch Set 3:
>
> Thank you for doing this.
>
> The patch itself seems perfectly fine to me. I appreciated the comment
> and the test case -- those help a lot.
>
> However, I wonder why gdb is even making a symbol for a declaration in the
> first place. For some other kinds of declarations, gdb doesn't bother, so
> I was wondering if it's possible to simply ignore them in general.
Another way to ask the same question is what would it mean to have a type which has a declaration, but no definition. Wouldn't we still want to know about that type?
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 3
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Mon, 04 Nov 2019 01:11:31 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 21+ messages in thread* [review v3] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (7 preceding siblings ...)
2019-11-04 1:11 ` Joel Brobecker (Code Review)
@ 2019-11-07 14:27 ` Tom de Vries (Code Review)
2019-11-07 15:30 ` Tom de Vries (Code Review)
` (11 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Tom de Vries (Code Review) @ 2019-11-07 14:27 UTC (permalink / raw)
To: gdb-patches; +Cc: Joel Brobecker, Tom Tromey, Luis Machado, Andrew Burgess
Tom de Vries has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
Patch Set 3:
> Patch Set 3:
>
> Thank you for doing this.
>
> The patch itself seems perfectly fine to me. I appreciated the comment
> and the test case -- those help a lot.
>
> However, I wonder why gdb is even making a symbol for a declaration in the
> first place. For some other kinds of declarations, gdb doesn't bother, so
> I was wondering if it's possible to simply ignore them in general.
In general, there's a use of adding variable declarations to the symbol table. Say we have a declaration of a variable in one file, and the definition in another. If the file with the definition is build without debug info, but the file with the declaration with debug info, we can combine the debuginfo of the declaration and the minimal symbol info of the definition to print the value of the variable using the proper type.
But in the case of this PR, the size of the declaration is zero, and that currently means that this information composition doesn't work. That could be fixed by using the size of the symbol from the minimal symbol info (that's PR24989).
As for the question whether we can ignore the declaration, I've tried this approach (let's call it ignore.patch).
Ignore.patch:
...
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 0a7a0334202..381bd45e805 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -21830,6 +21830,10 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
else if (attr2 && (DW_UNSND (attr2) != 0)
&& dwarf2_attr (die, DW_AT_type, cu) != NULL)
{
+ if (die->tag == DW_TAG_variable && die_is_declaration (die, cu)
+ && TYPE_LENGTH (SYMBOL_TYPE (sym)) == 0)
+ suppress_add = 1;
+
/* A variable with DW_AT_external is never static, but it
may be block-scoped. */
list_to_add
...
It passes regression testing, and fixes the original test-case for this PR (PR24971).
Ignore.patch does not fix the test-case added in the current patch set, but that one would have to be adapted for this fix (this fix works when type size is zero, and that's not the case for that test-case).
Also, ignore.patch fixes the test-case of the spinoff PR24985.
OTOH, if we go with ignore.patch, it may have to be reverted again in order to trigger the (currently non-existing) fix for PR24989.
So AFAIU:
- if we fix this PR using ignore.patch and then revert it in order to fix PR24972, the test-case for this PR is handled by looking up the symbol size in the minimal symbol info
- if we fix this PR using the currently proposed patch set, and then fix PR24972, the test-case for this PR is still handled by finding the definition instead of the declaration.
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 3
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Thu, 07 Nov 2019 14:27:11 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 21+ messages in thread* [review v3] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (8 preceding siblings ...)
2019-11-07 14:27 ` Tom de Vries (Code Review)
@ 2019-11-07 15:30 ` Tom de Vries (Code Review)
2019-11-11 16:45 ` Joel Brobecker (Code Review)
` (10 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Tom de Vries (Code Review) @ 2019-11-07 15:30 UTC (permalink / raw)
To: gdb-patches; +Cc: Joel Brobecker, Tom Tromey, Luis Machado, Andrew Burgess
Tom de Vries has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
Patch Set 3:
> Patch Set 3:
>
> > Patch Set 3:
> >
> > Thank you for doing this.
> >
> > The patch itself seems perfectly fine to me. I appreciated the comment
> > and the test case -- those help a lot.
> >
> > However, I wonder why gdb is even making a symbol for a declaration in the
> > first place. For some other kinds of declarations, gdb doesn't bother, so
> > I was wondering if it's possible to simply ignore them in general.
>
> Another way to ask the same question is what would it mean to have a type which has a declaration, but no definition. Wouldn't we still want to know about that type?
The thing with types is that we can list them using info types, which makes them visible.
Variable declarations by themselves are not visible, only variable definitions. But the declaration can help to add information that's not available at the definition, as I've tried to explain in more detail at the previous comment.
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 3
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Thu, 07 Nov 2019 15:30:12 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 21+ messages in thread* [review v3] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (9 preceding siblings ...)
2019-11-07 15:30 ` Tom de Vries (Code Review)
@ 2019-11-11 16:45 ` Joel Brobecker (Code Review)
2019-11-14 16:38 ` Tom de Vries (Code Review)
` (9 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Joel Brobecker (Code Review) @ 2019-11-11 16:45 UTC (permalink / raw)
To: Tom de Vries, gdb-patches; +Cc: Tom Tromey, Luis Machado, Andrew Burgess
Joel Brobecker has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
Patch Set 3:
> > > The patch itself seems perfectly fine to me. I appreciated the comment
> > > and the test case -- those help a lot.
> > >
> > > However, I wonder why gdb is even making a symbol for a declaration in the
> > > first place. For some other kinds of declarations, gdb doesn't bother, so
> > > I was wondering if it's possible to simply ignore them in general.
> >
> > Another way to ask the same question is what would it mean to have a type which has a declaration, but no definition. Wouldn't we still want to know about that type?
>
> The thing with types is that we can list them using info types, which makes them visible.
>
> Variable declarations by themselves are not visible, only variable definitions. But the declaration can help to add information that's not available at the definition, as I've tried to explain in more detail at the previous comment.
At this stage of the discussion, I think it shows that the subject is a little bit delicate and maybe we should start viewing the resolution for the 9.x branch separately from the resolution in master, because 9.x should be imminent.
For me, the current patch is OK for 9.x, as it seems its impact is relatively controlled.
The ignore.patch fix, on the other hand, seems a little too risky for inclusion this late before we branch 9.x.
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 3
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Mon, 11 Nov 2019 16:45:16 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 21+ messages in thread* [review v3] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (10 preceding siblings ...)
2019-11-11 16:45 ` Joel Brobecker (Code Review)
@ 2019-11-14 16:38 ` Tom de Vries (Code Review)
2019-11-21 13:34 ` Tom Tromey (Code Review)
` (8 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Tom de Vries (Code Review) @ 2019-11-14 16:38 UTC (permalink / raw)
To: gdb-patches; +Cc: Joel Brobecker, Tom Tromey, Luis Machado, Andrew Burgess
Tom de Vries has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
Patch Set 3:
> Patch Set 3:
> For me, the current patch is OK for 9.x, as it seems its impact is relatively controlled.
>
> The ignore.patch fix, on the other hand, seems a little too risky for inclusion this late before we branch 9.x.
Hi Joel, thanks for the comments. Do I understand correctly then that I should commit the current patch to trunk?
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 3
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Thu, 14 Nov 2019 16:38:27 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 21+ messages in thread* [review v3] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (11 preceding siblings ...)
2019-11-14 16:38 ` Tom de Vries (Code Review)
@ 2019-11-21 13:34 ` Tom Tromey (Code Review)
2019-11-22 10:24 ` Tom de Vries (Code Review)
` (7 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Tom Tromey (Code Review) @ 2019-11-21 13:34 UTC (permalink / raw)
To: Tom de Vries, gdb-patches; +Cc: Joel Brobecker, Luis Machado, Andrew Burgess
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
Patch Set 3: Code-Review+2
> Patch Set 3:
>
> > Patch Set 3:
> > For me, the current patch is OK for 9.x, as it seems its impact is relatively controlled.
> >
> > The ignore.patch fix, on the other hand, seems a little too risky for inclusion this late before we branch 9.x.
>
> Hi Joel, thanks for the comments. Do I understand correctly then that I should commit the current patch to trunk?
I re-read the thread and the patch, and I think this is fine to check in.
One factor that helped was that I finally realized that this would only cause
more searching in the case where a declaration is found, which maybe is not
super common.
Thank you for doing this.
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 3
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 13:34:06 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 21+ messages in thread* [review v3] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (12 preceding siblings ...)
2019-11-21 13:34 ` Tom Tromey (Code Review)
@ 2019-11-22 10:24 ` Tom de Vries (Code Review)
2019-11-22 11:02 ` Tom de Vries (Code Review)
` (6 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Tom de Vries (Code Review) @ 2019-11-22 10:24 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey, Joel Brobecker, Luis Machado, Andrew Burgess
Tom de Vries has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
Patch Set 3:
I just tested master+patch using the original example from the PR, and found that there is a difference in calling before and after start:
...
$ ./gdb.sh a.24971.out -batch -ex "p zzz"
$1 = {0x400504 "abc", 0x400508 "cde"}
$ ./gdb.sh a.24971.out -batch -ex start -ex "p zzz"
Temporary breakpoint 1 at 0x40047b: file t.c, line 6.
Temporary breakpoint 1, main () at t.c:6
6 main (void)
$1 = 0x601030 <zzz>
...
Doing the same using gdb-8.3-branch+patch, gives the required outcome in both cases. Same for 8.3 branchpoint + patch.
I'm currently doing a bisect to find out at what point this stopped working.
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 3
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-Comment-Date: Fri, 22 Nov 2019 10:24:33 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 21+ messages in thread* [review v3] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (13 preceding siblings ...)
2019-11-22 10:24 ` Tom de Vries (Code Review)
@ 2019-11-22 11:02 ` Tom de Vries (Code Review)
2019-11-22 12:17 ` [review v4] " Tom de Vries (Code Review)
` (5 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Tom de Vries (Code Review) @ 2019-11-22 11:02 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey, Joel Brobecker, Luis Machado, Andrew Burgess
Tom de Vries has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
Patch Set 3:
> I'm currently doing a bisect to find out at what point this stopped working.
That's commit d3d323915c0 "Search global block from basic_lookup_symbol_nonlocal"
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 3
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-Comment-Date: Fri, 22 Nov 2019 11:02:03 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 21+ messages in thread* [review v4] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (14 preceding siblings ...)
2019-11-22 11:02 ` Tom de Vries (Code Review)
@ 2019-11-22 12:17 ` Tom de Vries (Code Review)
2019-11-22 12:19 ` Tom de Vries (Code Review)
` (4 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Tom de Vries (Code Review) @ 2019-11-22 12:17 UTC (permalink / raw)
To: Luis Machado, Andrew Burgess, Tom Tromey, gdb-patches; +Cc: Joel Brobecker
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
[gdb/symtab] Prefer var def over decl
Consider the DWARF as generated by gcc with the tentative patch to fix gcc
PR91507 - "wrong debug for completed array with previous incomplete
declaration":
...
<1><f4>: Abbrev Number: 2 (DW_TAG_array_type)
<f5> DW_AT_type : <0xff>
<f9> DW_AT_sibling : <0xff>
<2><fd>: Abbrev Number: 3 (DW_TAG_subrange_type)
<2><fe>: Abbrev Number: 0
<1><ff>: Abbrev Number: 4 (DW_TAG_pointer_type)
<100> DW_AT_byte_size : 8
<101> DW_AT_type : <0x105>
<1><105>: Abbrev Number: 5 (DW_TAG_base_type)
<106> DW_AT_byte_size : 1
<107> DW_AT_encoding : 6 (signed char)
<108> DW_AT_name : (indirect string, offset: 0x19f): char
<1><10c>: Abbrev Number: 6 (DW_TAG_variable)
<10d> DW_AT_name : zzz
<111> DW_AT_decl_file : 1
<112> DW_AT_decl_line : 1
<113> DW_AT_decl_column : 14
<114> DW_AT_type : <0xf4>
<118> DW_AT_external : 1
<118> DW_AT_declaration : 1
<1><118>: Abbrev Number: 2 (DW_TAG_array_type)
<119> DW_AT_type : <0xff>
<11d> DW_AT_sibling : <0x128>
<1><12f>: Abbrev Number: 8 (DW_TAG_variable)
<130> DW_AT_specification: <0x10c>
<134> DW_AT_decl_line : 2
<135> DW_AT_decl_column : 7
<136> DW_AT_type : <0x118>
<13a> DW_AT_location : 9 byte block: 3 30 10 60 0 0 0 0 0 (DW_OP_addr: 601030)
...
The DWARF will result in two entries in the symbol table, a decl with type
char *[] and a def with type char*[2].
When trying to print the value of zzz:
...
$ gdb a.spec.out -batch -ex "p zzz"
...
the decl (rather than the def) will be found in the symbol table, which is
missing the location information, and consequently we get:
...
$1 = 0x601030 <zzz>
...
[ There is a fallback mechanism that finds the address of the variable in the
minimal symbol table, but that's not used here, because the type of the decl
does not specify a size. We could use the symbol size here to get the size
of the type, but that's currently not done: PR exp/24989. Still, fixing that
PR would not fix the generic case, where minimal symbol info is not
available. ]
Fix this by preferring defs over decls when searching in the symbol table.
Build and reg-tested on x86_64-linux.
gdb/ChangeLog:
2019-09-10 Tom de Vries <tdevries@suse.de>
PR symtab/24971
* block.c (best_symbol, better_symbol): New function.
(block_lookup_symbol_primary, block_lookup_symbol): Prefer def over
decl.
gdb/testsuite/ChangeLog:
2019-09-10 Tom de Vries <tdevries@suse.de>
* gdb.dwarf2/varval.exp: Add decl before def test.
Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
---
M gdb/block.c
M gdb/testsuite/gdb.dwarf2/varval.exp
2 files changed, 112 insertions(+), 10 deletions(-)
diff --git a/gdb/block.c b/gdb/block.c
index 40cd3f4..c9b93d1 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -657,6 +657,43 @@
return block_iter_match_step (iterator, name, 0);
}
+/* Return true if symbol A is the best match possible for DOMAIN. */
+
+static bool
+best_symbol (struct symbol *a, const domain_enum domain)
+{
+ return (SYMBOL_DOMAIN (a) == domain
+ && SYMBOL_CLASS (a) != LOC_UNRESOLVED);
+}
+
+/* Return symbol B if it is a better match than symbol A for DOMAIN.
+ Otherwise return A. */
+
+static struct symbol *
+better_symbol (struct symbol *a, struct symbol *b, const domain_enum domain)
+{
+ if (a == NULL)
+ return b;
+ if (b == NULL)
+ return a;
+
+ if (SYMBOL_DOMAIN (a) == domain
+ && SYMBOL_DOMAIN (b) != domain)
+ return a;
+ if (SYMBOL_DOMAIN (b) == domain
+ && SYMBOL_DOMAIN (a) != domain)
+ return b;
+
+ if (SYMBOL_CLASS (a) != LOC_UNRESOLVED
+ && SYMBOL_CLASS (b) == LOC_UNRESOLVED)
+ return a;
+ if (SYMBOL_CLASS (b) != LOC_UNRESOLVED
+ && SYMBOL_CLASS (a) == LOC_UNRESOLVED)
+ return b;
+
+ return a;
+}
+
/* See block.h.
Note that if NAME is the demangled form of a C++ symbol, we will fail
@@ -684,7 +721,9 @@
ALL_BLOCK_SYMBOLS_WITH_NAME (block, lookup_name, iter, sym)
{
- if (SYMBOL_DOMAIN (sym) == domain)
+ /* See comment related to PR gcc/debug/91507 in
+ block_lookup_symbol_primary. */
+ if (best_symbol (sym, domain))
return sym;
/* This is a bit of a hack, but symbol_matches_domain might ignore
STRUCT vs VAR domain symbols. So if a matching symbol is found,
@@ -692,7 +731,7 @@
exactly the same domain. PR 16253. */
if (symbol_matches_domain (SYMBOL_LANGUAGE (sym),
SYMBOL_DOMAIN (sym), domain))
- other = sym;
+ other = better_symbol (other, sym, domain);
}
return other;
}
@@ -746,7 +785,34 @@
sym != NULL;
sym = mdict_iter_match_next (lookup_name, &mdict_iter))
{
- if (SYMBOL_DOMAIN (sym) == domain)
+ /* With the fix for PR gcc/debug/91507, we get for:
+ ...
+ extern char *zzz[];
+ char *zzz[ ] = {
+ "abc",
+ "cde"
+ };
+ ...
+ DWARF which will result in two entries in the symbol table, a decl
+ with type char *[] and a def with type char *[2].
+
+ If we return the decl here, we don't get the value of zzz:
+ ...
+ $ gdb a.spec.out -batch -ex "p zzz"
+ $1 = 0x601030 <zzz>
+ ...
+ because we're returning the symbol without location information, and
+ because the fallback that uses the address from the minimal symbols
+ doesn't work either because the type of the decl does not specify a
+ size.
+
+ To fix this, we prefer def over decl in best_symbol and
+ better_symbol.
+
+ In absence of the gcc fix, both def and decl have type char *[], so
+ the only option to make this work is improve the fallback to use the
+ size of the minimal symbol. Filed as PR exp/24989. */
+ if (best_symbol (sym, domain))
return sym;
/* This is a bit of a hack, but symbol_matches_domain might ignore
@@ -755,7 +821,7 @@
exactly the same domain. PR 16253. */
if (symbol_matches_domain (SYMBOL_LANGUAGE (sym),
SYMBOL_DOMAIN (sym), domain))
- other = sym;
+ other = better_symbol (other, sym, domain);
}
return other;
diff --git a/gdb/testsuite/gdb.dwarf2/varval.exp b/gdb/testsuite/gdb.dwarf2/varval.exp
index 5945910..8f5e16f 100644
--- a/gdb/testsuite/gdb.dwarf2/varval.exp
+++ b/gdb/testsuite/gdb.dwarf2/varval.exp
@@ -55,7 +55,8 @@
var_b_label var_c_label var_p_label var_bad_label \
varval_label var_s_label var_untyped_label \
var_a_abstract_label var_a_concrete_label \
- varval2_label
+ varval2_label varval3_def_label varval3_decl_label \
+ int_array_label int_array_of_1_label
set int_size [get_sizeof "int" -1]
@@ -171,6 +172,32 @@
{DW_AT_location {DW_OP_addr [gdb_target_symbol "var_b"]} SPECIAL_expr}
}
+ int_array_label: DW_TAG_array_type {
+ {DW_AT_type :${int_label}}
+ } {
+ DW_TAG_subrange_type {}
+ }
+ varval3_decl_label: DW_TAG_variable {
+ {DW_AT_name "varval3"}
+ {DW_AT_type :${int_array_label}}
+ {DW_AT_external 1 DW_FORM_flag}
+ {DW_AT_declaration 1 DW_FORM_flag}
+ }
+ int_array_of_1_label: DW_TAG_array_type {
+ {DW_AT_type :${int_label}}
+ } {
+ DW_TAG_subrange_type {
+ {DW_AT_type :$int_label}
+ {DW_AT_upper_bound 0 DW_FORM_data1}
+ }
+ }
+ varval3_def_label: DW_TAG_variable {
+ {DW_AT_name "varval3"}
+ {DW_AT_external 1 DW_FORM_flag}
+ {DW_AT_type :${int_array_of_1_label}}
+ {DW_AT_location {DW_OP_addr [gdb_target_symbol "var_a"]} SPECIAL_expr}
+ }
+
DW_TAG_subprogram {
{MACRO_AT_func { "main" "${srcdir}/${subdir}/${srcfile}" }}
{DW_AT_type :${int_label}}
@@ -277,19 +304,24 @@
if [prepare_for_testing "failed to prepare" ${executable} [list ${asm_file} ${srcfile}] {}] {
return -1
}
-
- # DW_OP_GNU_variable_value implementation requires a valid frame.
- if ![runto_main] {
- return -1
- }
}
if { [setup_exec 0] == -1 } {
return -1
}
+with_test_prefix "pre-main" {
+ gdb_test "print varval3" "= \\{8\\}" ""
+}
+
+# DW_OP_GNU_variable_value implementation requires a valid frame.
+if ![runto_main] {
+ return -1
+}
+
gdb_test "print varval" "= 8"
gdb_test "print varval2" "= 8"
+gdb_test "print varval3" "= \\{8\\}"
gdb_test "print constval" "= 53"
gdb_test "print mixedval" "= 42"
gdb_test "print pointerval" "= \\(int \\*\\) $hex <var_b>"
@@ -311,6 +343,10 @@
return -1
}
+# DW_OP_GNU_variable_value implementation requires a valid frame.
+if ![runto_main] {
+ return -1
+}
gdb_test "print badval" "value has been optimized out"
gdb_test "print bad_die_val1" \
"invalid dwarf2 offset 0xabcdef11"
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 4
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-MessageType: newpatchset
^ permalink raw reply [flat|nested] 21+ messages in thread* [review v4] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (15 preceding siblings ...)
2019-11-22 12:17 ` [review v4] " Tom de Vries (Code Review)
@ 2019-11-22 12:19 ` Tom de Vries (Code Review)
2019-12-05 20:29 ` Tom Tromey (Code Review)
` (3 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Tom de Vries (Code Review) @ 2019-11-22 12:19 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey, Joel Brobecker, Luis Machado, Andrew Burgess
Tom de Vries has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
Patch Set 4:
The new version of the patch set:
- improves the testcase to test both before and in main, and
- copies the fix from block_lookup_symbol_primary to block_lookup_symbol
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 4
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-Comment-Date: Fri, 22 Nov 2019 12:19:01 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 21+ messages in thread* [review v4] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (16 preceding siblings ...)
2019-11-22 12:19 ` Tom de Vries (Code Review)
@ 2019-12-05 20:29 ` Tom Tromey (Code Review)
2019-12-06 17:53 ` [pushed] " Sourceware to Gerrit sync (Code Review)
` (2 subsequent siblings)
20 siblings, 0 replies; 21+ messages in thread
From: Tom Tromey (Code Review) @ 2019-12-05 20:29 UTC (permalink / raw)
To: Tom de Vries, gdb-patches; +Cc: Joel Brobecker, Luis Machado, Andrew Burgess
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
Patch Set 4: Code-Review+2
Thanks once again. This is OK.
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 4
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-Comment-Date: Thu, 05 Dec 2019 20:28:59 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 21+ messages in thread* [pushed] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (17 preceding siblings ...)
2019-12-05 20:29 ` Tom Tromey (Code Review)
@ 2019-12-06 17:53 ` Sourceware to Gerrit sync (Code Review)
2019-12-06 17:53 ` Sourceware to Gerrit sync (Code Review)
2019-12-09 6:29 ` [review v5] " Tom de Vries (Code Review)
20 siblings, 0 replies; 21+ messages in thread
From: Sourceware to Gerrit sync (Code Review) @ 2019-12-06 17:53 UTC (permalink / raw)
To: Tom de Vries, Luis Machado, Andrew Burgess, Tom Tromey, gdb-patches
Cc: Joel Brobecker
The original change was created by Tom de Vries.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
[gdb/symtab] Prefer var def over decl
Consider the DWARF as generated by gcc with the tentative patch to fix gcc
PR91507 - "wrong debug for completed array with previous incomplete
declaration":
...
<1><f4>: Abbrev Number: 2 (DW_TAG_array_type)
<f5> DW_AT_type : <0xff>
<f9> DW_AT_sibling : <0xff>
<2><fd>: Abbrev Number: 3 (DW_TAG_subrange_type)
<2><fe>: Abbrev Number: 0
<1><ff>: Abbrev Number: 4 (DW_TAG_pointer_type)
<100> DW_AT_byte_size : 8
<101> DW_AT_type : <0x105>
<1><105>: Abbrev Number: 5 (DW_TAG_base_type)
<106> DW_AT_byte_size : 1
<107> DW_AT_encoding : 6 (signed char)
<108> DW_AT_name : (indirect string, offset: 0x19f): char
<1><10c>: Abbrev Number: 6 (DW_TAG_variable)
<10d> DW_AT_name : zzz
<111> DW_AT_decl_file : 1
<112> DW_AT_decl_line : 1
<113> DW_AT_decl_column : 14
<114> DW_AT_type : <0xf4>
<118> DW_AT_external : 1
<118> DW_AT_declaration : 1
<1><118>: Abbrev Number: 2 (DW_TAG_array_type)
<119> DW_AT_type : <0xff>
<11d> DW_AT_sibling : <0x128>
<1><12f>: Abbrev Number: 8 (DW_TAG_variable)
<130> DW_AT_specification: <0x10c>
<134> DW_AT_decl_line : 2
<135> DW_AT_decl_column : 7
<136> DW_AT_type : <0x118>
<13a> DW_AT_location : 9 byte block: 3 30 10 60 0 0 0 0 0 (DW_OP_addr: 601030)
...
The DWARF will result in two entries in the symbol table, a decl with type
char *[] and a def with type char*[2].
When trying to print the value of zzz:
...
$ gdb a.spec.out -batch -ex "p zzz"
...
the decl (rather than the def) will be found in the symbol table, which is
missing the location information, and consequently we get:
...
$1 = 0x601030 <zzz>
...
[ There is a fallback mechanism that finds the address of the variable in the
minimal symbol table, but that's not used here, because the type of the decl
does not specify a size. We could use the symbol size here to get the size
of the type, but that's currently not done: PR exp/24989. Still, fixing that
PR would not fix the generic case, where minimal symbol info is not
available. ]
Fix this by preferring defs over decls when searching in the symbol table.
Build and reg-tested on x86_64-linux.
gdb/ChangeLog:
2019-12-06 Tom de Vries <tdevries@suse.de>
PR symtab/24971
* block.c (best_symbol, better_symbol): New function.
(block_lookup_symbol_primary, block_lookup_symbol): Prefer def over
decl.
gdb/testsuite/ChangeLog:
2019-12-06 Tom de Vries <tdevries@suse.de>
* gdb.dwarf2/varval.exp: Add decl before def test.
Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
---
M gdb/ChangeLog
M gdb/block.c
M gdb/testsuite/ChangeLog
M gdb/testsuite/gdb.dwarf2/varval.exp
4 files changed, 123 insertions(+), 10 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2d60712..55ba844 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2019-12-06 Tom de Vries <tdevries@suse.de>
+
+ PR symtab/24971
+ * block.c (best_symbol, better_symbol): New function.
+ (block_lookup_symbol_primary, block_lookup_symbol): Prefer def over
+ decl.
+
2019-12-06 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* gdbtypes.h: Define the REFERENCE_SEE_THROUGH_BADNESS value.
diff --git a/gdb/block.c b/gdb/block.c
index a4592e3..b49c548 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -657,6 +657,43 @@
return block_iter_match_step (iterator, name, 0);
}
+/* Return true if symbol A is the best match possible for DOMAIN. */
+
+static bool
+best_symbol (struct symbol *a, const domain_enum domain)
+{
+ return (SYMBOL_DOMAIN (a) == domain
+ && SYMBOL_CLASS (a) != LOC_UNRESOLVED);
+}
+
+/* Return symbol B if it is a better match than symbol A for DOMAIN.
+ Otherwise return A. */
+
+static struct symbol *
+better_symbol (struct symbol *a, struct symbol *b, const domain_enum domain)
+{
+ if (a == NULL)
+ return b;
+ if (b == NULL)
+ return a;
+
+ if (SYMBOL_DOMAIN (a) == domain
+ && SYMBOL_DOMAIN (b) != domain)
+ return a;
+ if (SYMBOL_DOMAIN (b) == domain
+ && SYMBOL_DOMAIN (a) != domain)
+ return b;
+
+ if (SYMBOL_CLASS (a) != LOC_UNRESOLVED
+ && SYMBOL_CLASS (b) == LOC_UNRESOLVED)
+ return a;
+ if (SYMBOL_CLASS (b) != LOC_UNRESOLVED
+ && SYMBOL_CLASS (a) == LOC_UNRESOLVED)
+ return b;
+
+ return a;
+}
+
/* See block.h.
Note that if NAME is the demangled form of a C++ symbol, we will fail
@@ -684,7 +721,9 @@
ALL_BLOCK_SYMBOLS_WITH_NAME (block, lookup_name, iter, sym)
{
- if (SYMBOL_DOMAIN (sym) == domain)
+ /* See comment related to PR gcc/debug/91507 in
+ block_lookup_symbol_primary. */
+ if (best_symbol (sym, domain))
return sym;
/* This is a bit of a hack, but symbol_matches_domain might ignore
STRUCT vs VAR domain symbols. So if a matching symbol is found,
@@ -692,7 +731,7 @@
exactly the same domain. PR 16253. */
if (symbol_matches_domain (SYMBOL_LANGUAGE (sym),
SYMBOL_DOMAIN (sym), domain))
- other = sym;
+ other = better_symbol (other, sym, domain);
}
return other;
}
@@ -746,7 +785,34 @@
sym != NULL;
sym = mdict_iter_match_next (lookup_name, &mdict_iter))
{
- if (SYMBOL_DOMAIN (sym) == domain)
+ /* With the fix for PR gcc/debug/91507, we get for:
+ ...
+ extern char *zzz[];
+ char *zzz[ ] = {
+ "abc",
+ "cde"
+ };
+ ...
+ DWARF which will result in two entries in the symbol table, a decl
+ with type char *[] and a def with type char *[2].
+
+ If we return the decl here, we don't get the value of zzz:
+ ...
+ $ gdb a.spec.out -batch -ex "p zzz"
+ $1 = 0x601030 <zzz>
+ ...
+ because we're returning the symbol without location information, and
+ because the fallback that uses the address from the minimal symbols
+ doesn't work either because the type of the decl does not specify a
+ size.
+
+ To fix this, we prefer def over decl in best_symbol and
+ better_symbol.
+
+ In absence of the gcc fix, both def and decl have type char *[], so
+ the only option to make this work is improve the fallback to use the
+ size of the minimal symbol. Filed as PR exp/24989. */
+ if (best_symbol (sym, domain))
return sym;
/* This is a bit of a hack, but symbol_matches_domain might ignore
@@ -755,7 +821,7 @@
exactly the same domain. PR 16253. */
if (symbol_matches_domain (SYMBOL_LANGUAGE (sym),
SYMBOL_DOMAIN (sym), domain))
- other = sym;
+ other = better_symbol (other, sym, domain);
}
return other;
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index f7447dc..c9f66ad 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2019-12-06 Tom de Vries <tdevries@suse.de>
+
+ * gdb.dwarf2/varval.exp: Add decl before def test.
+
2019-12-06 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* gdb.cp/rvalue-ref-overload.exp: Minor cleanup.
diff --git a/gdb/testsuite/gdb.dwarf2/varval.exp b/gdb/testsuite/gdb.dwarf2/varval.exp
index 5945910..8f5e16f 100644
--- a/gdb/testsuite/gdb.dwarf2/varval.exp
+++ b/gdb/testsuite/gdb.dwarf2/varval.exp
@@ -55,7 +55,8 @@
var_b_label var_c_label var_p_label var_bad_label \
varval_label var_s_label var_untyped_label \
var_a_abstract_label var_a_concrete_label \
- varval2_label
+ varval2_label varval3_def_label varval3_decl_label \
+ int_array_label int_array_of_1_label
set int_size [get_sizeof "int" -1]
@@ -171,6 +172,32 @@
{DW_AT_location {DW_OP_addr [gdb_target_symbol "var_b"]} SPECIAL_expr}
}
+ int_array_label: DW_TAG_array_type {
+ {DW_AT_type :${int_label}}
+ } {
+ DW_TAG_subrange_type {}
+ }
+ varval3_decl_label: DW_TAG_variable {
+ {DW_AT_name "varval3"}
+ {DW_AT_type :${int_array_label}}
+ {DW_AT_external 1 DW_FORM_flag}
+ {DW_AT_declaration 1 DW_FORM_flag}
+ }
+ int_array_of_1_label: DW_TAG_array_type {
+ {DW_AT_type :${int_label}}
+ } {
+ DW_TAG_subrange_type {
+ {DW_AT_type :$int_label}
+ {DW_AT_upper_bound 0 DW_FORM_data1}
+ }
+ }
+ varval3_def_label: DW_TAG_variable {
+ {DW_AT_name "varval3"}
+ {DW_AT_external 1 DW_FORM_flag}
+ {DW_AT_type :${int_array_of_1_label}}
+ {DW_AT_location {DW_OP_addr [gdb_target_symbol "var_a"]} SPECIAL_expr}
+ }
+
DW_TAG_subprogram {
{MACRO_AT_func { "main" "${srcdir}/${subdir}/${srcfile}" }}
{DW_AT_type :${int_label}}
@@ -277,19 +304,24 @@
if [prepare_for_testing "failed to prepare" ${executable} [list ${asm_file} ${srcfile}] {}] {
return -1
}
-
- # DW_OP_GNU_variable_value implementation requires a valid frame.
- if ![runto_main] {
- return -1
- }
}
if { [setup_exec 0] == -1 } {
return -1
}
+with_test_prefix "pre-main" {
+ gdb_test "print varval3" "= \\{8\\}" ""
+}
+
+# DW_OP_GNU_variable_value implementation requires a valid frame.
+if ![runto_main] {
+ return -1
+}
+
gdb_test "print varval" "= 8"
gdb_test "print varval2" "= 8"
+gdb_test "print varval3" "= \\{8\\}"
gdb_test "print constval" "= 53"
gdb_test "print mixedval" "= 42"
gdb_test "print pointerval" "= \\(int \\*\\) $hex <var_b>"
@@ -311,6 +343,10 @@
return -1
}
+# DW_OP_GNU_variable_value implementation requires a valid frame.
+if ![runto_main] {
+ return -1
+}
gdb_test "print badval" "value has been optimized out"
gdb_test "print bad_die_val1" \
"invalid dwarf2 offset 0xabcdef11"
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 5
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-MessageType: newpatchset
^ permalink raw reply [flat|nested] 21+ messages in thread* [pushed] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (18 preceding siblings ...)
2019-12-06 17:53 ` [pushed] " Sourceware to Gerrit sync (Code Review)
@ 2019-12-06 17:53 ` Sourceware to Gerrit sync (Code Review)
2019-12-09 6:29 ` [review v5] " Tom de Vries (Code Review)
20 siblings, 0 replies; 21+ messages in thread
From: Sourceware to Gerrit sync (Code Review) @ 2019-12-06 17:53 UTC (permalink / raw)
To: Tom de Vries, gdb-patches
Cc: Tom Tromey, Joel Brobecker, Luis Machado, Andrew Burgess
Sourceware to Gerrit sync has submitted this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
[gdb/symtab] Prefer var def over decl
Consider the DWARF as generated by gcc with the tentative patch to fix gcc
PR91507 - "wrong debug for completed array with previous incomplete
declaration":
...
<1><f4>: Abbrev Number: 2 (DW_TAG_array_type)
<f5> DW_AT_type : <0xff>
<f9> DW_AT_sibling : <0xff>
<2><fd>: Abbrev Number: 3 (DW_TAG_subrange_type)
<2><fe>: Abbrev Number: 0
<1><ff>: Abbrev Number: 4 (DW_TAG_pointer_type)
<100> DW_AT_byte_size : 8
<101> DW_AT_type : <0x105>
<1><105>: Abbrev Number: 5 (DW_TAG_base_type)
<106> DW_AT_byte_size : 1
<107> DW_AT_encoding : 6 (signed char)
<108> DW_AT_name : (indirect string, offset: 0x19f): char
<1><10c>: Abbrev Number: 6 (DW_TAG_variable)
<10d> DW_AT_name : zzz
<111> DW_AT_decl_file : 1
<112> DW_AT_decl_line : 1
<113> DW_AT_decl_column : 14
<114> DW_AT_type : <0xf4>
<118> DW_AT_external : 1
<118> DW_AT_declaration : 1
<1><118>: Abbrev Number: 2 (DW_TAG_array_type)
<119> DW_AT_type : <0xff>
<11d> DW_AT_sibling : <0x128>
<1><12f>: Abbrev Number: 8 (DW_TAG_variable)
<130> DW_AT_specification: <0x10c>
<134> DW_AT_decl_line : 2
<135> DW_AT_decl_column : 7
<136> DW_AT_type : <0x118>
<13a> DW_AT_location : 9 byte block: 3 30 10 60 0 0 0 0 0 (DW_OP_addr: 601030)
...
The DWARF will result in two entries in the symbol table, a decl with type
char *[] and a def with type char*[2].
When trying to print the value of zzz:
...
$ gdb a.spec.out -batch -ex "p zzz"
...
the decl (rather than the def) will be found in the symbol table, which is
missing the location information, and consequently we get:
...
$1 = 0x601030 <zzz>
...
[ There is a fallback mechanism that finds the address of the variable in the
minimal symbol table, but that's not used here, because the type of the decl
does not specify a size. We could use the symbol size here to get the size
of the type, but that's currently not done: PR exp/24989. Still, fixing that
PR would not fix the generic case, where minimal symbol info is not
available. ]
Fix this by preferring defs over decls when searching in the symbol table.
Build and reg-tested on x86_64-linux.
gdb/ChangeLog:
2019-12-06 Tom de Vries <tdevries@suse.de>
PR symtab/24971
* block.c (best_symbol, better_symbol): New function.
(block_lookup_symbol_primary, block_lookup_symbol): Prefer def over
decl.
gdb/testsuite/ChangeLog:
2019-12-06 Tom de Vries <tdevries@suse.de>
* gdb.dwarf2/varval.exp: Add decl before def test.
Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
---
M gdb/ChangeLog
M gdb/block.c
M gdb/testsuite/ChangeLog
M gdb/testsuite/gdb.dwarf2/varval.exp
4 files changed, 123 insertions(+), 10 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2d60712..55ba844 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2019-12-06 Tom de Vries <tdevries@suse.de>
+
+ PR symtab/24971
+ * block.c (best_symbol, better_symbol): New function.
+ (block_lookup_symbol_primary, block_lookup_symbol): Prefer def over
+ decl.
+
2019-12-06 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* gdbtypes.h: Define the REFERENCE_SEE_THROUGH_BADNESS value.
diff --git a/gdb/block.c b/gdb/block.c
index a4592e3..b49c548 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -657,6 +657,43 @@
return block_iter_match_step (iterator, name, 0);
}
+/* Return true if symbol A is the best match possible for DOMAIN. */
+
+static bool
+best_symbol (struct symbol *a, const domain_enum domain)
+{
+ return (SYMBOL_DOMAIN (a) == domain
+ && SYMBOL_CLASS (a) != LOC_UNRESOLVED);
+}
+
+/* Return symbol B if it is a better match than symbol A for DOMAIN.
+ Otherwise return A. */
+
+static struct symbol *
+better_symbol (struct symbol *a, struct symbol *b, const domain_enum domain)
+{
+ if (a == NULL)
+ return b;
+ if (b == NULL)
+ return a;
+
+ if (SYMBOL_DOMAIN (a) == domain
+ && SYMBOL_DOMAIN (b) != domain)
+ return a;
+ if (SYMBOL_DOMAIN (b) == domain
+ && SYMBOL_DOMAIN (a) != domain)
+ return b;
+
+ if (SYMBOL_CLASS (a) != LOC_UNRESOLVED
+ && SYMBOL_CLASS (b) == LOC_UNRESOLVED)
+ return a;
+ if (SYMBOL_CLASS (b) != LOC_UNRESOLVED
+ && SYMBOL_CLASS (a) == LOC_UNRESOLVED)
+ return b;
+
+ return a;
+}
+
/* See block.h.
Note that if NAME is the demangled form of a C++ symbol, we will fail
@@ -684,7 +721,9 @@
ALL_BLOCK_SYMBOLS_WITH_NAME (block, lookup_name, iter, sym)
{
- if (SYMBOL_DOMAIN (sym) == domain)
+ /* See comment related to PR gcc/debug/91507 in
+ block_lookup_symbol_primary. */
+ if (best_symbol (sym, domain))
return sym;
/* This is a bit of a hack, but symbol_matches_domain might ignore
STRUCT vs VAR domain symbols. So if a matching symbol is found,
@@ -692,7 +731,7 @@
exactly the same domain. PR 16253. */
if (symbol_matches_domain (SYMBOL_LANGUAGE (sym),
SYMBOL_DOMAIN (sym), domain))
- other = sym;
+ other = better_symbol (other, sym, domain);
}
return other;
}
@@ -746,7 +785,34 @@
sym != NULL;
sym = mdict_iter_match_next (lookup_name, &mdict_iter))
{
- if (SYMBOL_DOMAIN (sym) == domain)
+ /* With the fix for PR gcc/debug/91507, we get for:
+ ...
+ extern char *zzz[];
+ char *zzz[ ] = {
+ "abc",
+ "cde"
+ };
+ ...
+ DWARF which will result in two entries in the symbol table, a decl
+ with type char *[] and a def with type char *[2].
+
+ If we return the decl here, we don't get the value of zzz:
+ ...
+ $ gdb a.spec.out -batch -ex "p zzz"
+ $1 = 0x601030 <zzz>
+ ...
+ because we're returning the symbol without location information, and
+ because the fallback that uses the address from the minimal symbols
+ doesn't work either because the type of the decl does not specify a
+ size.
+
+ To fix this, we prefer def over decl in best_symbol and
+ better_symbol.
+
+ In absence of the gcc fix, both def and decl have type char *[], so
+ the only option to make this work is improve the fallback to use the
+ size of the minimal symbol. Filed as PR exp/24989. */
+ if (best_symbol (sym, domain))
return sym;
/* This is a bit of a hack, but symbol_matches_domain might ignore
@@ -755,7 +821,7 @@
exactly the same domain. PR 16253. */
if (symbol_matches_domain (SYMBOL_LANGUAGE (sym),
SYMBOL_DOMAIN (sym), domain))
- other = sym;
+ other = better_symbol (other, sym, domain);
}
return other;
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index f7447dc..c9f66ad 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2019-12-06 Tom de Vries <tdevries@suse.de>
+
+ * gdb.dwarf2/varval.exp: Add decl before def test.
+
2019-12-06 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* gdb.cp/rvalue-ref-overload.exp: Minor cleanup.
diff --git a/gdb/testsuite/gdb.dwarf2/varval.exp b/gdb/testsuite/gdb.dwarf2/varval.exp
index 5945910..8f5e16f 100644
--- a/gdb/testsuite/gdb.dwarf2/varval.exp
+++ b/gdb/testsuite/gdb.dwarf2/varval.exp
@@ -55,7 +55,8 @@
var_b_label var_c_label var_p_label var_bad_label \
varval_label var_s_label var_untyped_label \
var_a_abstract_label var_a_concrete_label \
- varval2_label
+ varval2_label varval3_def_label varval3_decl_label \
+ int_array_label int_array_of_1_label
set int_size [get_sizeof "int" -1]
@@ -171,6 +172,32 @@
{DW_AT_location {DW_OP_addr [gdb_target_symbol "var_b"]} SPECIAL_expr}
}
+ int_array_label: DW_TAG_array_type {
+ {DW_AT_type :${int_label}}
+ } {
+ DW_TAG_subrange_type {}
+ }
+ varval3_decl_label: DW_TAG_variable {
+ {DW_AT_name "varval3"}
+ {DW_AT_type :${int_array_label}}
+ {DW_AT_external 1 DW_FORM_flag}
+ {DW_AT_declaration 1 DW_FORM_flag}
+ }
+ int_array_of_1_label: DW_TAG_array_type {
+ {DW_AT_type :${int_label}}
+ } {
+ DW_TAG_subrange_type {
+ {DW_AT_type :$int_label}
+ {DW_AT_upper_bound 0 DW_FORM_data1}
+ }
+ }
+ varval3_def_label: DW_TAG_variable {
+ {DW_AT_name "varval3"}
+ {DW_AT_external 1 DW_FORM_flag}
+ {DW_AT_type :${int_array_of_1_label}}
+ {DW_AT_location {DW_OP_addr [gdb_target_symbol "var_a"]} SPECIAL_expr}
+ }
+
DW_TAG_subprogram {
{MACRO_AT_func { "main" "${srcdir}/${subdir}/${srcfile}" }}
{DW_AT_type :${int_label}}
@@ -277,19 +304,24 @@
if [prepare_for_testing "failed to prepare" ${executable} [list ${asm_file} ${srcfile}] {}] {
return -1
}
-
- # DW_OP_GNU_variable_value implementation requires a valid frame.
- if ![runto_main] {
- return -1
- }
}
if { [setup_exec 0] == -1 } {
return -1
}
+with_test_prefix "pre-main" {
+ gdb_test "print varval3" "= \\{8\\}" ""
+}
+
+# DW_OP_GNU_variable_value implementation requires a valid frame.
+if ![runto_main] {
+ return -1
+}
+
gdb_test "print varval" "= 8"
gdb_test "print varval2" "= 8"
+gdb_test "print varval3" "= \\{8\\}"
gdb_test "print constval" "= 53"
gdb_test "print mixedval" "= 42"
gdb_test "print pointerval" "= \\(int \\*\\) $hex <var_b>"
@@ -311,6 +343,10 @@
return -1
}
+# DW_OP_GNU_variable_value implementation requires a valid frame.
+if ![runto_main] {
+ return -1
+}
gdb_test "print badval" "value has been optimized out"
gdb_test "print bad_die_val1" \
"invalid dwarf2 offset 0xabcdef11"
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 5
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-MessageType: merged
^ permalink raw reply [flat|nested] 21+ messages in thread* [review v5] [gdb/symtab] Prefer var def over decl
[not found] <gerrit.1571043046000.Id92326cb8ef9903b121ef9e320658eb565d0f5a9@gnutoolchain-gerrit.osci.io>
` (19 preceding siblings ...)
2019-12-06 17:53 ` Sourceware to Gerrit sync (Code Review)
@ 2019-12-09 6:29 ` Tom de Vries (Code Review)
20 siblings, 0 replies; 21+ messages in thread
From: Tom de Vries (Code Review) @ 2019-12-09 6:29 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey, Joel Brobecker, Luis Machado, Andrew Burgess
Tom de Vries has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29
......................................................................
Patch Set 5:
> Patch Set 3:
>
> At this stage of the discussion, I think it shows that the subject is a little bit delicate and maybe we should start viewing the resolution for the 9.x branch separately from the resolution in master, because 9.x should be imminent.
>
> For me, the current patch is OK for 9.x, as it seems its impact is relatively controlled.
>
> The ignore.patch fix, on the other hand, seems a little too risky for inclusion this late before we branch 9.x.
I've filed PR25260 - "Handle decl before def more robustly" to track the ignore.patch fix ( https://sourceware.org/bugzilla/show_bug.cgi?id=25260 ).
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9
Gerrit-Change-Number: 29
Gerrit-PatchSet: 5
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-Comment-Date: Mon, 09 Dec 2019 06:29:16 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 21+ messages in thread