Support for DWARF5 location lists entries

  [AMD Official Use Only - Internal Distribution Only]

Hi Simon,

Sorry. I sent the wrong patch by mistake. Updating the patch.

*Handle DW_LLE_base_addressx, DW_LLE_startx_length and DW_LLE_start_length.

Tested by running the testsuite before and after the patch and there is no increase in the number of test cases that fails.
Tested with both -gdwarf-4 and -gdwarf-5 flags. Also tested -gslit-dwarf along with -gdwarf-4 as well as -gdwarf5 flags.


gdb/ChangeLog:
        *gdb/dwarf2loc.c (decode_debug_loclists_addresses): Handle DW_LLE_base_addressx, DW_LLE_startx_length, DW_LLE_start_length.

This is an effort to support DWARF5 in gdb. diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 99cac03a54..e0940689ee 100644


Regards,
Nitika

-----Original Message-----
From: Simon Marchi <simark@simark.ca>
Sent: Wednesday, January 1, 2020 12:03 AM
To: Achra, Nitika <Nitika.Achra@amd.com>; gdb-patches@sourceware.org
Cc: George, Jini Susan <JiniSusan.George@amd.com>; Ali Tamur <tamur@google.com>
Subject: Re: [PATCH] Support for DWARF5 location lists entries

[CAUTION: External Email]

On 2019-12-31 4:41 a.m., Achra, Nitika wrote:
> [AMD Official Use Only - Internal Distribution Only]
>
>
>
> * Handle DW_LLE_base_addressx, DW_LLE_startx_length and DW_LLE_start_length.
>
>
>
> Tested by running the testsuite before and after the patch and there is no increase in the number of test cases that fails.
>
> Tested with both -gdwarf-4 and -gdwarf-5 flags.
>
>
>
>
> gdb/ChangeLog:
>         *gdb/dwarf2loc.c (decode_debug_loclists_addresses): Handle DW_LLE_base_addressx, DW_LLE_startx_length, DW_LLE_start_length.

Hi Nitika,

Thanks for the patch.  I don't have time to do an in-depth review right now, but I just wanted to point out the common formatting quirks of the GNU style, so you can apply it to this patch.

In terms of indentation, we use two columns for each indent.  However, leading groups of 8 spaces get converted to a tab.  So instead of 14 spaces, you would have one tab followed by 6 spaces to reach the desired column.  Most editors have a way to be set up like this.  Unfortunately, VSCode can't.

We use a space before opening parenthesis in function declaration/definitions/calls.
So:

  loc_ptr·=·gdb_read_uleb128(loc_ptr,·buf_end,·&u64);

would become

  loc_ptr·=·gdb_read_uleb128 (loc_ptr,·buf_end,·&u64);

This:

      if·(loc_ptr·==·NULL)
               return·DEBUG_LOC_BUFFER_OVERFLOW;

should become:

      if·(loc_ptr·==·NULL)
        return·DEBUG_LOC_BUFFER_OVERFLOW;

Simon
  

On 2020-01-06 5:14 a.m., Achra, Nitika wrote:
> [AMD Official Use Only - Internal Distribution Only]
> 
> Hi Simon,
> 
> Sorry. I sent the wrong patch by mistake. Updating the patch.
> 
> *Handle DW_LLE_base_addressx, DW_LLE_startx_length and DW_LLE_start_length.
> 
> Tested by running the testsuite before and after the patch and there is no increase in the number of test cases that fails.
> Tested with both -gdwarf-4 and -gdwarf-5 flags. Also tested -gslit-dwarf along with -gdwarf-4 as well as -gdwarf5 flags.

Hi Nitika,

Thanks, the patch LGTM, with a few formatting comments addressed.

> gdb/ChangeLog:
>         *gdb/dwarf2loc.c (decode_debug_loclists_addresses): Handle DW_LLE_base_addressx, DW_LLE_startx_length, DW_LLE_start_length.

The file in the ChangeLog entry should be relative to the ChangeLog file
it will end up in.  Since this will go in gdb/ChangeLog, it should be
"dwarf2loc.c", not "gdb/dwarf2loc.c".  Also, it should be wrapped to 80
columns.  So, something like this:

	* dwarf2loc.c (decode_debug_loclists_addresses): Handle
	DW_LLE_base_addressx, DW_LLE_startx_length, DW_LLE_start_length.

> This is an effort to support DWARF5 in gdb. diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index 99cac03a54..e0940689ee 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -173,6 +173,41 @@ decode_debug_loclists_addresses (struct dwarf2_per_cu_data *per_cu,
> 
>    switch (*loc_ptr++)
>      {
> +    case DW_LLE_base_addressx:
> +      *low = 0;
> +      loc_ptr = gdb_read_uleb128 (loc_ptr, buf_end, &u64);
> +      if (loc_ptr == NULL)
> +        return DEBUG_LOC_BUFFER_OVERFLOW;

Not sure if it's your email client that changed them to spaces, but the lines
at this indentation level (there are a few in the patch) should be indented
with a tab.

> +      *high = dwarf2_read_addr_index (per_cu, u64);
> +      *new_ptr = loc_ptr;
> +      return DEBUG_LOC_BASE_ADDRESS;
> +    case DW_LLE_startx_length:
> +      loc_ptr = gdb_read_uleb128 (loc_ptr, buf_end, &u64);
> +      if (loc_ptr == NULL)
> +        return DEBUG_LOC_BUFFER_OVERFLOW;
> +      *low = dwarf2_read_addr_index (per_cu, u64);
> +      *high = *low;
> +      loc_ptr = gdb_read_uleb128 (loc_ptr, buf_end, &u64);
> +      if (loc_ptr == NULL)
> +        return DEBUG_LOC_BUFFER_OVERFLOW;
> +      *high += u64;
> +      *new_ptr = loc_ptr;
> +      return DEBUG_LOC_START_LENGTH;

Something I noticed while reviewing, which could be a possible cleanup made
in a separate patch if you'd like.  I don't really see why DEBUG_LOC_START_LENGTH
exists.  As we return the low and high addresses to the caller, it's no different
than DEBUG_LOC_START_END.  And indeed, the callers treat both DEBUG_LOC_START_END
and DEBUG_LOC_START_LENGTH the same way.  So I think _LENGTH could be removed.

And since `enum debug_loc_kind` is only used internally in GDB to communicate
between functions (it's not used for parsing anything), it doesn't really need
to have explicit values for enumerators.  The documentation about the encoding
also seems irrelevant to me, since, again, they are not even used for parsing
anything.

> +    case DW_LLE_start_length:
> +      if (buf_end - loc_ptr < addr_size)
> +        return DEBUG_LOC_BUFFER_OVERFLOW;
> +      if (signed_addr_p)
> +        *low = extract_signed_integer (loc_ptr, addr_size, byte_order);
> +      else
> +        *low = extract_unsigned_integer (loc_ptr, addr_size, byte_order);
> +      loc_ptr += addr_size;
> +      *high = *low;
> +      loc_ptr = gdb_read_uleb128 (loc_ptr, buf_end, &u64);
> +      if (loc_ptr == NULL)
> +        return DEBUG_LOC_BUFFER_OVERFLOW;
> +      *high += u64;
> +      *new_ptr = loc_ptr;
> +      return DEBUG_LOC_START_LENGTH;
>      case DW_LLE_end_of_list:
>        *new_ptr = loc_ptr;
>        return DEBUG_LOC_END_OF_LIST;
> @@ -197,6 +232,9 @@ decode_debug_loclists_addresses (struct dwarf2_per_cu_data *per_cu,
>        *high = u64;
>        *new_ptr = loc_ptr;
>        return DEBUG_LOC_START_END;
> +    case DW_LLE_startx_endx:
> +    case DW_LLE_start_end:
> +    case DW_LLE_default_location:

Did you add them here because you don't intend to add support for them, at least
for the moment?  If so, please add a comment, like:

/* Not supported yet.  */

Thanks,

Simon