On 7/30/21 3:25 AM, Simon Marchi wrote:
> 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
> 

Fixed.

>>   (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?
> 

I build gdb 9.2 and did observe that the warning / Internal error does
not trigger there.  I have not yet checked this in more detail, but that
might be because of what you suggest here.

>>
>> 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?
> 

Done.

>> 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?
> 

Done.

>> +
>> +# 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.

Done.

Thanks for the review, and the .debug_rnglists addition.

I've added the .debug_rnglists addition here as well, and added testing
the complaint when doing gdb_load $lib1, in other words, the unrelocated
case (which was already passing before this patch).

Thanks,
- Tom