Re: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.

[Jan Beulich <jbeulich@suse.com>]

On 05.07.2022 08:46, Potharla, Rupesh via Binutils wrote:
> Patch Inline:

Please note how the inlined version has extra blank lines inserted
between any two actual lines, whereas the attached variant is a
*.docx rather than a *.patch. I'll use the inlined version for the
few comments / questions I have, but you will want to sort your mail
setup.

> --- a/bfd/dwarf2.c
> 
> +++ b/bfd/dwarf2.c
> 
> @@ -1369,7 +1369,7 @@ read_indexed_address (uint64_t idx, struct comp_unit *unit)
> 
>                             &file->dwarf_addr_buffer, &file->dwarf_addr_size))
> 
>      return 0;
> 
> -  if (_bfd_mul_overflow (idx, unit->offset_size, &offset))
> 
> +  if (_bfd_mul_overflow (idx, unit->addr_size, &offset))
> 
>      return 0;
> 
>    offset += unit->dwarf_addr_offset;
> 
> @@ -1380,9 +1380,9 @@ read_indexed_address (uint64_t idx, struct comp_unit *unit)
> 
>    info_ptr = file->dwarf_addr_buffer + offset;
> 
> -  if (unit->offset_size == 4)
> 
> +  if (unit->addr_size == 4)
> 
>      return bfd_get_32 (unit->abfd, info_ptr);
> 
> -  else if (unit->offset_size == 8)
> 
> +  else if (unit->addr_size == 8)
> 
>      return bfd_get_64 (unit->abfd, info_ptr);
> 
>    else
> 
>      return 0;
> 

I have to admit that I don't see the (direct) connection to the
patch title here - are these changes perhaps better to form a
standalone change?

> @@ -1951,6 +1952,8 @@ concat_filename (struct line_info_table *table, unsigned int file)
> 
> {
> 
>    char *filename;
> 
> +  if (table->version >= 5)
> 
> +    file = file + 1;
> 

Don't you need either this ...

>    if (table == NULL || file - 1 >= table->num_files)
> 
>      {
> 
>        /* FILE == 0 means unknown.  */
> 
> @@ -2579,10 +2582,8 @@ read_formatted_entries (struct comp_unit *unit, bfd_byte **bufp,
> 
>                }
> 
>            }
> 
> -      /* Skip the first "zero entry", which is the compilation dir/file.  */
> 
> -      if (datai != 0)
> 
> -           if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
> 
> -             return false;
> 
> +      if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
> 
> +          return false;
> 
>      }

... or this adjustment, but not both at the same time?

> --- /dev/null
> 
> +++ b/binutils/testsuite/binutils-all/addr2line.exp
> 
> @@ -0,0 +1,59 @@
> 
> +#   Copyright (C) 2018-2022 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, write to the Free Software
> 
> +# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.
> 
> +
> 
> +    global $NM
> 
> +    global $ADDR2LINE
> 
> +
> 
> +    set testname "addr2line"
> 
> +    if { [target_compile $srcdir/$subdir/testprog.c tmpdir/testprog executable debug] != "" } {

Could you help me spotting where the Dwarf version to be used it coming
from? (Ideally the new test would be run once for v5 and once with an
earlier version; yet more ideally once per version. Yet I'm not sure
how easy this would be to achieve.)

> +        verbose "Unable to compile test file."
> 
> +        untested "addr2line"
> 
> +        return
> 
> +    }
> 
> +
> 
> +    #testcase for default option.
> 
> +    set output [binutils_run $NM "tmpdir/testprog"]
> 
> +    regexp -line {^[0-9]+\s+[A-Z]\s+main} $output contents
> 
> +    set list [regexp -inline -all -- {\S+} $contents]
> 
> +
> 
> +    set got [binutils_run $ADDR2LINE "-e tmpdir/testprog  [lindex $list 0]"]
> 
> +    set want "$srcdir/$subdir/testprog.c:\[0-9\]+"
> 
> +    if ![regexp $want $got] then {
> 
> +        fail "$testname $got\n"
> 
> +    } else {
> 
> +        pass "$testname"
> 
> +    }

I have to admit that I can't really spot what you're actually looking
for, and hence whether that's sufficient for a test. Rather than all of
this open-coding, did you consider using the pre-cooked run_dump_test?

Jan