Consider test-case gdb.base/arrayidx.exp, augmented with fedora patch https://src.fedoraproject.org/rpms/gdb/blob/master/f/gdb-archer-vla-tests.patch: ... diff --git a/gdb/testsuite/gdb.base/arrayidx.c b/gdb/testsuite/gdb.base/arrayidx.c --- a/gdb/testsuite/gdb.base/arrayidx.c +++ b/gdb/testsuite/gdb.base/arrayidx.c @@ -17,6 +17,13 @@ int array[] = {1, 2, 3, 4}; +#ifdef __GNUC__ +struct + { + int a[0]; + } unbound; +#endif + int main (void) { diff --git a/gdb/testsuite/gdb.base/arrayidx.exp b/gdb/testsuite/gdb.base/arrayidx.exp --- a/gdb/testsuite/gdb.base/arrayidx.exp +++ b/gdb/testsuite/gdb.base/arrayidx.exp @@ -49,4 +49,12 @@ gdb_test "print array" \ "\\{\\\[0\\\] = 1, \\\[1\\\] = 2, \\\[2\\\] = 3, \\\[3\\\] = 4\\}" \ "print array with array-indexes on" - +set test "p unbound.a == &unbound.a\[0\]" +gdb_test_multiple $test $test { + -re " = 1\r\n$gdb_prompt $" { + pass $test + } + -re "No symbol \"unbound\" in current context.\r\n$gdb_prompt $" { + unsupported "$test (no GCC)" + } +} ... We run into this FAIL: ... p unbound.a == &unbound.a[0]^M $7 = 0^M (gdb) FAIL: gdb.base/arrayidx.exp: p unbound.a == &unbound.a[0] ... A bit more probing: ... (gdb) p unbound $3 = {a = 0x601044} (gdb) p &unbound $4 = (struct {...} *) 0x601044 (gdb) p unbound.a $5 = 0x601044 (gdb) p &unbound.a[0] $6 = (int *) 0xffffffffefc93d84^M ... reveals that we have a problem printing the address of the first element of a zero length array.
Tentative patch (not taking side-effects into account yet): ... diff --git a/gdb/eval.c b/gdb/eval.c index 8d0c5747f39..69262340c8f 100644 --- a/gdb/eval.c +++ b/gdb/eval.c @@ -2814,6 +2814,18 @@ evaluate_subexp_for_address (struct expression *exp, int *pos, switch (op) { + case BINOP_SUBSCRIPT: + { + int pos_base = *pos + 1; + struct value *base + = evaluate_subexp (nullptr, exp, &pos_base, noside); + int pos_index = pos_base; + struct value * index + = evaluate_subexp (nullptr, exp, &pos_index, noside); + if (value_equal (index, value_zero (value_type (index), not_lval))) + return base; + } + goto default_case; case UNOP_IND: (*pos)++; x = evaluate_subexp (nullptr, exp, pos, noside); ...
... diff --git a/gdb/eval.c b/gdb/eval.c index 8d0c5747f39..62528075880 100644 --- a/gdb/eval.c +++ b/gdb/eval.c @@ -2814,6 +2814,29 @@ evaluate_subexp_for_address (struct expression *exp, int *pos, switch (op) { + case BINOP_SUBSCRIPT: + if (noside != EVAL_SKIP) + { + int pos_index = *pos + 1; + evaluate_subexp (nullptr, exp, &pos_index, EVAL_SKIP); + + struct value *index + = evaluate_subexp (nullptr, exp, &pos_index, + EVAL_AVOID_SIDE_EFFECTS); + if (value_equal (index, value_zero (value_type (index), not_lval))) + { + (*pos)++; + struct value *base = evaluate_subexp (nullptr, exp, pos, noside); + /* Evaluate side effects for index and increase pos past + index. */ + evaluate_subexp (nullptr, exp, pos, noside); + + return base; + } + } + + goto default_case; + case UNOP_IND: (*pos)++; x = evaluate_subexp (nullptr, exp, pos, noside); ...
This passes testing: ... diff --git a/gdb/eval.c b/gdb/eval.c index 8d0c5747f39..0e8698bd488 100644 --- a/gdb/eval.c +++ b/gdb/eval.c @@ -2814,6 +2814,39 @@ evaluate_subexp_for_address (struct expression *exp, int *pos, switch (op) { + case BINOP_SUBSCRIPT: + if (noside != EVAL_SKIP) + { + /* Skip over the base to get the position of the index. */ + int pos_index = *pos + 1; + evaluate_subexp (nullptr, exp, &pos_index, EVAL_SKIP); + + /* Check if the index is zero. Do this without evaluating + side effects, such that we can still fall back to default_case. */ + struct value *index + = evaluate_subexp (nullptr, exp, &pos_index, + EVAL_AVOID_SIDE_EFFECTS); + if (value_equal (index, value_zero (value_type (index), not_lval))) + { + /* We're handling op here, skip it. */ + (*pos)++; + + /* Evaluate base. */ + struct value *base = evaluate_subexp (nullptr, exp, pos, noside); + + /* Evaluate index, for the side-effect, and to return with pos + past index. */ + evaluate_subexp (nullptr, exp, pos, noside); + + /* Translate &base[0] into (typeof(base[0]) *)base. */ + type *element_type = TYPE_TARGET_TYPE (value_type (base)); + type *return_type = lookup_pointer_type (element_type); + return value_cast (return_type, base); + } + } + + goto default_case; + case UNOP_IND: (*pos)++; x = evaluate_subexp (nullptr, exp, pos, noside); ...
This passed with gdb 9. The first bad commit is either: - commit 7c6f271296319576fa00587928e5ff52ced9c1bb (could not build) gdb: make get_discrete_bounds check for non-constant range bounds, or - commit 8c2e4e0689ea244d0ed979171a3d09c9176b8175 gdb: add accessors to struct dynamic_prop
(In reply to Tom de Vries from comment #4) > This passed with gdb 9. > > The first bad commit is either: > - commit 7c6f271296319576fa00587928e5ff52ced9c1bb (could not build) > gdb: make get_discrete_bounds check for non-constant range bounds, or > - commit 8c2e4e0689ea244d0ed979171a3d09c9176b8175 > gdb: add accessors to struct dynamic_prop Hmm, before these commits, we handle this type in get_discrete_bounds: ... (gdb) p recursive_dump_type (type, 0) type node 0x2283020 name '<NULL>' (0x0) code 0xc (TYPE_CODE_RANGE) length 8 objfile 0x1fced60 target_type 0x223c440 type node 0x223c440 name 'long unsigned int' (0x225be83) code 0x8 (TYPE_CODE_INT) length 8 objfile 0x1fced60 target_type 0x0 pointer_type 0x0 reference_type 0x0 type_chain 0x223c440 instance_flags 0x0 flags TYPE_UNSIGNED nfields 0 0x0 pointer_type 0x0 reference_type 0x0 type_chain 0x2283020 instance_flags 0x0 flags TYPE_UNSIGNED nfields 0 0x22830a0 low 0 high 0 (undefined) $12 = void ... and ignored the TYPE_HIGH_BOUND_UNDEFINED, and just return 1, with lowerbound == 0 and upperbound == 0. The setting of lowerbound == 0 had the effect that we printed the right value. After the commits, get_discrete_bounds returns -1, and we have both lowerbound and upperbound uninitialized. We don't check the return status though in value_subscript, and proceed with the uninitialized values: ... (gdb) p lowerbound $2 = 36429408 (gdb) p upperbound $3 = 11281392 ... And after this: ... (gdb) 171 index -= lowerbound; ... we have: ... (gdb) p index $4 = -36429408 ... So we end up here: ... return value_ind (value_ptradd (array, index)); ... constructing a value "*(array + -36429408)", and we end up printing &ubound.a[-36429408]. This is an ad-hoc patch that sets lowerbound, even if upperbound is undefined: ... diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c index 686edafcf64..dfc3de870c1 100644 --- a/gdb/gdbtypes.c +++ b/gdb/gdbtypes.c @@ -1049,11 +1049,16 @@ get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp) case TYPE_CODE_RANGE: /* This function currently only works for ranges with two defined, constant bounds. */ - if (type->bounds ()->low.kind () != PROP_CONST - || type->bounds ()->high.kind () != PROP_CONST) + if (type->bounds ()->low.kind () != PROP_CONST) return -1; - *lowp = type->bounds ()->low.const_val (); + + if (type->bounds ()->high.kind () != PROP_CONST) + { + *highp = *lowp - 1; + return 1; + } + *highp = type->bounds ()->high.const_val (); if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM) ... and handles the undefined upper bound by returning an empty range.
The simplest fix would be to make sure get_discrete_bounds always returns defined *lowp and *highp: ... diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c index 686edafcf64..9a891e7bfde 100644 --- a/gdb/gdbtypes.c +++ b/gdb/gdbtypes.c @@ -1043,6 +1043,8 @@ has_static_range (const struct range_bounds *bounds) int get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp) { + *lowp = 0; + *highp = -1; type = check_typedef (type); switch (type->code ()) { ... which also happens to fix PR26870 - "[fortran] Printing dynamic array fails".
I am not able to reproduce. In my version of the compiled test case, both bounds are const: (top-gdb) p type->bounds ()->low.kind () $4 = PROP_CONST (top-gdb) p type->bounds ()->high.kind () $5 = PROP_CONST What compiler do you use to build the test case? In my version, this is the DIE that corresponds to the type of the zero-length field: 0x00000076: DW_TAG_array_type DW_AT_type [DW_FORM_ref4] (0x00000044 "int") DW_AT_sibling [DW_FORM_ref4] (0x00000086) 0x0000007f: DW_TAG_subrange_type DW_AT_type [DW_FORM_ref4] (0x0000003d "long unsigned int") DW_AT_count [DW_FORM_data1] (0x00) So I presume GDB translates that to two constant bounds with value 0. What does it look like in yours? I stumbled on a similar problem and filed this bug here, they are likely related: https://sourceware.org/bugzilla/show_bug.cgi?id=26901 I have this patch series in the pipeline that I made to address 26901: https://review.lttng.org/c/binutils-gdb/+/4399/4 If/when I am able to reproduce this (26875) bug, I can check if that helps with it too.
Here's the URL that points to the always-tested version of the patch: https://review.lttng.org/c/binutils-gdb/+/4399
(In reply to Simon Marchi from comment #8) > Here's the URL that points to the always-tested version of the patch: > > https://review.lttng.org/c/binutils-gdb/+/4399 Err it's currently broken due to a rebase, I'll fix it soon.
Ok, it now builds properly.
(In reply to Simon Marchi from comment #7) > I am not able to reproduce. In my version of the compiled test case, both > bounds are const: > > (top-gdb) p type->bounds ()->low.kind () > $4 = PROP_CONST > (top-gdb) p type->bounds ()->high.kind () > $5 = PROP_CONST > > What compiler do you use to build the test case? Gcc 7.5.0 > In my version, this is the > DIE that corresponds to the type of the zero-length field: > > 0x00000076: DW_TAG_array_type > DW_AT_type [DW_FORM_ref4] (0x00000044 "int") > DW_AT_sibling [DW_FORM_ref4] (0x00000086) > > 0x0000007f: DW_TAG_subrange_type > DW_AT_type [DW_FORM_ref4] (0x0000003d "long unsigned > int") > DW_AT_count [DW_FORM_data1] (0x00) > I get: ... <1><13a>: Abbrev Number: 2 (DW_TAG_array_type) <13b> DW_AT_type : <0x10b> <13f> DW_AT_sibling : <0x149> <2><143>: Abbrev Number: 9 (DW_TAG_subrange_type) <144> DW_AT_type : <0x104> ... without DW_AT_count. The DW_AT_count is added at gcc-9, and using that the test-cases passes for me. > So I presume GDB translates that to two constant bounds with value 0. What > does it look like in yours? > > I stumbled on a similar problem and filed this bug here, they are likely > related: > > https://sourceware.org/bugzilla/show_bug.cgi?id=26901 > > I have this patch series in the pipeline that I made to address 26901: > > https://review.lttng.org/c/binutils-gdb/+/4399/4 > Ah, thanks for the pointer, I'll take a look. > If/when I am able to reproduce this (26875) bug, I can check if that helps > with it too. That would be great.
(In reply to Tom de Vries from comment #11) > (In reply to Simon Marchi from comment #7) > > I am not able to reproduce. In my version of the compiled test case, both > > bounds are const: > > > > (top-gdb) p type->bounds ()->low.kind () > > $4 = PROP_CONST > > (top-gdb) p type->bounds ()->high.kind () > > $5 = PROP_CONST > > > > What compiler do you use to build the test case? > > Gcc 7.5.0 Thanks, I see the failure with gcc 7. > > So I presume GDB translates that to two constant bounds with value 0. What > > does it look like in yours? > > > > I stumbled on a similar problem and filed this bug here, they are likely > > related: > > > > https://sourceware.org/bugzilla/show_bug.cgi?id=26901 > > > > I have this patch series in the pipeline that I made to address 26901: > > > > https://review.lttng.org/c/binutils-gdb/+/4399/4 > > > > Ah, thanks for the pointer, I'll take a look. The patches are not commented at all now, but the idea is to be able to fetch the low and high bounds separately, not all or nothing like it is now. Here, we only care about the low bound, so we only get that one, and ignore the high bound (which is unknown). > > > If/when I am able to reproduce this (26875) bug, I can check if that helps > > with it too. > > That would be great. The failure goes away with these patches applied. The only thing that bugs me is that some part of the code assumes that a bound of "-1" means an unknown bound. It seems to me that if we use gdb::optional, we don't need that: an empty optional means an unknown bound. So we would need to go through all the users to see we if need the "set to -1" vs "empty optional" disctinction or not.
Proposed patch series: https://sourceware.org/pipermail/gdb-patches/2020-November/173496.html
The master branch has been updated by Simon Marchi <simark@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=5b56203a7cadd545b33713e98e274e582242e090 commit 5b56203a7cadd545b33713e98e274e582242e090 Author: Simon Marchi <simon.marchi@efficios.com> Date: Wed Dec 9 13:52:12 2020 -0500 gdb: fix value_subscript when array upper bound is not known Since commit 7c6f27129631 ("gdb: make get_discrete_bounds check for non-constant range bounds"), subscripting flexible array member fails: struct no_size { int n; int items[]; }; (gdb) p *ns $1 = {n = 3, items = 0x5555555592a4} (gdb) p ns->items[0] Cannot access memory at address 0xfffe555b733a0164 (gdb) p *((int *) 0x5555555592a4) $2 = 101 <--- we would expect that (gdb) p &ns->items[0] $3 = (int *) 0xfffe5559ee829a24 <--- wrong address Since the flexible array member (items) has an unspecified size, the array type created for it in the DWARF doesn't have dimensions (this is with gcc 9.3.0, Ubuntu 20.04): 0x000000a4: DW_TAG_array_type DW_AT_type [DW_FORM_ref4] (0x00000038 "int") DW_AT_sibling [DW_FORM_ref4] (0x000000b3) 0x000000ad: DW_TAG_subrange_type DW_AT_type [DW_FORM_ref4] (0x00000031 "long unsigned int") This causes GDB to create a range type (TYPE_CODE_RANGE) with a defined constant low bound (dynamic_prop with kind PROP_CONST) and an undefined high bound (dynamic_prop with kind PROP_UNDEFINED). value_subscript gets both bounds of that range using get_discrete_bounds. Before commit 7c6f27129631, get_discrete_bounds didn't check the kind of the dynamic_props and would just blindly read them as if they were PROP_CONST. It would return 0 for the high bound, because we zero-initialize the range_bounds structure. And it didn't really matter in this case, because the returned high bound wasn't used in the end. Commit 7c6f27129631 changed get_discrete_bounds to return a failure if either the low or high bound is not a constant, to make sure we don't read a dynamic prop that isn't a PROP_CONST as a PROP_CONST. This change made get_discrete_bounds start to return a failure for that range, and as a result would not set *lowp and *highp. And since value_subscript doesn't check get_discrete_bounds' return value, it just carries on an uses an uninitialized value for the low bound. If value_subscript did check the return value of get_discrete_bounds, we would get an error message instead of a bogus value. But it would still be a bug, as we wouldn't be able to print the flexible array member's elements. Looking at value_subscript, we see that the low bound is always needed, but the high bound is only needed if !c_style. So, change value_subscript to use get_discrete_low_bound and get_discrete_high_bound separately. This fixes the case described above, where the low bound is known but the high bound isn't (and is not needed). This restores the original behavior without accessing a dynamic_prop in a wrong way. A test is added. In addition to the case described above, a case with an array member of size 0 is added, which is a GNU C extension that existed before flexible array members were introduced. That case currently fails when compiled with gcc <= 8. gcc <= 8 produces DWARF similar to the one shown above, while gcc 9 adds a DW_AT_count of 0 in there, which makes the high bound known. A case where an array member of size 0 is the only member of the struct is also added, as that was how PR 28675 was originally reported, and it's an interesting corner case that I think could trigger other funny bugs. Question about the implementation: in value_subscript, I made it such that if the low or high bound is unknown, we fall back to zero. That effectively makes it the same as it was before 7c6f27129631. But should we instead error() out? gdb/ChangeLog: PR 26875, PR 26901 * gdbtypes.c (get_discrete_low_bound): Make non-static. (get_discrete_high_bound): Make non-static. * gdbtypes.h (get_discrete_low_bound): New declaration. (get_discrete_high_bound): New declaration. * valarith.c (value_subscript): Only fetch high bound if necessary. gdb/testsuite/ChangeLog: PR 26875, PR 26901 * gdb.base/flexible-array-member.c: New test. * gdb.base/flexible-array-member.exp: New test. Change-Id: I832056f80e6c56f621f398b4780d55a3a1e299d7
The gdb-10-branch branch has been updated by Simon Marchi <simark@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=138084b24811c191a85c919116d839a8bd89d57c commit 138084b24811c191a85c919116d839a8bd89d57c Author: Simon Marchi <simon.marchi@efficios.com> Date: Wed Dec 9 13:52:12 2020 -0500 gdb: fix value_subscript when array upper bound is not known Since commit 7c6f27129631 ("gdb: make get_discrete_bounds check for non-constant range bounds"), subscripting flexible array member fails: struct no_size { int n; int items[]; }; (gdb) p *ns $1 = {n = 3, items = 0x5555555592a4} (gdb) p ns->items[0] Cannot access memory at address 0xfffe555b733a0164 (gdb) p *((int *) 0x5555555592a4) $2 = 101 <--- we would expect that (gdb) p &ns->items[0] $3 = (int *) 0xfffe5559ee829a24 <--- wrong address Since the flexible array member (items) has an unspecified size, the array type created for it in the DWARF doesn't have dimensions (this is with gcc 9.3.0, Ubuntu 20.04): 0x000000a4: DW_TAG_array_type DW_AT_type [DW_FORM_ref4] (0x00000038 "int") DW_AT_sibling [DW_FORM_ref4] (0x000000b3) 0x000000ad: DW_TAG_subrange_type DW_AT_type [DW_FORM_ref4] (0x00000031 "long unsigned int") This causes GDB to create a range type (TYPE_CODE_RANGE) with a defined constant low bound (dynamic_prop with kind PROP_CONST) and an undefined high bound (dynamic_prop with kind PROP_UNDEFINED). value_subscript gets both bounds of that range using get_discrete_bounds. Before commit 7c6f27129631, get_discrete_bounds didn't check the kind of the dynamic_props and would just blindly read them as if they were PROP_CONST. It would return 0 for the high bound, because we zero-initialize the range_bounds structure. And it didn't really matter in this case, because the returned high bound wasn't used in the end. Commit 7c6f27129631 changed get_discrete_bounds to return a failure if either the low or high bound is not a constant, to make sure we don't read a dynamic prop that isn't a PROP_CONST as a PROP_CONST. This change made get_discrete_bounds start to return a failure for that range, and as a result would not set *lowp and *highp. And since value_subscript doesn't check get_discrete_bounds' return value, it just carries on an uses an uninitialized value for the low bound. If value_subscript did check the return value of get_discrete_bounds, we would get an error message instead of a bogus value. But it would still be a bug, as we wouldn't be able to print the flexible array member's elements. Looking at value_subscript, we see that the low bound is always needed, but the high bound is only needed if !c_style. So, change value_subscript to use get_discrete_low_bound and get_discrete_high_bound separately. This fixes the case described above, where the low bound is known but the high bound isn't (and is not needed). This restores the original behavior without accessing a dynamic_prop in a wrong way. A test is added. In addition to the case described above, a case with an array member of size 0 is added, which is a GNU C extension that existed before flexible array members were introduced. That case currently fails when compiled with gcc <= 8. gcc <= 8 produces DWARF similar to the one shown above, while gcc 9 adds a DW_AT_count of 0 in there, which makes the high bound known. A case where an array member of size 0 is the only member of the struct is also added, as that was how PR 28675 was originally reported, and it's an interesting corner case that I think could trigger other funny bugs. Question about the implementation: in value_subscript, I made it such that if the low or high bound is unknown, we fall back to zero. That effectively makes it the same as it was before 7c6f27129631. But should we instead error() out? gdb/ChangeLog: PR 26875, PR 26901 * gdbtypes.c (get_discrete_low_bound): Make non-static. (get_discrete_high_bound): Make non-static. * gdbtypes.h (get_discrete_low_bound): New declaration. (get_discrete_high_bound): New declaration. * valarith.c (value_subscript): Only fetch high bound if necessary. gdb/testsuite/ChangeLog: PR 26875, PR 26901 * gdb.base/flexible-array-member.c: New test. * gdb.base/flexible-array-member.exp: New test. Change-Id: I832056f80e6c56f621f398b4780d55a3a1e299d7
This is now fixed.
$ git tag --contains 138084b24811c191a85c919116d839a8bd89d57c gdb-10.2-release