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

[Simon Marchi <simon.marchi@efficios.com>]

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