diff mbox

Support for DWARF5 location lists entries

Message ID MN2PR12MB3744B5CD29DB4D15BC9A4F4F9A340@MN2PR12MB3744.namprd12.prod.outlook.com
State New, archived
Headers

Commit Message

Achra, Nitika Jan. 14, 2020, 7:02 a.m. UTC 
  [AMD Official Use Only - Internal Distribution Only]

Hi Simon,

Sorry, I missed the comment. And I will try to setup the git-send-email before the next patch. Thank you.

*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:
        *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.


Regards,
Nitika

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

[CAUTION: External Email]

On 2020-01-13 5:45 a.m., Achra, Nitika wrote:
> [AMD Official Use Only - Internal Distribution Only]
>
>
>
> Hi Simon,
>
> Thanks for the detailed review.
>
> ***
>> +        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.
>
> I was sending the mail in HTML format which is converting tabs to spaces while sending. Now, I am sending in plain text format. It should fix the issue.

Thanks.  The best is to use git-send-email, which makes git send the patch by email directly, in the right format.  It requires a bit of setup, since you need to specify your SMTP server settings, but it's really worth it.

If you can't use git-send-email for some reason, the second best option is git-format-patch.  git-format-patch will produce a complete patch, including the subject and the commit message.  So if you want to send your patch as an attachment or paste it in an email, please use git-format-patch instead of git-diff, which just produces the diff, without the commit message.

Finally, if you manually paste your patch in an email, then you need to make sure the email client doesn't change anything (like break long lines), otherwise the patch will be corrupted.

> ***
>> +      *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.
>
> I will made this cleanup in the separate patch.

Thanks!

> ***
>> +    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.  */
>
> Done. Clang and gcc are not emitting the above three entries as of now. So, I didn't add the support for them.

Hmm, I don't see the comment in the new version of the patch.

Simon

Comments

Simon Marchi Jan. 14, 2020, 4:28 p.m. UTC | #1 
On 2020-01-14 2:02 a.m., Achra, Nitika wrote:
> [AMD Official Use Only - Internal Distribution Only]
> 
> Hi Simon,
> 
> Sorry, I missed the comment. And I will try to setup the git-send-email before the next patch. Thank you.

Thanks.

I could push this patch on your behalf, but I am having trouble applying it, probably
because your client (I'm guessing outlook?) has messed with it.  Could you please
generate it with git-format-patch and send it as an attachment?  This way it should
arrive intact.

Simon
Achra, Nitika Jan. 16, 2020, 11:29 a.m. UTC | #2 
[AMD Official Use Only - Internal Distribution Only]

Hi Simon,

Please find the attachment.


*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:
        *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.

Regards,
Nitika Achra

-----Original Message-----
From: Simon Marchi <simark@simark.ca> 

Sent: Tuesday, January 14, 2020 9:59 PM
To: Achra, Nitika <Nitika.Achra@amd.com>; gdb-patches@sourceware.org
Cc: Ali Tamur <tamur@google.com>; George, Jini Susan <JiniSusan.George@amd.com>
Subject: Re: [PATCH] Support for DWARF5 location lists entries

[CAUTION: External Email]

On 2020-01-14 2:02 a.m., Achra, Nitika wrote:
> [AMD Official Use Only - Internal Distribution Only]

>

> Hi Simon,

>

> Sorry, I missed the comment. And I will try to setup the git-send-email before the next patch. Thank you.


Thanks.

I could push this patch on your behalf, but I am having trouble applying it, probably because your client (I'm guessing outlook?) has messed with it.  Could you please generate it with git-format-patch and send it as an attachment?  This way it should arrive intact.

Simon
Simon Marchi Jan. 16, 2020, 2:15 p.m. UTC | #3 
On 2020-01-16 6:29 a.m., Achra, Nitika wrote:
> [AMD Official Use Only - Internal Distribution Only]
> 
> Hi Simon,
> 
> Please find the attachment.
> 
> 
> *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:
>         *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.

Hi Achra,

The commit message of the patch is empty.  The text that is above, which describes what
your patch does, should be placed in your git commit, such that it will appear in the
patch file when you use "git format-patch".

This is the text that people will see when they do "git log", so you can do "git log"
to see how it's usually formatted.

Thanks,

Simon
Simon Marchi Jan. 16, 2020, 2:16 p.m. UTC | #4 
On 2020-01-16 9:15 a.m., Simon Marchi wrote:
> On 2020-01-16 6:29 a.m., Achra, Nitika wrote:
>> [AMD Official Use Only - Internal Distribution Only]
>>
>> Hi Simon,
>>
>> Please find the attachment.
>>
>>
>> *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:
>>         *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.
> 
> Hi Achra,

Err sorry, I meant "Hi Nitika"!
Achra, Nitika Jan. 16, 2020, 3:30 p.m. UTC | #5 
[AMD Official Use Only - Internal Distribution Only]

Hi Simon,

I have modified the commit message. Please find the attachment.

Regards,
Nitika Achra

-----Original Message-----
From: Simon Marchi <simark@simark.ca> 

Sent: Thursday, January 16, 2020 7:45 PM
To: Achra, Nitika <Nitika.Achra@amd.com>; gdb-patches@sourceware.org
Cc: Ali Tamur <tamur@google.com>; George, Jini Susan <JiniSusan.George@amd.com>
Subject: Re: [PATCH] Support for DWARF5 location lists entries

[CAUTION: External Email]

On 2020-01-16 6:29 a.m., Achra, Nitika wrote:
> [AMD Official Use Only - Internal Distribution Only]

>

> Hi Simon,

>

> Please find the attachment.

>

>

> *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:

>         *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.


Hi Achra,

The commit message of the patch is empty.  The text that is above, which describes what your patch does, should be placed in your git commit, such that it will appear in the patch file when you use "git format-patch".

This is the text that people will see when they do "git log", so you can do "git log"
to see how it's usually formatted.

Thanks,

Simon
Simon Marchi Jan. 16, 2020, 5:22 p.m. UTC | #6 
On 2020-01-16 10:30 a.m., Achra, Nitika wrote:
> [AMD Official Use Only - Internal Distribution Only]
> 
> Hi Simon,
> 
> I have modified the commit message. Please find the attachment.
> 
> Regards,
> Nitika Achra

Thanks!  I have pushed the patch on your behalf.

Simon
diff mbox

Patch

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 99cac03a54..dd99ea35c0 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;
+      *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;
+    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;
+    /* Following cases are not supported yet. */
+    case DW_LLE_startx_endx:
+    case DW_LLE_start_end:
+    case DW_LLE_default_location:
     default:
       return DEBUG_LOC_INVALID_ENTRY;
     }