[PATCH] Fix dwarf2_string_attr for -gsplit-dwarf

[PATCH] Fix dwarf2_string_attr for -gsplit-dwarf

[Leszek Swirski via gdb-patches]

The dwarf2_string_attr did not allow DW_FORM_GNU_str_index as a form for
string types. This manifested as null strings in the namespace_name
lookup (replaced with "(anonymous namespace)") when debugging
Fission-compiled code.

gdb/ChangeLog:

	* dwarf2read.c (dwarf2_string_attr): Allow DW_FORM_GNU_strp_alt.

(cherry picked from commits 16eb6b2db49e6cf2fdca56efd37689fcc170cd37 and
b33404388e5bbd8a1fddfde73cd4593ae2b557e8)
---
 gdb/ChangeLog    | 4 ++++
 gdb/dwarf2read.c | 4 +++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b77435ce7a..bfa2359a35 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2017-10-25  Leszek Swirski  <leszeks@google.com>
+
+	* dwarf2read.c (dwarf2_string_attr): Allow DW_FORM_GNU_str_index.
+
 2017-10-16  Walfred Tedeschi  <walfred.tedeschi@intel.com>
 
 	* features/Makefile (i386-avx-mpx-avx512-pku.dat): Add backslash 
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 8503e6171c..93ec587787 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -17577,7 +17577,9 @@ dwarf2_string_attr (struct die_info *die, unsigned int name, struct dwarf2_cu *c
   if (attr != NULL)
     {
       if (attr->form == DW_FORM_strp || attr->form == DW_FORM_line_strp
-	  || attr->form == DW_FORM_string || attr->form == DW_FORM_GNU_strp_alt)
+	  || attr->form == DW_FORM_string
+	  || attr->form == DW_FORM_GNU_str_index
+	  || attr->form == DW_FORM_GNU_strp_alt)
 	str = DW_STRING (attr);
       else
         complaint (&symfile_complaints,
-- 
2.15.0.rc2.357.g7e34df9404-goog

Re: [PATCH] Fix dwarf2_string_attr for -gsplit-dwarf

[Yao Qi]

"Leszek Swirski via gdb-patches" <gdb-patches@sourceware.org> writes:

> gdb/ChangeLog:
>
> 	* dwarf2read.c (dwarf2_string_attr): Allow DW_FORM_GNU_strp_alt.

s/DW_FORM_GNU_strp_alt/DW_FORM_GNU_str_index

Otherwise, patch is good to me.

-- 
Yao (齐尧)

Re: [PATCH] Fix dwarf2_string_attr for -gsplit-dwarf

[Simon Marchi]

On 2017-10-25 05:24, Leszek Swirski via gdb-patches wrote:
> The dwarf2_string_attr did not allow DW_FORM_GNU_str_index as a form 
> for
> string types. This manifested as null strings in the namespace_name
> lookup (replaced with "(anonymous namespace)") when debugging
> Fission-compiled code.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2read.c (dwarf2_string_attr): Allow DW_FORM_GNU_strp_alt.
> 
> (cherry picked from commits 16eb6b2db49e6cf2fdca56efd37689fcc170cd37 
> and
> b33404388e5bbd8a1fddfde73cd4593ae2b557e8)
> ---
>  gdb/ChangeLog    | 4 ++++
>  gdb/dwarf2read.c | 4 +++-
>  2 files changed, 7 insertions(+), 1 deletion(-)

Hi Leszek,

It's not mentioned, but from the context of the ChangeLog change, I 
understand this is for the 8.0 branch?  8.0.1 was already released, and 
I don't know if there are plans for a 8.0.2.  But in any case, 
cherry-picking this fix doesn't hurt for anybody building from the 
gdb-8.0-branch.

Thanks,

Simon

Re: [PATCH] Fix dwarf2_string_attr for -gsplit-dwarf

[Leszek Swirski via gdb-patches]

> It's not mentioned, but from the context of the ChangeLog change, I
> understand this is for the 8.0 branch?  8.0.1 was already released, and I
> don't know if there are plans for a 8.0.2.  But in any case, cherry-picking
> this fix doesn't hurt for anybody building from the gdb-8.0-branch.

Yes, sorry, I should have made that clearer.

Re: [PATCH] Fix dwarf2_string_attr for -gsplit-dwarf

[Simon Marchi]

On 2017-10-25 11:27, Leszek Swirski via gdb-patches wrote:
>> It's not mentioned, but from the context of the ChangeLog change, I
>> understand this is for the 8.0 branch?  8.0.1 was already released, 
>> and I
>> don't know if there are plans for a 8.0.2.  But in any case, 
>> cherry-picking
>> this fix doesn't hurt for anybody building from the gdb-8.0-branch.
> 
> Yes, sorry, I should have made that clearer.

I pushed the patch and added a reference to the original PR in the 
ChangeLog.

Thanks!

Simon

Re: [PATCH] Fix dwarf2_string_attr for -gsplit-dwarf

[Leszek Swirski via gdb-patches]

> I pushed the patch and added a reference to the original PR in the
> ChangeLog.

Thanks Simon.

[PATCH] Fix dwarf2_string_attr for -gsplit-dwarf

[Leszek Swirski via gdb-patches]

The dwarf2_string_attr did not allow DW_FORM_GNU_str_index as a form for
string types. This manifested as null strings in the namespace_name
lookup (replaced with "(anonymous namespace)") when debugging
Fission-compiled code.

gdb/doc/ChangeLog:

	* dwarf2read.c: Fix dwarf2_string_attr to allow
	DW_FORM_GNU_str_index
---
 gdb/dwarf2read.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 2c2ecda7fc..f5bed09116 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -17623,7 +17623,8 @@ dwarf2_string_attr (struct die_info *die, unsigned int name, struct dwarf2_cu *c
   if (attr != NULL)
     {
       if (attr->form == DW_FORM_strp || attr->form == DW_FORM_line_strp
-	  || attr->form == DW_FORM_string || attr->form == DW_FORM_GNU_strp_alt)
+	  || attr->form == DW_FORM_string || DW_FORM_GNU_str_index
+          || attr->form == DW_FORM_GNU_strp_alt)
 	str = DW_STRING (attr);
       else
         complaint (&symfile_complaints,
-- 
2.14.0.rc0.400.g1c36432dff-goog

Re: [PATCH] Fix dwarf2_string_attr for -gsplit-dwarf

[Simon Marchi]

Hi Leszek,

On 2017-08-01 11:20, Leszek Swirski via gdb-patches wrote:
> The dwarf2_string_attr did not allow DW_FORM_GNU_str_index as a form 
> for
> string types. This manifested as null strings in the namespace_name
> lookup (replaced with "(anonymous namespace)") when debugging
> Fission-compiled code.

Thanks for the patch.  I am not very knowledgeable in that area, but I 
looked at the problem for a while and I think the change is good.  
DW_STRING (attr) is set for attribute values of this form in 
read_attribute_value.  Other functions like dwarf2_const_value_attr and 
dump_die_shallow read DW_STRING (attr) when dealing with an attribute of 
form DW_FORM_GNU_str_index as well.

If you think this will be a one-time contribution, we can merge the 
patch for you, and there is no need for copyright assignment for a 
simple patch like that.  However, if you'd like to contribute further to 
GDB, we can look into giving you write access, so you'll be able to push 
patches by yourself.  Let me know which one you prefer.

I just have some formatting comments about the patch itself.  No need 
for a v2, especially if we push for you, but just so you are aware of 
the GNU/GDB coding standards.

> gdb/doc/ChangeLog:

The ChangeLog entry for this patch will go in the gdb/ChangeLog file.

> 
> 	* dwarf2read.c: Fix dwarf2_string_attr to allow
> 	DW_FORM_GNU_str_index

The ChangeLog format has the function name in parentheses, for example:

   * dwarf2read.c (dwarf2_string_attr): Allow DW_FORM_GNU_strp_alt.

> ---
>  gdb/dwarf2read.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 2c2ecda7fc..f5bed09116 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -17623,7 +17623,8 @@ dwarf2_string_attr (struct die_info *die,
> unsigned int name, struct dwarf2_cu *c
>    if (attr != NULL)
>      {
>        if (attr->form == DW_FORM_strp || attr->form == 
> DW_FORM_line_strp
> -	  || attr->form == DW_FORM_string || attr->form == 
> DW_FORM_GNU_strp_alt)
> +	  || attr->form == DW_FORM_string || DW_FORM_GNU_str_index
> +          || attr->form == DW_FORM_GNU_strp_alt)

The indentation for this line should be one tab + two spaces (like the 
previous line).

Thanks,

Simon

Re: [PATCH] Fix dwarf2_string_attr for -gsplit-dwarf

[Leszek Swirski via gdb-patches]

On Tue, Aug 1, 2017 at 9:20 PM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
> Thanks for the patch.  I am not very knowledgeable in that area, but I
> looked at the problem for a while and I think the change is good.  DW_STRING
> (attr) is set for attribute values of this form in read_attribute_value.
> Other functions like dwarf2_const_value_attr and dump_die_shallow read
> DW_STRING (attr) when dealing with an attribute of form
> DW_FORM_GNU_str_index as well.

Thanks, I'm not particularly knowledgeable here either, but this was pretty
much the thread that I pulled on.

> If you think this will be a one-time contribution, we can merge the patch
> for you, and there is no need for copyright assignment for a simple patch
> like that.  However, if you'd like to contribute further to GDB, we can look
> into giving you write access, so you'll be able to push patches by yourself.
> Let me know which one you prefer.

Probably a one-off for now, this was pretty much just scratching an itch.
Anything that requires copyright assignment becomes more complicated for
obvious reasons if sent from my work account.

> The ChangeLog entry for this patch will go in the gdb/ChangeLog file.

Right, this is obvious with 20/20 hindsight. Thanks.

> The ChangeLog format has the function name in parentheses

Thanks, acknowledged for next time.

> The indentation for this line should be one tab + two spaces (like the
> previous line).

Yeah, I noticed that as soon as the email sent -- I had it right at one point,
but I guess I got bitten by `set expandtab` before sending. My bad.

I'm assuming you don't need anything more from me to push?

Cheers,
Leszek

Re: [PATCH] Fix dwarf2_string_attr for -gsplit-dwarf

[Simon Marchi]

On 2017-08-02 12:12, Leszek Swirski via gdb-patches wrote:
>> If you think this will be a one-time contribution, we can merge the 
>> patch
>> for you, and there is no need for copyright assignment for a simple 
>> patch
>> like that.  However, if you'd like to contribute further to GDB, we 
>> can look
>> into giving you write access, so you'll be able to push patches by 
>> yourself.
>> Let me know which one you prefer.
> 
> Probably a one-off for now, this was pretty much just scratching an 
> itch.
> Anything that requires copyright assignment becomes more complicated 
> for
> obvious reasons if sent from my work account.

Ok.

>> The indentation for this line should be one tab + two spaces (like the
>> previous line).
> 
> Yeah, I noticed that as soon as the email sent -- I had it right at one 
> point,
> but I guess I got bitten by `set expandtab` before sending. My bad.
> 
> I'm assuming you don't need anything more from me to push?

No, it should be fine.  I'll just leave this patch up for review for a 
few days in case somebody else wants to comment, and will push after 
that.

Thanks,

Simon

Re: [PATCH] Fix dwarf2_string_attr for -gsplit-dwarf

[Simon Marchi]

On 2017-08-02 12:56, Simon Marchi wrote:
> On 2017-08-02 12:12, Leszek Swirski via gdb-patches wrote:
>>> If you think this will be a one-time contribution, we can merge the 
>>> patch
>>> for you, and there is no need for copyright assignment for a simple 
>>> patch
>>> like that.  However, if you'd like to contribute further to GDB, we 
>>> can look
>>> into giving you write access, so you'll be able to push patches by 
>>> yourself.
>>> Let me know which one you prefer.
>> 
>> Probably a one-off for now, this was pretty much just scratching an 
>> itch.
>> Anything that requires copyright assignment becomes more complicated 
>> for
>> obvious reasons if sent from my work account.
> 
> Ok.
> 
>>> The indentation for this line should be one tab + two spaces (like 
>>> the
>>> previous line).
>> 
>> Yeah, I noticed that as soon as the email sent -- I had it right at 
>> one point,
>> but I guess I got bitten by `set expandtab` before sending. My bad.
>> 
>> I'm assuming you don't need anything more from me to push?
> 
> No, it should be fine.  I'll just leave this patch up for review for a
> few days in case somebody else wants to comment, and will push after
> that.
> 
> Thanks,
> 
> Simon

I have pushed this patch, thanks again.

I ran the tests with the "fission" board file and there were many tests 
that went from FAIL -> PASS.  So I concluded that we don't need to add a 
test specifically for this, but we should probably run the testsuite 
with the fission board on the buildbot.

Simon

Re: [PATCH] Fix dwarf2_string_attr for -gsplit-dwarf

[Pedro Alves]

On 08/02/2017 11:12 AM, Leszek Swirski via gdb-patches wrote:

> Probably a one-off for now, this was pretty much just scratching an itch.
> Anything that requires copyright assignment becomes more complicated for
> obvious reasons if sent from my work account.

Google has a blanket copyright assignment in place, so you're already
covered.

Thanks,
Pedro Alves