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

[Tom de Vries <tdevries@suse.de>]

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