[PATCH][gdb/symtab] Store external var decls in psymtab

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

[PATCH][gdb/symtab] Store external var decls in psymtab

Tom de Vries
Hi,

Consider a test-case consisting of source file test.c:
...
extern int aaa;
int
main (void)
{
  return 0;
}
...
and test-2.c:
...
int aaa = 33;
...
compiled with debug info only for test.c:
...
$ gcc -c test.c -g; gcc -c test2.c; gcc test.o test2.o -g
...

When trying to print aaa, we get:
...
$ gdb -batch a.out -ex "print aaa"
'aaa' has unknown type; cast it to its declared type
...
but with -readnow we have:
...
$ gdb -readnow -batch a.out -ex "print aaa"
$1 = 33
...

In the -readnow case, the symbol for aaa in the full symtab has
LOC_UNRESOLVED, and the symbol type is combined with the minimal symbol
address, to read the value and print it without cast.

Without the -readnow, we create partial symbols, but the aaa decl is missing
from the partial symtabs, so we find it only in the minimal symbols, resulting
in the cast request.  If the aaa decl would have been in the partial symtabs,
it would have been found, and the full symtab would have been expanded, after
which things would be as with -readnow.

The function add_partial_symbol has a comment on the LOC_UNRESOLVED +
minimal symbol addres construct at DW_TAG_variable handling:
...
      else if (pdi->is_external)
        {
          /* Global Variable.
             Don't enter into the minimal symbol tables as there is
             a minimal symbol table entry from the ELF symbols already.
             Enter into partial symbol table if it has a location
             descriptor or a type.
             If the location descriptor is missing, new_symbol will create
             a LOC_UNRESOLVED symbol, the address of the variable will then
             be determined from the minimal symbol table whenever the variable
             is referenced.
...
but it's not triggered due to this test in scan_partial_symbols:
...
            case DW_TAG_variable:
            ...
              if (!pdi->is_declaration)
                {
                  add_partial_symbol (pdi, cu);
                }
...

Fix this in scan_partial_symbols by allowing external variable decls to be
added to the partial symtabs.

Build and reg-tested on x86_64-linux.

The patch caused this regression:
...
(gdb) print a_thread_local^M
Cannot find thread-local storage for process 0, executable file tls/tls:^M
Cannot find thread-local variables on this target^M
(gdb) FAIL: gdb.threads/tls.exp: print a_thread_local
...
while without the patch we have:
...
(gdb) print a_thread_local^M
Cannot read `a_thread_local' without registers^M
(gdb) PASS: gdb.threads/tls.exp: print a_thread_local
...

However, without the patch but with -readnow we have the same FAIL as with the
patch.  In other words, the patch has the effect that we get the same result
with and without -readnow.

This can be explained as follows.  Without the patch, and without -readnow, we
have two a_thread_locals, the def and the decl:
...
$ gdb -batch outputs/gdb.threads/tls/tls \
    -ex "maint expand-symtabs" \
    -ex "print a_thread_local" \
    -ex "maint print symbols" \
    | grep "a_thread_local;"
Cannot read `a_thread_local' without registers
 int a_thread_local; computed at runtime
 int a_thread_local; unresolved
...
while without the patch and with -readnow, we have the opposite order:
...
$ gdb -readnow -batch outputs/gdb.threads/tls/tls  \
    -ex "maint expand-symtabs" \
    -ex "print a_thread_local" \
    -ex "maint print symbols" \
    | grep "a_thread_local;"
Cannot find thread-local storage for process 0, executable file tls/tls:
Cannot find thread-local variables on this target
 int a_thread_local; unresolved
 int a_thread_local; computed at runtime
...

With the patch we have the same order with and without -readnow, but just a
different one than before without -readnow.

AFAIU, the fact that we pick the unresolved symbol over the computed at
runtime symbol is some variant of PR24985 - "Cannot print value of global
variable because decl in one CU shadows def in other CU".

Mark the "Cannot find thread-local variables on this target" variant a PR24985
kfail.

OK for trunk?

Thanks,
- Tom

[gdb/symtab] Store external var decls in psymtab

gdb/ChangeLog:

2020-04-08  Tom de Vries  <[hidden email]>

        PR symtab/25764
        * dwarf2/read.c (scan_partial_symbols): Allow external variable decls
        in psymtabs.

gdb/testsuite/ChangeLog:

2020-04-08  Tom de Vries  <[hidden email]>

        PR symtab/25764
        * gdb.base/psym-external-decl-2.c: New test.
        * gdb.base/psym-external-decl.c: New test.
        * gdb.base/psym-external-decl.exp: New file.
        * gdb.threads/tls.exp: Add PR24985 kfail.

---
 gdb/dwarf2/read.c                             |  3 ++-
 gdb/testsuite/gdb.base/psym-external-decl-2.c | 18 ++++++++++++++++
 gdb/testsuite/gdb.base/psym-external-decl.c   | 25 ++++++++++++++++++++++
 gdb/testsuite/gdb.base/psym-external-decl.exp | 30 +++++++++++++++++++++++++++
 gdb/testsuite/gdb.threads/tls.exp             | 10 +++++++--
 5 files changed, 83 insertions(+), 3 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index da702205c6..82f698cc03 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -7908,7 +7908,8 @@ scan_partial_symbols (struct partial_die_info *first_die, CORE_ADDR *lowpc,
     case DW_TAG_variable:
     case DW_TAG_typedef:
     case DW_TAG_union_type:
-      if (!pdi->is_declaration)
+      if (!pdi->is_declaration
+  || (pdi->tag == DW_TAG_variable && pdi->is_external))
  {
   add_partial_symbol (pdi, cu);
  }
diff --git a/gdb/testsuite/gdb.base/psym-external-decl-2.c b/gdb/testsuite/gdb.base/psym-external-decl-2.c
new file mode 100644
index 0000000000..76e0587301
--- /dev/null
+++ b/gdb/testsuite/gdb.base/psym-external-decl-2.c
@@ -0,0 +1,18 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int aaa = 33;
diff --git a/gdb/testsuite/gdb.base/psym-external-decl.c b/gdb/testsuite/gdb.base/psym-external-decl.c
new file mode 100644
index 0000000000..7a4b107774
--- /dev/null
+++ b/gdb/testsuite/gdb.base/psym-external-decl.c
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+extern int aaa;
+
+int
+main (void)
+{
+  return 0;
+}
+
diff --git a/gdb/testsuite/gdb.base/psym-external-decl.exp b/gdb/testsuite/gdb.base/psym-external-decl.exp
new file mode 100644
index 0000000000..bbcc274575
--- /dev/null
+++ b/gdb/testsuite/gdb.base/psym-external-decl.exp
@@ -0,0 +1,30 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+standard_testfile .c psym-external-decl-2.c
+
+set srcfiles [list $srcfile $srcfile2]
+
+if { [build_executable_from_specs \
+  "failed to prepare" \
+  $testfile [list] \
+  $srcfile [list debug] \
+  $srcfile2 [list]] == -1 } {
+    return -1
+}
+
+clean_restart $testfile
+
+gdb_test "print aaa" " = 33"
diff --git a/gdb/testsuite/gdb.threads/tls.exp b/gdb/testsuite/gdb.threads/tls.exp
index fca5fa6db2..b4140ffd41 100644
--- a/gdb/testsuite/gdb.threads/tls.exp
+++ b/gdb/testsuite/gdb.threads/tls.exp
@@ -153,8 +153,14 @@ proc check_thread_stack {number spin_threads spin_threads_level} {
 
 clean_restart ${binfile}
 
-gdb_test "print a_thread_local" \
-    "Cannot read .a_thread_local. without registers"
+gdb_test_multiple "print a_thread_local" "" {
+    -re -wrap "Cannot find thread-local variables on this target" {
+ kfail "gdb/24985" $gdb_test_name
+    }
+    -re -wrap "Cannot read .a_thread_local. without registers" {
+ pass $gdb_test_name
+    }
+}
 
 if ![runto_main] then {
    fail "can't run to main"
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][gdb/symtab] Store external var decls in psymtab

Tom de Vries
On 08-04-2020 17:07, Tom de Vries wrote:

> Hi,
>
> Consider a test-case consisting of source file test.c:
> ...
> extern int aaa;
> int
> main (void)
> {
>   return 0;
> }
> ...
> and test-2.c:
> ...
> int aaa = 33;
> ...
> compiled with debug info only for test.c:
> ...
> $ gcc -c test.c -g; gcc -c test2.c; gcc test.o test2.o -g
> ...
>
> When trying to print aaa, we get:
> ...
> $ gdb -batch a.out -ex "print aaa"
> 'aaa' has unknown type; cast it to its declared type
> ...
> but with -readnow we have:
> ...
> $ gdb -readnow -batch a.out -ex "print aaa"
> $1 = 33
> ...
>
> In the -readnow case, the symbol for aaa in the full symtab has
> LOC_UNRESOLVED, and the symbol type is combined with the minimal symbol
> address, to read the value and print it without cast.
>
> Without the -readnow, we create partial symbols, but the aaa decl is missing
> from the partial symtabs, so we find it only in the minimal symbols, resulting
> in the cast request.  If the aaa decl would have been in the partial symtabs,
> it would have been found, and the full symtab would have been expanded, after
> which things would be as with -readnow.
>
> The function add_partial_symbol has a comment on the LOC_UNRESOLVED +
> minimal symbol addres construct at DW_TAG_variable handling:
> ...
>       else if (pdi->is_external)
> {
>  /* Global Variable.
>     Don't enter into the minimal symbol tables as there is
>     a minimal symbol table entry from the ELF symbols already.
>     Enter into partial symbol table if it has a location
>     descriptor or a type.
>     If the location descriptor is missing, new_symbol will create
>     a LOC_UNRESOLVED symbol, the address of the variable will then
>     be determined from the minimal symbol table whenever the variable
>     is referenced.
> ...
> but it's not triggered due to this test in scan_partial_symbols:
> ...
>             case DW_TAG_variable:
>    ...
>               if (!pdi->is_declaration)
>                 {
>                   add_partial_symbol (pdi, cu);
>                 }
> ...
>
> Fix this in scan_partial_symbols by allowing external variable decls to be
> added to the partial symtabs.
>
> Build and reg-tested on x86_64-linux.
>
> The patch caused this regression:
> ...
> (gdb) print a_thread_local^M
> Cannot find thread-local storage for process 0, executable file tls/tls:^M
> Cannot find thread-local variables on this target^M
> (gdb) FAIL: gdb.threads/tls.exp: print a_thread_local
> ...
> while without the patch we have:
> ...
> (gdb) print a_thread_local^M
> Cannot read `a_thread_local' without registers^M
> (gdb) PASS: gdb.threads/tls.exp: print a_thread_local
> ...
>
> However, without the patch but with -readnow we have the same FAIL as with the
> patch.  In other words, the patch has the effect that we get the same result
> with and without -readnow.
>
> This can be explained as follows.  Without the patch, and without -readnow, we
> have two a_thread_locals, the def and the decl:
> ...
> $ gdb -batch outputs/gdb.threads/tls/tls \
>     -ex "maint expand-symtabs" \
>     -ex "print a_thread_local" \
>     -ex "maint print symbols" \
>     | grep "a_thread_local;"
> Cannot read `a_thread_local' without registers
>  int a_thread_local; computed at runtime
>  int a_thread_local; unresolved
> ...
> while without the patch and with -readnow, we have the opposite order:
> ...
> $ gdb -readnow -batch outputs/gdb.threads/tls/tls  \
>     -ex "maint expand-symtabs" \
>     -ex "print a_thread_local" \
>     -ex "maint print symbols" \
>     | grep "a_thread_local;"
> Cannot find thread-local storage for process 0, executable file tls/tls:
> Cannot find thread-local variables on this target
>  int a_thread_local; unresolved
>  int a_thread_local; computed at runtime
> ...
>
> With the patch we have the same order with and without -readnow, but just a
> different one than before without -readnow.
>
> AFAIU, the fact that we pick the unresolved symbol over the computed at
> runtime symbol is some variant of PR24985 - "Cannot print value of global
> variable because decl in one CU shadows def in other CU".
>
> Mark the "Cannot find thread-local variables on this target" variant a PR24985
> kfail.
>
> OK for trunk?

Committed (
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=317d2668d01c7ddc9545029bf56d2b8c4d2bbfeb
).

Thanks,
- Tom