Re: [PATCH] Binutils support for dwarf-5 (location and range lists related)

[Jan Beulich <jbeulich@suse.com>]

On 31.05.2022 13:06, Kumar N, Bhuvanendra via Binutils wrote:
> PATCH 1/2 inlined :
> 
> From 96ce32f762803a7d74ca30c3930f1679afc0100c Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?=E2=80=9Cbhkumarn=E2=80=9D?= Bhuvanendra.KumarN@amd.com<mailto:Bhuvanendra.KumarN@amd.com>
> Date: Tue, 31 May 2022 14:07:17 +0530
> Subject: [PATCH] [PATCH 1/2] Binutils support for dwarf-5.
> 
> For clang compiled with dwarf-5, multiple issues are fixed which are
> related to .debug_rnglists and .debug_loclists sections and their offset
> address dump comprising single and multiple CU. There are 2 patches
> and this is patch 1/2.
> 
> Issues fixed in patch 1/2 are:
> Issue 1: .debug_rnglists section dump for single CU.
> Issue 2: location list offset address dump under DW_AT_location is corrected.
> Issue 3: range list offset address dump under DW_AT_ranges is corrected.

To be honest I don't think it's helpful to put these all in a single
patch. They're not directly related, and if I'm getting it right the
code changes are also independent of one another. This would also
allow to approve one part while another still needs clarification or
refinement.

And then it would also help if you could send multiple patches
properly as a series, rather than all in a single email.

> @@ -2788,7 +2794,15 @@ read_and_display_attr_value (unsigned long           attribute,
>         offset = base + uvalue * pointer_size;
> -       if (do_wide)
> +       if (form == DW_FORM_loclistx)
> +         printf (_("%c(index: 0x%s): %s"), delimiter,
> +                 dwarf_vmatoa ("x", uvalue),
> +                 dwarf_vmatoa ("x", debug_info_p->loc_offsets [uvalue]));

Aren't you risking an oob array access here? And isn't the ordering of
elements in loc_offset[] an implementation detail, i.e. using a value
fetched from debug info is effectively meaningless when used as an
index into the array?

> +       else if (form == DW_FORM_rnglistx)
> +         printf (_("%c(index: 0x%s): %s"), delimiter,
> +                 dwarf_vmatoa ("x", uvalue),
> +                 dwarf_vmatoa ("x", fetch_indexed_value (uvalue, rnglists, 0)));
> +       else if (do_wide)
>          /* We have already displayed the form name.  */
>          printf (_("%c(index: 0x%s): %s"), delimiter,
>               dwarf_vmatoa ("x", uvalue),

For both special cases, how come they don't respect do_wide?

> @@ -7976,9 +7990,6 @@ display_debug_rnglists_list (unsigned char * start,
>      case DW_RLE_base_addressx:
>        READ_ULEB (base_address, start, finish);
>        print_dwarf_vma (base_address, pointer_size);
> -       printf (_("(base address index) "));
> -       base_address = fetch_indexed_addr (base_address, pointer_size);
> -       print_dwarf_vma (base_address, pointer_size);
>        printf (_("(base address)\n"));
>        break;
>      case DW_RLE_startx_endx:
> @@ -7990,7 +8001,6 @@ display_debug_rnglists_list (unsigned char * start,
>      case DW_RLE_startx_length:
>        READ_ULEB (begin, start, finish);
>        READ_ULEB (length, start, finish);
> -       begin = fetch_indexed_addr (begin, pointer_size);
>        end = begin + length;
>        break;
>      case DW_RLE_offset_pair:

I don't see how these two hunks can be correct: You're removing the
indirection through .debug_addr. And at the same time you're keeping
similar indirection for DW_RLE_startx_endx. Aiui all x-suffixed items
are to be dealt with identically.

> @@ -8243,7 +8253,7 @@ display_debug_ranges (struct dwarf_section *section,
>           (unsigned long) offset, i);
>        continue;
>      }
> -      next = section_begin + offset;
> +      next = section_begin + offset + DEBUG_LOCLISTS_RNGLISTS_SECTION_HEADER_LEN;

I'm afraid I can't figure what 12 bytes you're meaning to skip here.
I also think that _if_ an adjustment was needed here, the preceding
if() likely would also need adjustment, perhaps by way of actually
adjusting offset.

Jan