Re: [PATCH master + 11][gdb/symtab] Fix zero address complaint for shlib

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2021-07-29 5:23 p.m., Tom de Vries wrote:
> Hi,
> 
> In PR28004 the following warning / Internal error is reported:
> ...
> $ gdb -q -batch \
>     -iex "set sysroot $(pwd -P)/repro" \
>     ./repro/gdb \
>     ./repro/core \
>     -ex bt
>   ...
>  Program terminated with signal SIGABRT, Aborted.
>  #0  0x00007ff8fe8e5d22 in raise () from repro/usr/lib/libc.so.6
>  [Current thread is 1 (LWP 1762498)]
>  #1  0x00007ff8fe8cf862 in abort () from repro/usr/lib/libc.so.6
>  warning: (Internal error: pc 0x7ff8feb2c21d in read in psymtab, \
>            but not in symtab.)
>  warning: (Internal error: pc 0x7ff8feb2c218 in read in psymtab, \
>            but not in symtab.)
>   ...
>  #2  0x00007ff8feb2c21e in __gnu_debug::_Error_formatter::_M_error() const \
>    [clone .cold] (warning: (Internal error: pc 0x7ff8feb2c21d in read in \
>    psymtab, but not in symtab.)
> 
> ) from repro/usr/lib/libstdc++.so.6
> ...
> 
> The warning is about the following:
> - in find_pc_sect_compunit_symtab we try to find the addres

addres -> address

>   (0x7ff8feb2c218 / 0x7ff8feb2c21d) in the symtabs.
> - that fails, so we try again in the partial symtabs.
> - we find a matching partial symtab
> - however, the partial symtab has a matching symtab, so
>   we should have found a matching symtab in the first step.
> 
> The addresses are:
> ...
> (gdb) info sym 0x7ff8feb2c218
> __gnu_debug::_Error_formatter::_M_error() const [clone .cold] in \
>   section .text of repro/usr/lib/libstdc++.so.6
> (gdb) info sym 0x7ff8feb2c21d
> __gnu_debug::_Error_formatter::_M_error() const [clone .cold] + 5 in \
>   section .text of repro/usr/lib/libstdc++.so.6
> ...
> which correspond to unrelocated addresses 0x9c218 and 0x9c21d:
> ...
> $ nm -C  repro/usr/lib/libstdc++.so.6.0.29 | grep 000000000009c218
> 000000000009c218 t __gnu_debug::_Error_formatter::_M_error() const \
>   [clone .cold]
> ...
> 
> The unrelocated addresses can be found in the partial symbols addresss map:
> ...
> (gdb) set $map = (addrmap_fixed *)m_partial_symtabs->psymtabs_addrmap
> (gdb) p /x $map->transitions[24]
> $47 = {addr = 0x9989c, value = 0x231d1b0}
> (gdb) p /x $map->transitions[25]
> $48 = {addr = 0xa2170, value = 0x5439980}
> ...
> and indeed we do:
> ...
> (gdb) p ps
> $1 = (partial_symtab *) 0x231d1b0
> ...
> but not in the symbols address map:
> ...
> (gdb) set $symtab = ps->get_compunit_symtab (objfile)
> (gdb) set $map = (addrmap_fixed *)$symtab.blockvector.map
> (gdb) p /x $map.transitions[20]
> $42 = {addr = 0x7ff8feb2993a, value = 0x0}
> (gdb) p /x $map.transitions[21]
> $43 = {addr = 0x7ff8feb322b0, value = 0x6155ef0}
> ...
> 
> The reason for the difference is between the two address maps (who are based on
> the same data), is this code in dwarf2_rnglists_process:
> ...
>       /* A not-uncommon case of bad debug info.
>          Don't pollute the addrmap with bad data.  */
>       if (range_beginning + baseaddr == 0
>           && !per_objfile->per_bfd->has_section_at_zero)
>         {
>           complaint (_(".debug_rnglists entry has start address of zero"
>                        " [in module %s]"), objfile_name (objfile));
>           continue;
>         }
> ...
> which triggers for the partial symbol table case (with unrelocated addresses),
> but not for the symbol table case (with relocated addresses).

Interesting.  Before Tom Tromey's changes to make partial symtabs
shareable between objfiles, partial symbols were also relocated right?
So it would mean that the `range_beginning + baseaddr == 0` was wrong
for both the partial and full symtabs, but with that change it became
right for the partial symtabs?

> 
> The difference is that in the latter case, baseaddr (initialized from
> objfile->text_section_offset ()) isn't 0.
> 
> Fix this by removing the baseaddr part from the condition.  Same for
> dwarf2_ranges_process.

The change looks good to me.  I am still confused by these weird ranges
starting at zero, but given that the check is already present, let's
make it right.

> The test-case excercises the latter only for now.
> 
> Tested on x86_64-linux.
> 
> Any comments?

Maybe state in the commit message what impact this patch has.  It gets
rid of the warning shown above?

> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-zero-range.exp b/gdb/testsuite/gdb.dwarf2/dw2-zero-range.exp
> new file mode 100644
> index 00000000000..1805733db6c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-zero-range.exp
> @@ -0,0 +1,97 @@
> +# Copyright 2015-2021 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/>.
> +load_lib dwarf.exp

Can you please add a short comment explaining what this test intends to
test?

> +
> +# This test can only be run on targets which support DWARF-2 and use gas.
> +if {![dwarf2_support]} {
> +    verbose "Skipping $gdb_test_file_name."
> +    return 0
> +}
> +
> +if {[skip_shlib_tests]} {
> +    return 0
> +}
> +
> +# The .c files use __attribute__.
> +if [get_compiler_info] {
> +    return -1
> +}
> +if !$gcc_compiled {
> +    verbose "Skipping $gdb_test_file_name."
> +    return 0
> +}
> +
> +standard_testfile .c -shlib.c -dw.S
> +
> +set asm_file [standard_output_file $srcfile3]
> +Dwarf::assemble $asm_file {
> +    global srcdir subdir srcfile2
> +    declare_labels ranges_label
> +
> +    cu {} {
> +	compile_unit {
> +	    {language @DW_LANG_C}
> +	    {name $srcfile2}
> +	    {ranges ${ranges_label} DW_FORM_sec_offset}
> +	} {
> +	    subprogram {
> +		{external 1 flag}
> +		{name foo}
> +	    }
> +	}
> +    }
> +
> +    ranges {is_64 [is_64_target]} {
> +	ranges_label: sequence {
> +	    base 0
> +	    range 0 1
> +	}
> +    }
> +}
> +
> +set lib1 [standard_output_file shr1.sl]
> +set lib_opts "nodebug"
> +
> +set sources [list ${srcdir}/${subdir}/$srcfile2 $asm_file]
> +if { [gdb_compile_shlib $sources ${lib1} $lib_opts] != "" } {
> +    untested "failed to compile"
> +    return -1
> +}
> +
> +set exec_opts [list debug shlib=${lib1}]
> +set sources ${srcdir}/${subdir}/${srcfile}
> +if { [gdb_compile $sources  ${binfile} executable \
> +	  $exec_opts] != ""} {
> +    untested "failed to compile"
> +    return -1
> +}
> +
> +clean_restart $binfile
> +
> +# Don't load the symbols for $lib1 during runto_main.
> +# Instead, we do this afterwards using "sharedlibrary $lib1".
> +gdb_test_no_output "set auto-solib-add off"
> +
> +if ![runto_main] {
> +    return -1
> +}

I think runto_main does not (yet) produce a FAIL on failure, hence why
we need `fail "could not run to main"` everywhere.

Simon