[review v3] [gdb/symtab] Prefer var def over decl

["Tom de Vries (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>]

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