[PATCH] Add testcase for DW_AT_count referencing a variable

[PATCH] Add testcase for DW_AT_count referencing a variable

[Gary Benson]

Hi all,

Clang describes the upper bounds of variable length arrays using
a DW_AT_count attribute which references the DIE of a synthetic
variable whose value is specified using a DW_AT_location.  In some
cases GDB correctly handles these, but in other cases GDB adds an
extra dereference causing the test to fail.  This commit adds a
new test to gdb.dwarf2/count.exp with the same DWARF as that
generated by Clang for gdb.base/vla-optimized-out.exp, one of the
failing tests.

Checked on Fedora 32 x86_64, with GCC and Clang.  Ok to commit?

Thanks,
Gary

---
gdb/testsuite/ChangeLog:

	PR gdb/26905
	* gdb.dwarf2/count.exp: Add test for an array whose upper bound
	is defined using a DW_AT_count which references another DIE.
---
 gdb/testsuite/ChangeLog            |  6 +++++
 gdb/testsuite/gdb.dwarf2/count.exp | 49 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.dwarf2/count.exp b/gdb/testsuite/gdb.dwarf2/count.exp
index 5cefb15..c3fc93f 100644
--- a/gdb/testsuite/gdb.dwarf2/count.exp
+++ b/gdb/testsuite/gdb.dwarf2/count.exp
@@ -28,7 +28,10 @@ set asm_file [standard_output_file $srcfile2]
 Dwarf::assemble $asm_file {
     cu {} {
 	compile_unit {{language @DW_LANG_C99}} {
-	    declare_labels char_label array_label array_label2 static_array_label
+	    declare_labels char_label \
+		array_size_type_label long_unsigned_int_label \
+		array_label array_label2 static_array_label \
+		vla_length_label vla_array_label
 
 	    char_label: base_type {
 		{name char}
@@ -36,6 +39,18 @@ Dwarf::assemble $asm_file {
 		{byte_size 1 DW_FORM_sdata}
 	    }
 
+	    array_size_type_label: base_type {
+		{byte_size 8 DW_FORM_sdata}
+		{encoding @DW_ATE_unsigned}
+		{name __ARRAY_SIZE_TYPE__}
+	    }
+
+	    long_unsigned_int_label: base_type {
+		{byte_size 8 DW_FORM_sdata}
+		{encoding @DW_ATE_unsigned}
+		{name "long unsigned int"}
+	    }
+
 	    array_label: array_type {
 		{type :$char_label}
 	    } {
@@ -63,6 +78,23 @@ Dwarf::assemble $asm_file {
 		}
 	    }
 
+	    vla_length_label:
+	    DW_TAG_variable {
+		{location {lit6} SPECIAL_expr}
+		{name "__vla_array_length"}
+		{type :$long_unsigned_int_label}
+		{artificial 1 DW_FORM_flag_present}
+	    }
+
+	    vla_array_label: array_type {
+		{type :$char_label}
+	    } {
+		subrange_type {
+		    {type :$array_size_type_label}
+		    {count :$vla_length_label}
+		}
+	    }
+
 	    DW_TAG_variable {
 		{name array2}
 		{type :$array_label2}
@@ -80,6 +112,12 @@ Dwarf::assemble $asm_file {
 		{type :$static_array_label}
 		{const_value world DW_FORM_block1}
 	    }
+
+	    DW_TAG_variable {
+		{name vla_array}
+		{type :$vla_array_label}
+		{const_value saluton DW_FORM_block1}
+	    }
 	}
     }
 }
@@ -123,3 +161,12 @@ gdb_test "ptype static_array" "type = char \\\[5\\\]"
 gdb_test "whatis static_array" "type = char \\\[5\\\]"
 gdb_test "print static_array" " = \"world\""
 gdb_test "print sizeof static_array" " = 5"
+
+setup_kfail "gdb/26905" *-*-*
+gdb_test "ptype vla_array" "type = char \\\[6\\\]"
+setup_kfail "gdb/26905" *-*-*
+gdb_test "whatis vla_array" "type = char \\\[6\\\]"
+setup_kfail "gdb/26905" *-*-*
+gdb_test "print vla_array" " = \"saluto\""
+setup_kfail "gdb/26905" *-*-*
+gdb_test "print sizeof vla_array" " = 6"
-- 
1.8.3.1

[committed] Add testcase for DW_AT_count referencing a variable

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

On 11/16/20 6:52 PM, Gary Benson via Gdb-patches wrote:
> Hi all,
> 
> Clang describes the upper bounds of variable length arrays using
> a DW_AT_count attribute which references the DIE of a synthetic
> variable whose value is specified using a DW_AT_location.  In some
> cases GDB correctly handles these, but in other cases GDB adds an
> extra dereference causing the test to fail.  This commit adds a
> new test to gdb.dwarf2/count.exp with the same DWARF as that
> generated by Clang for gdb.base/vla-optimized-out.exp, one of the
> failing tests.
> 
> Checked on Fedora 32 x86_64, with GCC and Clang.  Ok to commit?
> 

Hi,

I wrote a test-case myself here (
https://sourceware.org/pipermail/gdb-patches/2020-November/173438.html )
and only after that found your patch.

I'm committing this patch (with you marked as author obviously), because
it's more complete, and the kfails allow us to commit it before having a
fix.

There was one problem: the DWARF expression was missing the
DW_OP_stack_value, I've added that.

Also, I've updated the commit message to better reflect what the actual
problem is.

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-testsuite-Add-testcase-for-DW_AT_count-referencing-a-variable.patch --]
[-- Type: text/x-patch, Size: 3478 bytes --]

[gdb/testsuite] Add testcase for DW_AT_count referencing a variable

Clang describes the upper bounds of variable length arrays using
a DW_AT_count attribute which references the DIE of a synthetic
variable whose value is specified using a DW_AT_location.  GDB handles
these incorrectly if the corresponding DWARF expression finishes with a
DW_OP_stack_value (PR26905).  This commit adds a new kfailed test to
gdb.dwarf2/count.exp with the same DWARF as that generated by Clang for
gdb.base/vla-optimized-out.exp, one of the failing tests.

Checked on Fedora 32 x86_64, with GCC and Clang.

gdb/testsuite/ChangeLog:

2020-11-17  Gary Benson <gbenson@redhat.com>

	PR gdb/26905
	* gdb.dwarf2/count.exp: Add test for an array whose upper bound
	is defined using a DW_AT_count which references another DIE.

---
 gdb/testsuite/gdb.dwarf2/count.exp | 53 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.dwarf2/count.exp b/gdb/testsuite/gdb.dwarf2/count.exp
index 5cefb15da4a..6dcccb57409 100644
--- a/gdb/testsuite/gdb.dwarf2/count.exp
+++ b/gdb/testsuite/gdb.dwarf2/count.exp
@@ -28,7 +28,10 @@ set asm_file [standard_output_file $srcfile2]
 Dwarf::assemble $asm_file {
     cu {} {
 	compile_unit {{language @DW_LANG_C99}} {
-	    declare_labels char_label array_label array_label2 static_array_label
+	    declare_labels char_label \
+		array_size_type_label long_unsigned_int_label \
+		array_label array_label2 static_array_label \
+		vla_length_label vla_array_label
 
 	    char_label: base_type {
 		{name char}
@@ -36,6 +39,18 @@ Dwarf::assemble $asm_file {
 		{byte_size 1 DW_FORM_sdata}
 	    }
 
+	    array_size_type_label: base_type {
+		{byte_size 8 DW_FORM_sdata}
+		{encoding @DW_ATE_unsigned}
+		{name __ARRAY_SIZE_TYPE__}
+	    }
+
+	    long_unsigned_int_label: base_type {
+		{byte_size 8 DW_FORM_sdata}
+		{encoding @DW_ATE_unsigned}
+		{name "long unsigned int"}
+	    }
+
 	    array_label: array_type {
 		{type :$char_label}
 	    } {
@@ -63,6 +78,27 @@ Dwarf::assemble $asm_file {
 		}
 	    }
 
+	    vla_length_label:
+	    DW_TAG_variable {
+		{location
+		    {
+			lit6
+			stack_value
+		    } SPECIAL_expr}
+		{name "__vla_array_length"}
+		{type :$long_unsigned_int_label}
+		{artificial 1 DW_FORM_flag_present}
+	    }
+
+	    vla_array_label: array_type {
+		{type :$char_label}
+	    } {
+		subrange_type {
+		    {type :$array_size_type_label}
+		    {count :$vla_length_label}
+		}
+	    }
+
 	    DW_TAG_variable {
 		{name array2}
 		{type :$array_label2}
@@ -80,6 +116,12 @@ Dwarf::assemble $asm_file {
 		{type :$static_array_label}
 		{const_value world DW_FORM_block1}
 	    }
+
+	    DW_TAG_variable {
+		{name vla_array}
+		{type :$vla_array_label}
+		{const_value saluton DW_FORM_block1}
+	    }
 	}
     }
 }
@@ -123,3 +165,12 @@ gdb_test "ptype static_array" "type = char \\\[5\\\]"
 gdb_test "whatis static_array" "type = char \\\[5\\\]"
 gdb_test "print static_array" " = \"world\""
 gdb_test "print sizeof static_array" " = 5"
+
+setup_kfail "gdb/26905" *-*-*
+gdb_test "ptype vla_array" "type = char \\\[6\\\]"
+setup_kfail "gdb/26905" *-*-*
+gdb_test "whatis vla_array" "type = char \\\[6\\\]"
+setup_kfail "gdb/26905" *-*-*
+gdb_test "print vla_array" " = \"saluto\""
+setup_kfail "gdb/26905" *-*-*
+gdb_test "print sizeof vla_array" " = 6"

Re: [committed] Add testcase for DW_AT_count referencing a variable

[Gary Benson]

Tom de Vries wrote:
> On 11/16/20 6:52 PM, Gary Benson via Gdb-patches wrote:
> > Clang describes the upper bounds of variable length arrays using
> > a DW_AT_count attribute which references the DIE of a synthetic
> > variable whose value is specified using a DW_AT_location.  In some
> > cases GDB correctly handles these, but in other cases GDB adds an
> > extra dereference causing the test to fail.  This commit adds a
> > new test to gdb.dwarf2/count.exp with the same DWARF as that
> > generated by Clang for gdb.base/vla-optimized-out.exp, one of the
> > failing tests.
> > 
> > Checked on Fedora 32 x86_64, with GCC and Clang.  Ok to commit?
> 
> I wrote a test-case myself here (
> https://sourceware.org/pipermail/gdb-patches/2020-November/173438.html )
> and only after that found your patch.

I hate when that happens!

> I'm committing this patch (with you marked as author obviously),
> because it's more complete, and the kfails allow us to commit it
> before having a fix.

Cool.

> There was one problem: the DWARF expression was missing the
> DW_OP_stack_value, I've added that.

Thanks.  FWIW I hadn't seen the "Implicit Location Descriptions" bit
in the spec, I just constructed an expression to end with the value
I wanted, but having read it now it seems it was a mistake that I'd
omitted it.

Reading that bit of the spec, though, it mentions three operations
that specify a value is an implicit location: DW_OP_stack_value,
as you've mentioned, but also DW_OP_implicit_value and
DW_OP_implicit_pointer.  It *doesn't* mention DW_OP_fbreg, which
Clang uses in the DWARF it generates for gdb.base/vla-ptr.exp
(https://sourceware.org/bugzilla/attachment.cgi?id=12970), but
I haven't checked what's actually at that location, the count
itself, or its address.

> Also, I've updated the commit message to better reflect what the
> actual problem is.

Thank you.  If you'd asked me beforehand I'd have said to put both our
names in there, to reflect the work you've put into this.  It's too
late to change this one now I guess, I'm more commenting in case
something similar comes up in future.

Cheers,
Gary

-- 
Gary Benson - he / him / his
Principal Software Engineer, Red Hat

Re: [committed] Add testcase for DW_AT_count referencing a variable

[Tom de Vries]

On 11/25/20 4:10 PM, Gary Benson wrote:
> Tom de Vries wrote:
>> On 11/16/20 6:52 PM, Gary Benson via Gdb-patches wrote:
>>> Clang describes the upper bounds of variable length arrays using
>>> a DW_AT_count attribute which references the DIE of a synthetic
>>> variable whose value is specified using a DW_AT_location.  In some
>>> cases GDB correctly handles these, but in other cases GDB adds an
>>> extra dereference causing the test to fail.  This commit adds a
>>> new test to gdb.dwarf2/count.exp with the same DWARF as that
>>> generated by Clang for gdb.base/vla-optimized-out.exp, one of the
>>> failing tests.
>>>
>>> Checked on Fedora 32 x86_64, with GCC and Clang.  Ok to commit?
>>
>> I wrote a test-case myself here (
>> https://sourceware.org/pipermail/gdb-patches/2020-November/173438.html )
>> and only after that found your patch.
> 
> I hate when that happens!
> 

Oh well.  I guess I prefer to look at it as review preparation ;)

>> I'm committing this patch (with you marked as author obviously),
>> because it's more complete, and the kfails allow us to commit it
>> before having a fix.
> 
> Cool.
> 
>> There was one problem: the DWARF expression was missing the
>> DW_OP_stack_value, I've added that.
> 
> Thanks.  FWIW I hadn't seen the "Implicit Location Descriptions" bit
> in the spec, I just constructed an expression to end with the value
> I wanted, but having read it now it seems it was a mistake that I'd
> omitted it.
> 
> Reading that bit of the spec, though, it mentions three operations
> that specify a value is an implicit location: DW_OP_stack_value,
> as you've mentioned, but also DW_OP_implicit_value and
> DW_OP_implicit_pointer.  It *doesn't* mention DW_OP_fbreg, which
> Clang uses in the DWARF it generates for gdb.base/vla-ptr.exp
> (https://sourceware.org/bugzilla/attachment.cgi?id=12970), but
> I haven't checked what's actually at that location, the count
> itself, or its address.
> 

Yes, it doesn't mention DW_OP_fbreg as implicit location, because it's a
memory location description (see 2.6.1.1.3 Implicit Location Descriptions).

Thanks,
- Tom

>> Also, I've updated the commit message to better reflect what the
>> actual problem is.
> 
> Thank you.  If you'd asked me beforehand I'd have said to put both our
> names in there, to reflect the work you've put into this.  It's too
> late to change this one now I guess, I'm more commenting in case
> something similar comes up in future.
> 
> Cheers,
> Gary
>