Re: [PATCH] Support for DWARF5 location lists entries

[Simon Marchi <simark@simark.ca>]

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