Re: [PATCH] Handle DW_AT_ranges when reading partial symtabs

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

On 2019-03-29 16:00, Tom Tromey wrote:
> add_partial_subprogram does not handle DW_AT_ranges, while the full
> symtab reader does.  This can lead to discrepancies where a function
> is not put into a partial symtab, and so is not available to "break"
> and the like -- but is available if the full symtab has somehow been
> read.
> 
> This patch fixes the bug by arranging to read DW_AT_ranges when
> reading partial DIEs.
> 
> This is PR symtab/23331.
> 
> The new test case is derived from dw2-ranges-func.exp, which is why I
> kept the copyright dates.
> 
> gdb/ChangeLog
> 2019-03-29  Tom Tromey  <tromey@adacore.com>
> 
> 	PR symtab/23331:
> 	* dwarf2read.c (partial_die_info::read): Handle DW_AT_ranges.
> 
> gdb/testsuite/ChangeLog
> 2019-03-29  Tom Tromey  <tromey@adacore.com>
> 
> 	PR symtab/23331:
> 	* gdb.dwarf2/dw2-ranges-main.c: New file.
> 	* gdb.dwarf2/dw2-ranges-psym.c: New file.
> 	* gdb.dwarf2/dw2-ranges-psym.exp: New file.

I don't have much experience in this area, but I read the patch and it 
seems to make sense.

I just have one tiny comment to help readability in the test.

> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-ranges-psym.exp
> b/gdb/testsuite/gdb.dwarf2/dw2-ranges-psym.exp
> new file mode 100644
> index 00000000000..eb1130a2238
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-ranges-psym.exp
> @@ -0,0 +1,134 @@
> +# Copyright 2018-2019 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
> +
> +# Test that psymbols are made when DW_AT_ranges is used.
> +
> +# This test can only be run on targets which support DWARF-2 and use 
> gas.
> +if {![dwarf2_support]} {
> +    unsupported "dwarf2 support required for this test"
> +    return 0
> +}
> +
> +if [get_compiler_info] {
> +    return -1
> +}
> +if !$gcc_compiled {
> +    unsupported "gcc required for this test"
> +    return 0
> +}
> +
> +standard_testfile dw2-ranges-main.c dw2-ranges-psym.c 
> dw2-ranges-psym-dw.S
> +
> +# We need to know the size of integer and address types in order to
> +# write some of the debugging info we'd like to generate.
> +#
> +# For that, we ask GDB by debugging our test program.  Any program
> +# would do, but since we already have it specifically for this
> +# testcase, might as well use that.
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} \
> +	  [list ${srcfile} ${srcfile2}]] } {
> +    return -1
> +}
> +
> +set asm_file [standard_output_file $srcfile3]
> +Dwarf::assemble $asm_file {
> +    global srcdir subdir srcfile srcfile2
> +    declare_labels integer_label volatile_label func_ranges_label
> cu_ranges_label
> +    set int_size [get_sizeof "int" 4]
> +
> +    # Find start address and length for our functions.
> +    set sources  [list ${srcdir}/${subdir}/$srcfile
> ${srcdir}/${subdir}/$srcfile2]
> +    lassign [function_range foo $sources] \
> +	foo_start foo_len
> +    set foo_end "$foo_start + $foo_len"
> +    lassign [function_range foo_low $sources] \
> +	foo_low_start foo_low_len
> +    set foo_low_end "$foo_low_start + $foo_low_len"
> +    lassign [function_range bar $sources] \
> +	bar_start bar_len
> +    set bar_end "$bar_start + $bar_len"
> +    lassign [function_range baz $sources] \
> +	baz_start baz_len
> +    set baz_end "$baz_start + $baz_len"

To help readability, I'd suggest adding some empty lines in there, 
between each pair of lassign+set statements.  At first it looked like a 
bit cryptic blob to me until I split it in smaller pieces.

Simon