[PATCH] gdb: check for partial symtab presence in dwarf2_initialize_objfile

[PATCH] gdb: check for partial symtab presence in dwarf2_initialize_objfile

[Simon Marchi]

This patch fixes an internal error that is triggered when loading the
same binary twice with the index-cache on:

    $ ./gdb -q -nx --data-directory=data-directory
    (gdb) set index-cache on
    (gdb) shell mktemp -d
    /tmp/tmp.BLgouVoPq4
    (gdb) set index-cache directory /tmp/tmp.BLgouVoPq4
    (gdb) file a.out
    Reading symbols from a.out...
    (gdb) file a.out
    Load new symbol table from "a.out"? (y or n) y
    Reading symbols from a.out...
    /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:2540: internal-error: void create_cus_from_index(dwarf2_per_bfd*, const gdb_byte*, offset_type, const gdb_byte*, offset_type): Assertion `per_bfd->all_comp_units.empty ()' failed.
    A problem internal to GDB has been detected,
    further debugging may prove unreliable.
    Quit this debugging session? (y or n)

This is what happens:

1. We load the binary the first time, partial symtabs are created,
   per_bfd->all_comp_units is filled from those.
2. Because index-cache is on, we also generate an index in the cache.
3. We load the binary a second time, in dwarf2_initialize_objfile we
   check: was an index already loaded for this BFD?  No, so we try to
   read the index and fill the per-bfd using it.  We do find an index,
   it's in the cache.
4. The function create_cus_from_index asserts (rightfully) that
   per_cu->all_comp_units is empty, and the assertion fails.

This assertion verifies that we are not reading an index for a BFD for
which we have already built partial symtabs or read another index.

The index-cache gives a situation that isn't currently accounted for: a
BFD for which we have built the partial symtabs the first time, but has
an index the second time.

This patch addresses it by checking for the presence of partial symtabs
in dwarf2_initialize_objfile.  If there are, we don't try reading the
index.

gdb/ChangeLog:

	* dwarf2/read.c (dwarf2_initialize_objfile): Check for presence
	of partial symtabs.

gdb/testsuite/ChangeLog:

	* gdb.base/index-cache-load-twice.c: New.
	* gdb.base/index-cache-load-twice.exp: New.

Change-Id: Ie05474c44823fcdff852b73170dd28dfd66cb6a2
---
 gdb/dwarf2/read.c                             |  8 ++++
 .../gdb.base/index-cache-load-twice.c         | 22 ++++++++++
 .../gdb.base/index-cache-load-twice.exp       | 42 +++++++++++++++++++
 3 files changed, 72 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/index-cache-load-twice.c
 create mode 100644 gdb/testsuite/gdb.base/index-cache-load-twice.exp

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index e3073fe43ce3..a6b74ae4da6b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -6027,6 +6027,14 @@ dwarf2_initialize_objfile (struct objfile *objfile, dw_index_kind *index_kind)
       return true;
     }
 
+  /* There might already be partial symtabs built for this BFD.  This happens
+     when loading the same binary twice with the index-cache enabled.  If so,
+     don't try to read an index.  The objfile / per_objfile initialization will
+     be completed in dwarf2_build_psymtabs, in the standard partial symtabs
+     code path.  */
+  if (per_bfd->partial_symtabs != nullptr)
+    return false;
+
   if (dwarf2_read_debug_names (per_objfile))
     {
       *index_kind = dw_index_kind::DEBUG_NAMES;
diff --git a/gdb/testsuite/gdb.base/index-cache-load-twice.c b/gdb/testsuite/gdb.base/index-cache-load-twice.c
new file mode 100644
index 000000000000..9d7b2f1a4c28
--- /dev/null
+++ b/gdb/testsuite/gdb.base/index-cache-load-twice.c
@@ -0,0 +1,22 @@
+/* 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
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/index-cache-load-twice.exp b/gdb/testsuite/gdb.base/index-cache-load-twice.exp
new file mode 100644
index 000000000000..4d58097529b8
--- /dev/null
+++ b/gdb/testsuite/gdb.base/index-cache-load-twice.exp
@@ -0,0 +1,42 @@
+#   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/>.
+
+# This test checks that loading a file twice with the index cache enabled does
+# not crash.
+
+standard_testfile
+
+lassign [remote_exec host mktemp -d] ret cache_dir
+
+# The output of mktemp contains an end of line, remote it.
+set cache_dir [string trimright $cache_dir \r\n]
+
+if { $ret != 0 } {
+    fail "couldn't create temporary cache dir"
+    return
+}
+
+save_vars { GDBFLAGS } {
+    set GDBFLAGS "$GDBFLAGS -iex \"set index-cache directory $cache_dir\""
+    set GDBFLAGS "$GDBFLAGS -iex \"set index-cache on\""
+
+    if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
+	  {debug additional_flags=-Wl,--build-id}] } {
+	return
+    }
+
+    # This file command would generate an internal error.
+    gdb_file_cmd [standard_output_file $testfile]
+}
-- 
2.26.2

Re: [PATCH] gdb: check for partial symtab presence in dwarf2_initialize_objfile

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> gdb/ChangeLog:

Simon> 	* dwarf2/read.c (dwarf2_initialize_objfile): Check for presence
Simon> 	of partial symtabs.

Looks good, and thanks again for doing this.

Tom

Re: [PATCH] gdb: check for partial symtab presence in dwarf2_initialize_objfile

[Simon Marchi]

On 2020-06-15 11:10 a.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> gdb/ChangeLog:
> 
> Simon> 	* dwarf2/read.c (dwarf2_initialize_objfile): Check for presence
> Simon> 	of partial symtabs.
> 
> Looks good, and thanks again for doing this.
> 
> Tom
> 

Thanks.

Tom (de Vries), would you like to take a look before I push it?

Simon

Re: [PATCH] gdb: check for partial symtab presence in dwarf2_initialize_objfile

[Tom de Vries]

On 6/12/20 11:44 PM, Simon Marchi wrote:
> This patch fixes an internal error that is triggered when loading the
> same binary twice with the index-cache on:
> 
>     $ ./gdb -q -nx --data-directory=data-directory
>     (gdb) set index-cache on
>     (gdb) shell mktemp -d
>     /tmp/tmp.BLgouVoPq4
>     (gdb) set index-cache directory /tmp/tmp.BLgouVoPq4
>     (gdb) file a.out
>     Reading symbols from a.out...
>     (gdb) file a.out
>     Load new symbol table from "a.out"? (y or n) y
>     Reading symbols from a.out...
>     /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:2540: internal-error: void create_cus_from_index(dwarf2_per_bfd*, const gdb_byte*, offset_type, const gdb_byte*, offset_type): Assertion `per_bfd->all_comp_units.empty ()' failed.
>     A problem internal to GDB has been detected,
>     further debugging may prove unreliable.
>     Quit this debugging session? (y or n)
> 
> This is what happens:
> 
> 1. We load the binary the first time, partial symtabs are created,
>    per_bfd->all_comp_units is filled from those.
> 2. Because index-cache is on, we also generate an index in the cache.
> 3. We load the binary a second time, in dwarf2_initialize_objfile we
>    check: was an index already loaded for this BFD?  No, so we try to
>    read the index and fill the per-bfd using it.  We do find an index,
>    it's in the cache.
> 4. The function create_cus_from_index asserts (rightfully) that
>    per_cu->all_comp_units is empty, and the assertion fails.
> 
> This assertion verifies that we are not reading an index for a BFD for
> which we have already built partial symtabs or read another index.
> 
> The index-cache gives a situation that isn't currently accounted for: a
> BFD for which we have built the partial symtabs the first time, but has
> an index the second time.
> 
> This patch addresses it by checking for the presence of partial symtabs
> in dwarf2_initialize_objfile.  If there are, we don't try reading the
> index.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2/read.c (dwarf2_initialize_objfile): Check for presence
> 	of partial symtabs.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/index-cache-load-twice.c: New.
> 	* gdb.base/index-cache-load-twice.exp: New.
> 
> Change-Id: Ie05474c44823fcdff852b73170dd28dfd66cb6a2
> ---
>  gdb/dwarf2/read.c                             |  8 ++++
>  .../gdb.base/index-cache-load-twice.c         | 22 ++++++++++
>  .../gdb.base/index-cache-load-twice.exp       | 42 +++++++++++++++++++
>  3 files changed, 72 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/index-cache-load-twice.c
>  create mode 100644 gdb/testsuite/gdb.base/index-cache-load-twice.exp
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index e3073fe43ce3..a6b74ae4da6b 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -6027,6 +6027,14 @@ dwarf2_initialize_objfile (struct objfile *objfile, dw_index_kind *index_kind)
>        return true;
>      }
>  
> +  /* There might already be partial symtabs built for this BFD.  This happens
> +     when loading the same binary twice with the index-cache enabled.  If so,
> +     don't try to read an index.  The objfile / per_objfile initialization will
> +     be completed in dwarf2_build_psymtabs, in the standard partial symtabs
> +     code path.  */
> +  if (per_bfd->partial_symtabs != nullptr)
> +    return false;
> +
>    if (dwarf2_read_debug_names (per_objfile))
>      {
>        *index_kind = dw_index_kind::DEBUG_NAMES;
> diff --git a/gdb/testsuite/gdb.base/index-cache-load-twice.c b/gdb/testsuite/gdb.base/index-cache-load-twice.c
> new file mode 100644
> index 000000000000..9d7b2f1a4c28
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/index-cache-load-twice.c
> @@ -0,0 +1,22 @@
> +/* 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
> +main (void)
> +{
> +  return 0;
> +}

I wonder if this is generic enough to call main.c, and then perhaps we
can start reusing that one.

> diff --git a/gdb/testsuite/gdb.base/index-cache-load-twice.exp b/gdb/testsuite/gdb.base/index-cache-load-twice.exp
> new file mode 100644
> index 000000000000..4d58097529b8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/index-cache-load-twice.exp
> @@ -0,0 +1,42 @@
> +#   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/>.
> +
> +# This test checks that loading a file twice with the index cache enabled does
> +# not crash.
> +
> +standard_testfile
> +
> +lassign [remote_exec host mktemp -d] ret cache_dir
> +
> +# The output of mktemp contains an end of line, remote it.

remote -> remove.

> +set cache_dir [string trimright $cache_dir \r\n]
> +
> +if { $ret != 0 } {
> +    fail "couldn't create temporary cache dir"
> +    return
> +}
> +
> +save_vars { GDBFLAGS } {
> +    set GDBFLAGS "$GDBFLAGS -iex \"set index-cache directory $cache_dir\""
> +    set GDBFLAGS "$GDBFLAGS -iex \"set index-cache on\""
> +
> +    if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
> +	  {debug additional_flags=-Wl,--build-id}] } {
> +	return
> +    }
> +
> +    # This file command would generate an internal error.
> +    gdb_file_cmd [standard_output_file $testfile]
> +}
> 

LGTM otherwise.

Thanks,
- Tom

Re: [PATCH] gdb: check for partial symtab presence in dwarf2_initialize_objfile

[Simon Marchi]

On 2020-06-17 12:22 p.m., Tom de Vries wrote:
> On 6/12/20 11:44 PM, Simon Marchi wrote:
>> This patch fixes an internal error that is triggered when loading the
>> same binary twice with the index-cache on:
>>
>>     $ ./gdb -q -nx --data-directory=data-directory
>>     (gdb) set index-cache on
>>     (gdb) shell mktemp -d
>>     /tmp/tmp.BLgouVoPq4
>>     (gdb) set index-cache directory /tmp/tmp.BLgouVoPq4
>>     (gdb) file a.out
>>     Reading symbols from a.out...
>>     (gdb) file a.out
>>     Load new symbol table from "a.out"? (y or n) y
>>     Reading symbols from a.out...
>>     /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:2540: internal-error: void create_cus_from_index(dwarf2_per_bfd*, const gdb_byte*, offset_type, const gdb_byte*, offset_type): Assertion `per_bfd->all_comp_units.empty ()' failed.
>>     A problem internal to GDB has been detected,
>>     further debugging may prove unreliable.
>>     Quit this debugging session? (y or n)
>>
>> This is what happens:
>>
>> 1. We load the binary the first time, partial symtabs are created,
>>    per_bfd->all_comp_units is filled from those.
>> 2. Because index-cache is on, we also generate an index in the cache.
>> 3. We load the binary a second time, in dwarf2_initialize_objfile we
>>    check: was an index already loaded for this BFD?  No, so we try to
>>    read the index and fill the per-bfd using it.  We do find an index,
>>    it's in the cache.
>> 4. The function create_cus_from_index asserts (rightfully) that
>>    per_cu->all_comp_units is empty, and the assertion fails.
>>
>> This assertion verifies that we are not reading an index for a BFD for
>> which we have already built partial symtabs or read another index.
>>
>> The index-cache gives a situation that isn't currently accounted for: a
>> BFD for which we have built the partial symtabs the first time, but has
>> an index the second time.
>>
>> This patch addresses it by checking for the presence of partial symtabs
>> in dwarf2_initialize_objfile.  If there are, we don't try reading the
>> index.
>>
>> gdb/ChangeLog:
>>
>> 	* dwarf2/read.c (dwarf2_initialize_objfile): Check for presence
>> 	of partial symtabs.
>>
>> gdb/testsuite/ChangeLog:
>>
>> 	* gdb.base/index-cache-load-twice.c: New.
>> 	* gdb.base/index-cache-load-twice.exp: New.
>>
>> Change-Id: Ie05474c44823fcdff852b73170dd28dfd66cb6a2
>> ---
>>  gdb/dwarf2/read.c                             |  8 ++++
>>  .../gdb.base/index-cache-load-twice.c         | 22 ++++++++++
>>  .../gdb.base/index-cache-load-twice.exp       | 42 +++++++++++++++++++
>>  3 files changed, 72 insertions(+)
>>  create mode 100644 gdb/testsuite/gdb.base/index-cache-load-twice.c
>>  create mode 100644 gdb/testsuite/gdb.base/index-cache-load-twice.exp
>>
>> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
>> index e3073fe43ce3..a6b74ae4da6b 100644
>> --- a/gdb/dwarf2/read.c
>> +++ b/gdb/dwarf2/read.c
>> @@ -6027,6 +6027,14 @@ dwarf2_initialize_objfile (struct objfile *objfile, dw_index_kind *index_kind)
>>        return true;
>>      }
>>  
>> +  /* There might already be partial symtabs built for this BFD.  This happens
>> +     when loading the same binary twice with the index-cache enabled.  If so,
>> +     don't try to read an index.  The objfile / per_objfile initialization will
>> +     be completed in dwarf2_build_psymtabs, in the standard partial symtabs
>> +     code path.  */
>> +  if (per_bfd->partial_symtabs != nullptr)
>> +    return false;
>> +
>>    if (dwarf2_read_debug_names (per_objfile))
>>      {
>>        *index_kind = dw_index_kind::DEBUG_NAMES;
>> diff --git a/gdb/testsuite/gdb.base/index-cache-load-twice.c b/gdb/testsuite/gdb.base/index-cache-load-twice.c
>> new file mode 100644
>> index 000000000000..9d7b2f1a4c28
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/index-cache-load-twice.c
>> @@ -0,0 +1,22 @@
>> +/* 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
>> +main (void)
>> +{
>> +  return 0;
>> +}
> 
> I wonder if this is generic enough to call main.c, and then perhaps we
> can start reusing that one.

I'd probably call it empty.c.  But yeah if there are multiple tests using
a trivial source file like this, I'm not against re-using the same.  I won't
do it as part of this patch though, it can be done separately.

>> +# The output of mktemp contains an end of line, remote it.
> 
> remote -> remove.

Fixed.

>> +set cache_dir [string trimright $cache_dir \r\n]
>> +
>> +if { $ret != 0 } {
>> +    fail "couldn't create temporary cache dir"
>> +    return
>> +}
>> +
>> +save_vars { GDBFLAGS } {
>> +    set GDBFLAGS "$GDBFLAGS -iex \"set index-cache directory $cache_dir\""
>> +    set GDBFLAGS "$GDBFLAGS -iex \"set index-cache on\""
>> +
>> +    if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
>> +	  {debug additional_flags=-Wl,--build-id}] } {
>> +	return
>> +    }
>> +
>> +    # This file command would generate an internal error.
>> +    gdb_file_cmd [standard_output_file $testfile]
>> +}
>>
> 
> LGTM otherwise.

Thanks, I'll push it.

Simon