* [PATCH 1/4] gdb: make discrete_position return optional
2020-11-23 16:21 [PATCH 0/4] Fix bug in value_subscript when range's high bound is not known Simon Marchi
@ 2020-11-23 16:21 ` Simon Marchi
2020-12-06 5:25 ` Joel Brobecker
2020-12-06 5:38 ` Joel Brobecker
2020-11-23 16:21 ` [PATCH 2/4] gdb: make get_discrete_bounds return bool Simon Marchi
` (2 subsequent siblings)
3 siblings, 2 replies; 21+ messages in thread
From: Simon Marchi @ 2020-11-23 16:21 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
Instead of returning a boolean status and returning the value through a
pointer, return an optional that does both jobs. This helps in the
following patches, and I think it is an improvement in general.
gdb/ChangeLog:
* ada-lang.c (ada_value_slice_from_ptr): Adjust.
(ada_value_slice): Adjust.
(pos_atr): Adjust.
* gdbtypes.c (get_discrete_bounds): Adjust.
(discrete_position): Return optional.
* gdbtypes.h (discrete_position): Return optional.
Change-Id: I758dbd8858b296ee472ed39ec35db1dbd624a5ae
---
gdb/ada-lang.c | 27 ++++++++++++++++-----------
gdb/gdbtypes.c | 33 ++++++++++++++++++++-------------
gdb/gdbtypes.h | 4 +++-
3 files changed, 39 insertions(+), 25 deletions(-)
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 714227d24dd..9ff470d97a7 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2817,11 +2817,13 @@ ada_value_slice_from_ptr (struct value *array_ptr, struct type *type,
type0->dyn_prop (DYN_PROP_BYTE_STRIDE),
TYPE_FIELD_BITSIZE (type0, 0));
int base_low = ada_discrete_type_low_bound (type0->index_type ());
- LONGEST base_low_pos, low_pos;
+ gdb::optional<LONGEST> base_low_pos, low_pos;
CORE_ADDR base;
- if (!discrete_position (base_index_type, low, &low_pos)
- || !discrete_position (base_index_type, base_low, &base_low_pos))
+ low_pos = discrete_position (base_index_type, low);
+ base_low_pos = discrete_position (base_index_type, base_low);
+
+ if (!low_pos.has_value () || !base_low_pos.has_value ())
{
warning (_("unable to get positions in slice, use bounds instead"));
low_pos = low;
@@ -2832,7 +2834,7 @@ ada_value_slice_from_ptr (struct value *array_ptr, struct type *type,
if (stride == 0)
stride = TYPE_LENGTH (TYPE_TARGET_TYPE (type0));
- base = value_as_address (array_ptr) + (low_pos - base_low_pos) * stride;
+ base = value_as_address (array_ptr) + (*low_pos - *base_low_pos) * stride;
return value_at_lazy (slice_type, base);
}
@@ -2848,10 +2850,13 @@ ada_value_slice (struct value *array, int low, int high)
(NULL, TYPE_TARGET_TYPE (type), index_type,
type->dyn_prop (DYN_PROP_BYTE_STRIDE),
TYPE_FIELD_BITSIZE (type, 0));
- LONGEST low_pos, high_pos;
+ gdb::optional<LONGEST> low_pos, high_pos;
+
- if (!discrete_position (base_index_type, low, &low_pos)
- || !discrete_position (base_index_type, high, &high_pos))
+ low_pos = discrete_position (base_index_type, low);
+ high_pos = discrete_position (base_index_type, high);
+
+ if (!low_pos.has_value () || !high_pos.has_value ())
{
warning (_("unable to get positions in slice, use bounds instead"));
low_pos = low;
@@ -2859,7 +2864,7 @@ ada_value_slice (struct value *array, int low, int high)
}
return value_cast (slice_type,
- value_slice (array, low, high_pos - low_pos + 1));
+ value_slice (array, low, *high_pos - *low_pos + 1));
}
/* If type is a record type in the form of a standard GNAT array
@@ -8929,15 +8934,15 @@ pos_atr (struct value *arg)
{
struct value *val = coerce_ref (arg);
struct type *type = value_type (val);
- LONGEST result;
if (!discrete_type_p (type))
error (_("'POS only defined on discrete types"));
- if (!discrete_position (type, value_as_long (val), &result))
+ gdb::optional<LONGEST> result = discrete_position (type, value_as_long (val));
+ if (!result.has_value ())
error (_("enumeration value is invalid: can't find 'POS"));
- return result;
+ return *result;
}
static struct value *
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index e6f70bbe2d3..09f33c21e28 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1062,9 +1062,21 @@ get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
{
- if (!discrete_position (TYPE_TARGET_TYPE (type), *lowp, lowp)
- || ! discrete_position (TYPE_TARGET_TYPE (type), *highp, highp))
+ gdb::optional<LONGEST> low_pos
+ = discrete_position (TYPE_TARGET_TYPE (type), *lowp);
+
+ if (!low_pos.has_value ())
+ return 0;
+
+ *lowp = *low_pos;
+
+ gdb::optional<LONGEST> high_pos
+ = discrete_position (TYPE_TARGET_TYPE (type), *highp);
+
+ if (!high_pos.has_value ())
return 0;
+
+ *highp = *high_pos;
}
return 1;
case TYPE_CODE_ENUM:
@@ -1160,8 +1172,8 @@ get_array_bounds (struct type *type, LONGEST *low_bound, LONGEST *high_bound)
in which case the value of POS is unmodified.
*/
-int
-discrete_position (struct type *type, LONGEST val, LONGEST *pos)
+gdb::optional<LONGEST>
+discrete_position (struct type *type, LONGEST val)
{
if (type->code () == TYPE_CODE_RANGE)
type = TYPE_TARGET_TYPE (type);
@@ -1173,19 +1185,14 @@ discrete_position (struct type *type, LONGEST val, LONGEST *pos)
for (i = 0; i < type->num_fields (); i += 1)
{
if (val == TYPE_FIELD_ENUMVAL (type, i))
- {
- *pos = i;
- return 1;
- }
+ return i;
}
+
/* Invalid enumeration value. */
- return 0;
+ return {};
}
else
- {
- *pos = val;
- return 1;
- }
+ return val;
}
/* If the array TYPE has static bounds calculate and update its
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 2b6f599f4c7..59ff6fc6ce3 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -46,6 +46,7 @@
#include "hashtab.h"
#include "gdbsupport/array-view.h"
+#include "gdbsupport/gdb_optional.h"
#include "gdbsupport/offset-type.h"
#include "gdbsupport/enum-flags.h"
#include "gdbsupport/underlying.h"
@@ -2445,7 +2446,8 @@ extern int get_discrete_bounds (struct type *, LONGEST *, LONGEST *);
extern bool get_array_bounds (struct type *type, LONGEST *low_bound,
LONGEST *high_bound);
-extern int discrete_position (struct type *type, LONGEST val, LONGEST *pos);
+extern gdb::optional<LONGEST> discrete_position (struct type *type,
+ LONGEST val);
extern int class_types_same_p (const struct type *, const struct type *);
--
2.29.2
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 1/4] gdb: make discrete_position return optional
2020-11-23 16:21 ` [PATCH 1/4] gdb: make discrete_position return optional Simon Marchi
@ 2020-12-06 5:25 ` Joel Brobecker
2020-12-06 5:38 ` Joel Brobecker
1 sibling, 0 replies; 21+ messages in thread
From: Joel Brobecker @ 2020-12-06 5:25 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi
Hi Simon,
On Mon, Nov 23, 2020 at 11:21:17AM -0500, Simon Marchi via Gdb-patches wrote:
> Instead of returning a boolean status and returning the value through a
> pointer, return an optional that does both jobs. This helps in the
> following patches, and I think it is an improvement in general.
>
> gdb/ChangeLog:
>
> * ada-lang.c (ada_value_slice_from_ptr): Adjust.
> (ada_value_slice): Adjust.
> (pos_atr): Adjust.
> * gdbtypes.c (get_discrete_bounds): Adjust.
> (discrete_position): Return optional.
> * gdbtypes.h (discrete_position): Return optional.
Thanks for this patch; an interesting cleanup!
This patch LGTM.
> Change-Id: I758dbd8858b296ee472ed39ec35db1dbd624a5ae
> ---
> gdb/ada-lang.c | 27 ++++++++++++++++-----------
> gdb/gdbtypes.c | 33 ++++++++++++++++++++-------------
> gdb/gdbtypes.h | 4 +++-
> 3 files changed, 39 insertions(+), 25 deletions(-)
>
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 714227d24dd..9ff470d97a7 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -2817,11 +2817,13 @@ ada_value_slice_from_ptr (struct value *array_ptr, struct type *type,
> type0->dyn_prop (DYN_PROP_BYTE_STRIDE),
> TYPE_FIELD_BITSIZE (type0, 0));
> int base_low = ada_discrete_type_low_bound (type0->index_type ());
> - LONGEST base_low_pos, low_pos;
> + gdb::optional<LONGEST> base_low_pos, low_pos;
> CORE_ADDR base;
>
> - if (!discrete_position (base_index_type, low, &low_pos)
> - || !discrete_position (base_index_type, base_low, &base_low_pos))
> + low_pos = discrete_position (base_index_type, low);
> + base_low_pos = discrete_position (base_index_type, base_low);
> +
> + if (!low_pos.has_value () || !base_low_pos.has_value ())
> {
> warning (_("unable to get positions in slice, use bounds instead"));
> low_pos = low;
> @@ -2832,7 +2834,7 @@ ada_value_slice_from_ptr (struct value *array_ptr, struct type *type,
> if (stride == 0)
> stride = TYPE_LENGTH (TYPE_TARGET_TYPE (type0));
>
> - base = value_as_address (array_ptr) + (low_pos - base_low_pos) * stride;
> + base = value_as_address (array_ptr) + (*low_pos - *base_low_pos) * stride;
> return value_at_lazy (slice_type, base);
> }
>
> @@ -2848,10 +2850,13 @@ ada_value_slice (struct value *array, int low, int high)
> (NULL, TYPE_TARGET_TYPE (type), index_type,
> type->dyn_prop (DYN_PROP_BYTE_STRIDE),
> TYPE_FIELD_BITSIZE (type, 0));
> - LONGEST low_pos, high_pos;
> + gdb::optional<LONGEST> low_pos, high_pos;
> +
>
> - if (!discrete_position (base_index_type, low, &low_pos)
> - || !discrete_position (base_index_type, high, &high_pos))
> + low_pos = discrete_position (base_index_type, low);
> + high_pos = discrete_position (base_index_type, high);
> +
> + if (!low_pos.has_value () || !high_pos.has_value ())
> {
> warning (_("unable to get positions in slice, use bounds instead"));
> low_pos = low;
> @@ -2859,7 +2864,7 @@ ada_value_slice (struct value *array, int low, int high)
> }
>
> return value_cast (slice_type,
> - value_slice (array, low, high_pos - low_pos + 1));
> + value_slice (array, low, *high_pos - *low_pos + 1));
> }
>
> /* If type is a record type in the form of a standard GNAT array
> @@ -8929,15 +8934,15 @@ pos_atr (struct value *arg)
> {
> struct value *val = coerce_ref (arg);
> struct type *type = value_type (val);
> - LONGEST result;
>
> if (!discrete_type_p (type))
> error (_("'POS only defined on discrete types"));
>
> - if (!discrete_position (type, value_as_long (val), &result))
> + gdb::optional<LONGEST> result = discrete_position (type, value_as_long (val));
> + if (!result.has_value ())
> error (_("enumeration value is invalid: can't find 'POS"));
>
> - return result;
> + return *result;
> }
>
> static struct value *
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index e6f70bbe2d3..09f33c21e28 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -1062,9 +1062,21 @@ get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
>
> if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
> {
> - if (!discrete_position (TYPE_TARGET_TYPE (type), *lowp, lowp)
> - || ! discrete_position (TYPE_TARGET_TYPE (type), *highp, highp))
> + gdb::optional<LONGEST> low_pos
> + = discrete_position (TYPE_TARGET_TYPE (type), *lowp);
> +
> + if (!low_pos.has_value ())
> + return 0;
> +
> + *lowp = *low_pos;
> +
> + gdb::optional<LONGEST> high_pos
> + = discrete_position (TYPE_TARGET_TYPE (type), *highp);
> +
> + if (!high_pos.has_value ())
> return 0;
> +
> + *highp = *high_pos;
> }
> return 1;
> case TYPE_CODE_ENUM:
> @@ -1160,8 +1172,8 @@ get_array_bounds (struct type *type, LONGEST *low_bound, LONGEST *high_bound)
> in which case the value of POS is unmodified.
> */
>
> -int
> -discrete_position (struct type *type, LONGEST val, LONGEST *pos)
> +gdb::optional<LONGEST>
> +discrete_position (struct type *type, LONGEST val)
> {
> if (type->code () == TYPE_CODE_RANGE)
> type = TYPE_TARGET_TYPE (type);
> @@ -1173,19 +1185,14 @@ discrete_position (struct type *type, LONGEST val, LONGEST *pos)
> for (i = 0; i < type->num_fields (); i += 1)
> {
> if (val == TYPE_FIELD_ENUMVAL (type, i))
> - {
> - *pos = i;
> - return 1;
> - }
> + return i;
> }
> +
> /* Invalid enumeration value. */
> - return 0;
> + return {};
> }
> else
> - {
> - *pos = val;
> - return 1;
> - }
> + return val;
> }
>
> /* If the array TYPE has static bounds calculate and update its
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index 2b6f599f4c7..59ff6fc6ce3 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -46,6 +46,7 @@
>
> #include "hashtab.h"
> #include "gdbsupport/array-view.h"
> +#include "gdbsupport/gdb_optional.h"
> #include "gdbsupport/offset-type.h"
> #include "gdbsupport/enum-flags.h"
> #include "gdbsupport/underlying.h"
> @@ -2445,7 +2446,8 @@ extern int get_discrete_bounds (struct type *, LONGEST *, LONGEST *);
> extern bool get_array_bounds (struct type *type, LONGEST *low_bound,
> LONGEST *high_bound);
>
> -extern int discrete_position (struct type *type, LONGEST val, LONGEST *pos);
> +extern gdb::optional<LONGEST> discrete_position (struct type *type,
> + LONGEST val);
>
> extern int class_types_same_p (const struct type *, const struct type *);
>
> --
> 2.29.2
--
Joel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 1/4] gdb: make discrete_position return optional
2020-11-23 16:21 ` [PATCH 1/4] gdb: make discrete_position return optional Simon Marchi
2020-12-06 5:25 ` Joel Brobecker
@ 2020-12-06 5:38 ` Joel Brobecker
2020-12-07 14:58 ` Simon Marchi
1 sibling, 1 reply; 21+ messages in thread
From: Joel Brobecker @ 2020-12-06 5:38 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi
Me again, Simon,
I just noticed...
> gdb/ChangeLog:
>
> * ada-lang.c (ada_value_slice_from_ptr): Adjust.
> (ada_value_slice): Adjust.
> (pos_atr): Adjust.
> * gdbtypes.c (get_discrete_bounds): Adjust.
> (discrete_position): Return optional.
> * gdbtypes.h (discrete_position): Return optional.
>
> Change-Id: I758dbd8858b296ee472ed39ec35db1dbd624a5ae
... that you revision logs have the Change-Id: field, which I assume
was for when we were trying Gerrit out. Since it's no longer the case,
perhaps you might want to disconnect the hook that adds them
automatically?
--
Joel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] gdb: make discrete_position return optional
2020-12-06 5:38 ` Joel Brobecker
@ 2020-12-07 14:58 ` Simon Marchi
2020-12-08 3:06 ` Joel Brobecker
0 siblings, 1 reply; 21+ messages in thread
From: Simon Marchi @ 2020-12-07 14:58 UTC (permalink / raw)
To: Joel Brobecker, Simon Marchi via Gdb-patches
On 2020-12-06 12:38 a.m., Joel Brobecker wrote:
> ... that you revision logs have the Change-Id: field, which I assume
> was for when we were trying Gerrit out. Since it's no longer the case,
> perhaps you might want to disconnect the hook that adds them
> automatically?
I use Gerrit myself for tracking the patches I send to GDB. That's the
only reliable way I found to keep track of patches I've sent that I
haven't merged yet, so they don't just get lost and forgotten on the
list. And I presume it doesn't really cause an inconvenience to anybody.
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] gdb: make discrete_position return optional
2020-12-07 14:58 ` Simon Marchi
@ 2020-12-08 3:06 ` Joel Brobecker
2020-12-08 11:41 ` Maciej W. Rozycki
0 siblings, 1 reply; 21+ messages in thread
From: Joel Brobecker @ 2020-12-08 3:06 UTC (permalink / raw)
To: Simon Marchi; +Cc: Simon Marchi via Gdb-patches
> On 2020-12-06 12:38 a.m., Joel Brobecker wrote:
> > ... that you revision logs have the Change-Id: field, which I assume
> > was for when we were trying Gerrit out. Since it's no longer the case,
> > perhaps you might want to disconnect the hook that adds them
> > automatically?
>
> I use Gerrit myself for tracking the patches I send to GDB. That's the only
> reliable way I found to keep track of patches I've sent that I haven't
> merged yet, so they don't just get lost and forgotten on the list. And I
> presume it doesn't really cause an inconvenience to anybody.
I can only speak for myself, but personally I don't mind. The only
reason I highlighted them is because I thought they were an oversight.
--
Joel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] gdb: make discrete_position return optional
2020-12-08 3:06 ` Joel Brobecker
@ 2020-12-08 11:41 ` Maciej W. Rozycki
2020-12-09 19:29 ` Simon Marchi
0 siblings, 1 reply; 21+ messages in thread
From: Maciej W. Rozycki @ 2020-12-08 11:41 UTC (permalink / raw)
To: Joel Brobecker; +Cc: Simon Marchi, Simon Marchi via Gdb-patches
On Tue, 8 Dec 2020, Joel Brobecker wrote:
> > > ... that you revision logs have the Change-Id: field, which I assume
> > > was for when we were trying Gerrit out. Since it's no longer the case,
> > > perhaps you might want to disconnect the hook that adds them
> > > automatically?
> >
> > I use Gerrit myself for tracking the patches I send to GDB. That's the only
> > reliable way I found to keep track of patches I've sent that I haven't
> > merged yet, so they don't just get lost and forgotten on the list. And I
> > presume it doesn't really cause an inconvenience to anybody.
>
> I can only speak for myself, but personally I don't mind. The only
> reason I highlighted them is because I thought they were an oversight.
FWIW at <https://patchwork.sourceware.org/project/gdb/> we have a working
instance of patchwork too that can be used to track own and other people's
submissions, which also has been recently brought up to date as far as the
engine is concerned, thanks to people using it actively for the glibc
project. They've even set up a weekly meeting recently to review patches:
<https://sourceware.org/glibc/wiki/PatchworkReviewMeetings>. That might
be something to consider I believe for patch management.
Maciej
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] gdb: make discrete_position return optional
2020-12-08 11:41 ` Maciej W. Rozycki
@ 2020-12-09 19:29 ` Simon Marchi
2020-12-09 19:53 ` Maciej W. Rozycki
0 siblings, 1 reply; 21+ messages in thread
From: Simon Marchi @ 2020-12-09 19:29 UTC (permalink / raw)
To: Maciej W. Rozycki, Joel Brobecker
Cc: Simon Marchi, Simon Marchi via Gdb-patches
On 2020-12-08 6:41 a.m., Maciej W. Rozycki wrote:
> FWIW at <https://patchwork.sourceware.org/project/gdb/> we have a working
> instance of patchwork too that can be used to track own and other people's
> submissions, which also has been recently brought up to date as far as the
> engine is concerned, thanks to people using it actively for the glibc
> project. They've even set up a weekly meeting recently to review patches:
> <https://sourceware.org/glibc/wiki/PatchworkReviewMeetings>. That might
> be something to consider I believe for patch management.
Yes, I think we should use it!
But our previous experience with Patchwork is that it's a lot of work to
manage patches to keep Patchwork in a good state. When you send a v2, v3,
v4 of a series, it creates new patches, and someone needs to manually go
set the older ones as "Superseded". And when then patches get merged,
someone needs to go mark them as "Merged" so they go away.
Is this how it still works?
If Patchwork could do that somewhat automatically and reliably (say, by
relying on same the Change-Ids as Gerrit uses), I think it would be
wonderful.
Last time I checked [1], the upstream project was open to receive such
contributions, but of course it takes some effort to implement.
[1 https://github.com/getpatchwork/patchwork/issues/327
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] gdb: make discrete_position return optional
2020-12-09 19:29 ` Simon Marchi
@ 2020-12-09 19:53 ` Maciej W. Rozycki
0 siblings, 0 replies; 21+ messages in thread
From: Maciej W. Rozycki @ 2020-12-09 19:53 UTC (permalink / raw)
To: Simon Marchi; +Cc: Joel Brobecker, Simon Marchi, Simon Marchi via Gdb-patches
On Wed, 9 Dec 2020, Simon Marchi wrote:
> > FWIW at <https://patchwork.sourceware.org/project/gdb/> we have a working
> > instance of patchwork too that can be used to track own and other people's
> > submissions, which also has been recently brought up to date as far as the
> > engine is concerned, thanks to people using it actively for the glibc
> > project. They've even set up a weekly meeting recently to review patches:
> > <https://sourceware.org/glibc/wiki/PatchworkReviewMeetings>. That might
> > be something to consider I believe for patch management.
>
> Yes, I think we should use it!
>
> But our previous experience with Patchwork is that it's a lot of work to
> manage patches to keep Patchwork in a good state. When you send a v2, v3,
> v4 of a series, it creates new patches, and someone needs to manually go
> set the older ones as "Superseded". And when then patches get merged,
> someone needs to go mark them as "Merged" so they go away.
>
> Is this how it still works?
>
> If Patchwork could do that somewhat automatically and reliably (say, by
> relying on same the Change-Ids as Gerrit uses), I think it would be
> wonderful.
This is a known issue glibc people have been actively working on:
<https://sourceware.org/pipermail/libc-alpha/2020-December/120473.html>
and given that we share the instance of patchwork once it has been done it
will either work automatically for us too, or it should be easy to adapt.
Also I guess someone from our side might as well step in and participate
in the effort.
Maciej
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/4] gdb: make get_discrete_bounds return bool
2020-11-23 16:21 [PATCH 0/4] Fix bug in value_subscript when range's high bound is not known Simon Marchi
2020-11-23 16:21 ` [PATCH 1/4] gdb: make discrete_position return optional Simon Marchi
@ 2020-11-23 16:21 ` Simon Marchi
2020-12-06 6:03 ` Joel Brobecker
2020-11-23 16:21 ` [PATCH 3/4] gdb: split get_discrete_bounds in two Simon Marchi
2020-11-23 16:21 ` [PATCH 4/4] gdb: fix value_subscript when array upper bound is not known Simon Marchi
3 siblings, 1 reply; 21+ messages in thread
From: Simon Marchi @ 2020-11-23 16:21 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
get_discrete_bounds currently has three possible return values (see its
current doc for details). It appears that for all callers, it would be
sufficient to have a boolean "worked" / "didn't work" return value.
Change the return type of get_discrete_bounds to bool and adjust all
callers. Doing so simplifies the following patch.
gdb/ChangeLog:
* gdbtypes.h (get_discrete_bounds): Return bool, adjust all
callers.
* gdbtypes.c (get_discrete_bounds): Return bool.
Change-Id: Ie51feee23c75f0cd7939742604282d745db59172
---
gdb/ada-lang.c | 6 ++---
gdb/ada-valprint.c | 2 +-
gdb/c-lang.c | 4 ++--
gdb/eval.c | 4 ++--
gdb/f-array-walker.h | 4 ++--
gdb/f-lang.c | 2 +-
gdb/gdbtypes.c | 53 ++++++++++++++++++++------------------------
gdb/gdbtypes.h | 8 ++++++-
gdb/m2-typeprint.c | 4 ++--
gdb/m2-valprint.c | 6 ++---
gdb/p-valprint.c | 3 ++-
gdb/valarith.c | 2 +-
gdb/valops.c | 4 ++--
13 files changed, 52 insertions(+), 50 deletions(-)
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 9ff470d97a7..ba8dde0e9af 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2114,7 +2114,7 @@ constrained_packed_array_type (struct type *type, long *elt_bits)
if ((check_typedef (index_type)->code () == TYPE_CODE_RANGE
&& is_dynamic_type (check_typedef (index_type)))
- || get_discrete_bounds (index_type, &low_bound, &high_bound) < 0)
+ || !get_discrete_bounds (index_type, &low_bound, &high_bound))
low_bound = high_bound = 0;
if (high_bound < low_bound)
*elt_bits = TYPE_LENGTH (new_type) = 0;
@@ -2184,7 +2184,7 @@ recursively_update_array_bitsize (struct type *type)
gdb_assert (type->code () == TYPE_CODE_ARRAY);
LONGEST low, high;
- if (get_discrete_bounds (type->index_type (), &low, &high) < 0
+ if (!get_discrete_bounds (type->index_type (), &low, &high)
|| low > high)
return 0;
LONGEST our_len = high - low + 1;
@@ -2301,7 +2301,7 @@ value_subscript_packed (struct value *arr, int arity, struct value **ind)
LONGEST lowerbound, upperbound;
LONGEST idx;
- if (get_discrete_bounds (range_type, &lowerbound, &upperbound) < 0)
+ if (!get_discrete_bounds (range_type, &lowerbound, &upperbound))
{
lim_warning (_("don't know bounds of array"));
lowerbound = upperbound = 0;
diff --git a/gdb/ada-valprint.c b/gdb/ada-valprint.c
index 482069a3fb2..09e9458f427 100644
--- a/gdb/ada-valprint.c
+++ b/gdb/ada-valprint.c
@@ -136,7 +136,7 @@ val_print_packed_array_elements (struct type *type, const gdb_byte *valaddr,
{
LONGEST high;
- if (get_discrete_bounds (index_type, &low, &high) < 0)
+ if (!get_discrete_bounds (index_type, &low, &high))
len = 1;
else if (low > high)
{
diff --git a/gdb/c-lang.c b/gdb/c-lang.c
index 624aea52f77..9d59c1333d4 100644
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -698,8 +698,8 @@ evaluate_subexp_c (struct type *expect_type, struct expression *exp,
LONGEST low_bound, high_bound;
int element_size = TYPE_LENGTH (type);
- if (get_discrete_bounds (expect_type->index_type (),
- &low_bound, &high_bound) < 0)
+ if (!get_discrete_bounds (expect_type->index_type (),
+ &low_bound, &high_bound))
{
low_bound = 0;
high_bound = (TYPE_LENGTH (expect_type) / element_size) - 1;
diff --git a/gdb/eval.c b/gdb/eval.c
index 2626ee6d876..6546b7dd5c6 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -1423,7 +1423,7 @@ evaluate_subexp_standard (struct type *expect_type,
int element_size = TYPE_LENGTH (check_typedef (element_type));
LONGEST low_bound, high_bound, index;
- if (get_discrete_bounds (range_type, &low_bound, &high_bound) < 0)
+ if (!get_discrete_bounds (range_type, &low_bound, &high_bound))
{
low_bound = 0;
high_bound = (TYPE_LENGTH (type) / element_size) - 1;
@@ -1476,7 +1476,7 @@ evaluate_subexp_standard (struct type *expect_type,
|| check_type->code () == TYPE_CODE_TYPEDEF)
check_type = TYPE_TARGET_TYPE (check_type);
- if (get_discrete_bounds (element_type, &low_bound, &high_bound) < 0)
+ if (!get_discrete_bounds (element_type, &low_bound, &high_bound))
error (_("(power)set type with unknown size"));
memset (valaddr, '\0', TYPE_LENGTH (type));
for (tem = 0; tem < nargs; tem++)
diff --git a/gdb/f-array-walker.h b/gdb/f-array-walker.h
index 417f9f07980..c202df6c65f 100644
--- a/gdb/f-array-walker.h
+++ b/gdb/f-array-walker.h
@@ -42,7 +42,7 @@ class fortran_array_offset_calculator
/* Get the range, and extract the bounds. */
struct type *range_type = type->index_type ();
- if (get_discrete_bounds (range_type, &m_lowerbound, &m_upperbound) < 0)
+ if (!get_discrete_bounds (range_type, &m_lowerbound, &m_upperbound))
error ("unable to read array bounds");
/* Figure out the stride for this array. */
@@ -198,7 +198,7 @@ class fortran_array_walker
/* Extract the range, and get lower and upper bounds. */
struct type *range_type = check_typedef (type)->index_type ();
LONGEST lowerbound, upperbound;
- if (get_discrete_bounds (range_type, &lowerbound, &upperbound) < 0)
+ if (!get_discrete_bounds (range_type, &lowerbound, &upperbound))
error ("failed to get range bounds");
/* CALC is used to calculate the offsets for each element in this
diff --git a/gdb/f-lang.c b/gdb/f-lang.c
index 4171c96c8a9..99d8b792caf 100644
--- a/gdb/f-lang.c
+++ b/gdb/f-lang.c
@@ -1423,7 +1423,7 @@ fortran_adjust_dynamic_array_base_address_hack (struct type *type,
tmp_type = check_typedef (tmp_type);
struct type *range_type = tmp_type->index_type ();
LONGEST lowerbound, upperbound, stride;
- if (get_discrete_bounds (range_type, &lowerbound, &upperbound) < 0)
+ if (!get_discrete_bounds (range_type, &lowerbound, &upperbound))
error ("failed to get range bounds");
/* Figure out the stride for this dimension. */
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 09f33c21e28..b47bd28945d 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1036,15 +1036,9 @@ has_static_range (const struct range_bounds *bounds)
&& bounds->stride.kind () == PROP_CONST);
}
+/* See gdbtypes.h. */
-/* Set *LOWP and *HIGHP to the lower and upper bounds of discrete type
- TYPE.
-
- Return 1 if type is a range type with two defined, constant bounds.
- Else, return 0 if it is discrete (and bounds will fit in LONGEST).
- Else, return -1. */
-
-int
+bool
get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
{
type = check_typedef (type);
@@ -1055,7 +1049,7 @@ get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
constant bounds. */
if (type->bounds ()->low.kind () != PROP_CONST
|| type->bounds ()->high.kind () != PROP_CONST)
- return -1;
+ return false;
*lowp = type->bounds ()->low.const_val ();
*highp = type->bounds ()->high.const_val ();
@@ -1065,20 +1059,17 @@ get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
gdb::optional<LONGEST> low_pos
= discrete_position (TYPE_TARGET_TYPE (type), *lowp);
- if (!low_pos.has_value ())
- return 0;
-
- *lowp = *low_pos;
+ if (low_pos.has_value ())
+ *lowp = *low_pos;
gdb::optional<LONGEST> high_pos
= discrete_position (TYPE_TARGET_TYPE (type), *highp);
- if (!high_pos.has_value ())
- return 0;
-
- *highp = *high_pos;
+ if (high_pos.has_value ())
+ *highp = *high_pos;
}
- return 1;
+ return true;
+
case TYPE_CODE_ENUM:
if (type->num_fields () > 0)
{
@@ -1104,19 +1095,22 @@ get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
*lowp = 0;
*highp = -1;
}
- return 0;
+ return true;
+
case TYPE_CODE_BOOL:
*lowp = 0;
*highp = 1;
- return 0;
+ return true;
+
case TYPE_CODE_INT:
if (TYPE_LENGTH (type) > sizeof (LONGEST)) /* Too big */
- return -1;
+ return false;
+
if (!type->is_unsigned ())
{
*lowp = -(1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1));
*highp = -*lowp - 1;
- return 0;
+ return true;
}
/* fall through */
case TYPE_CODE_CHAR:
@@ -1126,9 +1120,10 @@ get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
if TYPE_LENGTH (type) == sizeof (LONGEST). */
*highp = 1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1);
*highp = (*highp - 1) | *highp;
- return 0;
+ return true;
+
default:
- return -1;
+ return false;
}
}
@@ -1140,13 +1135,11 @@ get_array_bounds (struct type *type, LONGEST *low_bound, LONGEST *high_bound)
struct type *index = type->index_type ();
LONGEST low = 0;
LONGEST high = 0;
- int res;
if (index == NULL)
return false;
- res = get_discrete_bounds (index, &low, &high);
- if (res == -1)
+ if (!get_discrete_bounds (index, &low, &high))
return false;
if (low_bound)
@@ -1223,8 +1216,9 @@ update_static_array_size (struct type *type)
if (stride == 0)
stride = range_type->bit_stride ();
- if (get_discrete_bounds (range_type, &low_bound, &high_bound) < 0)
+ if (!get_discrete_bounds (range_type, &low_bound, &high_bound))
low_bound = high_bound = 0;
+
element_type = check_typedef (TYPE_TARGET_TYPE (type));
/* Be careful when setting the array length. Ada arrays can be
empty arrays with the high_bound being smaller than the low_bound.
@@ -1420,8 +1414,9 @@ create_set_type (struct type *result_type, struct type *domain_type)
{
LONGEST low_bound, high_bound, bit_length;
- if (get_discrete_bounds (domain_type, &low_bound, &high_bound) < 0)
+ if (!get_discrete_bounds (domain_type, &low_bound, &high_bound))
low_bound = high_bound = 0;
+
bit_length = high_bound - low_bound + 1;
TYPE_LENGTH (result_type)
= (bit_length + TARGET_CHAR_BIT - 1) / TARGET_CHAR_BIT;
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 59ff6fc6ce3..dd63ab8d876 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -2434,7 +2434,13 @@ extern struct type *lookup_template_type (const char *, struct type *,
extern int get_vptr_fieldno (struct type *, struct type **);
-extern int get_discrete_bounds (struct type *, LONGEST *, LONGEST *);
+/* Set *LOWP and *HIGHP to the lower and upper bounds of discrete type
+ TYPE.
+
+ Return true if the two bounds are available, false otherwise. */
+
+extern bool get_discrete_bounds (struct type *type, LONGEST *lowp,
+ LONGEST *highp);
/* Assuming TYPE is a simple, non-empty array type, compute its upper
and lower bound. Save the low bound into LOW_BOUND if not NULL.
diff --git a/gdb/m2-typeprint.c b/gdb/m2-typeprint.c
index 8fbcdf4c603..20a3d131476 100644
--- a/gdb/m2-typeprint.c
+++ b/gdb/m2-typeprint.c
@@ -372,7 +372,7 @@ m2_is_long_set (struct type *type)
This should be integrated into gdbtypes.c
inside get_discrete_bounds. */
-static int
+static bool
m2_get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
{
type = check_typedef (type);
@@ -419,7 +419,7 @@ m2_is_long_set_of_type (struct type *type, struct type **of_type)
l1 = type->field (i).type ()->bounds ()->low.const_val ();
h1 = type->field (len - 1).type ()->bounds ()->high.const_val ();
*of_type = target;
- if (m2_get_discrete_bounds (target, &l2, &h2) >= 0)
+ if (m2_get_discrete_bounds (target, &l2, &h2))
return (l1 == l2 && h1 == h2);
error (_("long_set failed to find discrete bounds for its subtype"));
return 0;
diff --git a/gdb/m2-valprint.c b/gdb/m2-valprint.c
index 96dc18119cf..542fa492d06 100644
--- a/gdb/m2-valprint.c
+++ b/gdb/m2-valprint.c
@@ -97,7 +97,7 @@ m2_print_long_set (struct type *type, const gdb_byte *valaddr,
target = TYPE_TARGET_TYPE (range);
- if (get_discrete_bounds (range, &field_low, &field_high) >= 0)
+ if (get_discrete_bounds (range, &field_low, &field_high))
{
for (i = low_bound; i <= high_bound; i++)
{
@@ -137,7 +137,7 @@ m2_print_long_set (struct type *type, const gdb_byte *valaddr,
if (field == len)
break;
range = type->field (field).type ()->index_type ();
- if (get_discrete_bounds (range, &field_low, &field_high) < 0)
+ if (!get_discrete_bounds (range, &field_low, &field_high))
break;
target = TYPE_TARGET_TYPE (range);
}
@@ -399,7 +399,7 @@ m2_language::value_print_inner (struct value *val, struct ui_file *stream,
fputs_filtered ("{", stream);
- i = get_discrete_bounds (range, &low_bound, &high_bound);
+ i = get_discrete_bounds (range, &low_bound, &high_bound) ? 0 : -1;
maybe_bad_bstring:
if (i < 0)
{
diff --git a/gdb/p-valprint.c b/gdb/p-valprint.c
index 428b2efc656..8f785b71ea4 100644
--- a/gdb/p-valprint.c
+++ b/gdb/p-valprint.c
@@ -343,7 +343,8 @@ pascal_value_print_inner (struct value *val, struct ui_file *stream,
fputs_filtered ("[", stream);
- int bound_info = get_discrete_bounds (range, &low_bound, &high_bound);
+ int bound_info = (get_discrete_bounds (range, &low_bound, &high_bound)
+ ? 0 : -1);
if (low_bound == 0 && high_bound == -1 && TYPE_LENGTH (type) > 0)
{
/* If we know the size of the set type, we can figure out the
diff --git a/gdb/valarith.c b/gdb/valarith.c
index f4497cd223f..a44d198a1de 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -1942,7 +1942,7 @@ value_bit_index (struct type *type, const gdb_byte *valaddr, int index)
unsigned rel_index;
struct type *range = type->index_type ();
- if (get_discrete_bounds (range, &low_bound, &high_bound) < 0)
+ if (!get_discrete_bounds (range, &low_bound, &high_bound))
return -2;
if (index < low_bound || index > high_bound)
return -1;
diff --git a/gdb/valops.c b/gdb/valops.c
index 0f84a70ceb6..4fc65ea20f5 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -451,7 +451,7 @@ value_cast (struct type *type, struct value *arg2)
int val_length = TYPE_LENGTH (type2);
LONGEST low_bound, high_bound, new_length;
- if (get_discrete_bounds (range_type, &low_bound, &high_bound) < 0)
+ if (!get_discrete_bounds (range_type, &low_bound, &high_bound))
low_bound = 0, high_bound = 0;
new_length = val_length / element_length;
if (val_length % element_length != 0)
@@ -3946,7 +3946,7 @@ value_slice (struct value *array, int lowbound, int length)
error (_("array not associated"));
range_type = array_type->index_type ();
- if (get_discrete_bounds (range_type, &lowerbound, &upperbound) < 0)
+ if (!get_discrete_bounds (range_type, &lowerbound, &upperbound))
error (_("slice from bad array or bitstring"));
if (lowbound < lowerbound || length < 0
--
2.29.2
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/4] gdb: make get_discrete_bounds return bool
2020-11-23 16:21 ` [PATCH 2/4] gdb: make get_discrete_bounds return bool Simon Marchi
@ 2020-12-06 6:03 ` Joel Brobecker
2020-12-07 15:19 ` Simon Marchi
0 siblings, 1 reply; 21+ messages in thread
From: Joel Brobecker @ 2020-12-06 6:03 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi
Hi Simon,
On Mon, Nov 23, 2020 at 11:21:18AM -0500, Simon Marchi via Gdb-patches wrote:
> get_discrete_bounds currently has three possible return values (see its
> current doc for details). It appears that for all callers, it would be
> sufficient to have a boolean "worked" / "didn't work" return value.
>
> Change the return type of get_discrete_bounds to bool and adjust all
> callers. Doing so simplifies the following patch.
>
> gdb/ChangeLog:
>
> * gdbtypes.h (get_discrete_bounds): Return bool, adjust all
> callers.
> * gdbtypes.c (get_discrete_bounds): Return bool.
>
> Change-Id: Ie51feee23c75f0cd7939742604282d745db59172
A small reminder to remember to remove the Change-Id...
Other than that, the change looks good to me.
This is a very nice simplification of the interface, IMO, so thank you
for doing that. In reading the documentation prior to this change,
it was hard for me to wrap my head around what it the function was
really doing. In trying to understand the initial motivation, I did
a bit of archeology, and it goes all the way back to the initial creation
of the sourceware repository (20 years ago already!), so no obvious
explanation from there.
> @@ -399,7 +399,7 @@ m2_language::value_print_inner (struct value *val, struct ui_file *stream,
>
> fputs_filtered ("{", stream);
>
> - i = get_discrete_bounds (range, &low_bound, &high_bound);
> + i = get_discrete_bounds (range, &low_bound, &high_bound) ? 0 : -1;
> maybe_bad_bstring:
> if (i < 0)
> {
FTR, this hunk required a bit more context to investigate. We can see
that the change looks correct when looking at how variable "i" is
(mis)used:
i = get_discrete_bounds (range, &low_bound, &high_bound) ? 0 : -1;
maybe_bad_bstring:
if (i < 0)
{
fputs_styled (_("<error value>"), metadata_style.style (),
stream);
goto done;
}
for (i = low_bound; i <= high_bound; i++)
Because of the use of labels, it's hard to propose a simpler rewriting
which one would feel confident about without testing...
> diff --git a/gdb/p-valprint.c b/gdb/p-valprint.c
> index 428b2efc656..8f785b71ea4 100644
> --- a/gdb/p-valprint.c
> +++ b/gdb/p-valprint.c
> @@ -343,7 +343,8 @@ pascal_value_print_inner (struct value *val, struct ui_file *stream,
>
> fputs_filtered ("[", stream);
>
> - int bound_info = get_discrete_bounds (range, &low_bound, &high_bound);
> + int bound_info = (get_discrete_bounds (range, &low_bound, &high_bound)
> + ? 0 : -1);
> if (low_bound == 0 && high_bound == -1 && TYPE_LENGTH (type) > 0)
> {
> /* If we know the size of the set type, we can figure out the
Similar story here.
--
Joel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/4] gdb: make get_discrete_bounds return bool
2020-12-06 6:03 ` Joel Brobecker
@ 2020-12-07 15:19 ` Simon Marchi
0 siblings, 0 replies; 21+ messages in thread
From: Simon Marchi @ 2020-12-07 15:19 UTC (permalink / raw)
To: Joel Brobecker, Simon Marchi via Gdb-patches
On 2020-12-06 1:03 a.m., Joel Brobecker wrote:
> Hi Simon,
>
> On Mon, Nov 23, 2020 at 11:21:18AM -0500, Simon Marchi via Gdb-patches wrote:
>> get_discrete_bounds currently has three possible return values (see its
>> current doc for details). It appears that for all callers, it would be
>> sufficient to have a boolean "worked" / "didn't work" return value.
>>
>> Change the return type of get_discrete_bounds to bool and adjust all
>> callers. Doing so simplifies the following patch.
>>
>> gdb/ChangeLog:
>>
>> * gdbtypes.h (get_discrete_bounds): Return bool, adjust all
>> callers.
>> * gdbtypes.c (get_discrete_bounds): Return bool.
>>
>> Change-Id: Ie51feee23c75f0cd7939742604282d745db59172
>
> A small reminder to remember to remove the Change-Id...
>
> Other than that, the change looks good to me.
>
> This is a very nice simplification of the interface, IMO, so thank you
> for doing that. In reading the documentation prior to this change,
> it was hard for me to wrap my head around what it the function was
> really doing. In trying to understand the initial motivation, I did
> a bit of archeology, and it goes all the way back to the initial creation
> of the sourceware repository (20 years ago already!), so no obvious
> explanation from there.
>
>> @@ -399,7 +399,7 @@ m2_language::value_print_inner (struct value *val, struct ui_file *stream,
>>
>> fputs_filtered ("{", stream);
>>
>> - i = get_discrete_bounds (range, &low_bound, &high_bound);
>> + i = get_discrete_bounds (range, &low_bound, &high_bound) ? 0 : -1;
>> maybe_bad_bstring:
>> if (i < 0)
>> {
>
> FTR, this hunk required a bit more context to investigate. We can see
> that the change looks correct when looking at how variable "i" is
> (mis)used:
>
> i = get_discrete_bounds (range, &low_bound, &high_bound) ? 0 : -1;
> maybe_bad_bstring:
> if (i < 0)
> {
> fputs_styled (_("<error value>"), metadata_style.style (),
> stream);
> goto done;
> }
>
> for (i = low_bound; i <= high_bound; i++)
>
> Because of the use of labels, it's hard to propose a simpler rewriting
> which one would feel confident about without testing...
Indeed this code is really messy. For the error handling of the
value_bit_index, it just assigns `i` again and jumps back to
maybe_bad_bstring, this is crazy. It's really tempting to do more
cleanup, but I prefer to just to the bare minimum changes in the Modula
and Pascal code.
Simon
>
>
>> diff --git a/gdb/p-valprint.c b/gdb/p-valprint.c
>> index 428b2efc656..8f785b71ea4 100644
>> --- a/gdb/p-valprint.c
>> +++ b/gdb/p-valprint.c
>> @@ -343,7 +343,8 @@ pascal_value_print_inner (struct value *val, struct ui_file *stream,
>>
>> fputs_filtered ("[", stream);
>>
>> - int bound_info = get_discrete_bounds (range, &low_bound, &high_bound);
>> + int bound_info = (get_discrete_bounds (range, &low_bound, &high_bound)
>> + ? 0 : -1);
>> if (low_bound == 0 && high_bound == -1 && TYPE_LENGTH (type) > 0)
>> {
>> /* If we know the size of the set type, we can figure out the
>
> Similar story here.
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/4] gdb: split get_discrete_bounds in two
2020-11-23 16:21 [PATCH 0/4] Fix bug in value_subscript when range's high bound is not known Simon Marchi
2020-11-23 16:21 ` [PATCH 1/4] gdb: make discrete_position return optional Simon Marchi
2020-11-23 16:21 ` [PATCH 2/4] gdb: make get_discrete_bounds return bool Simon Marchi
@ 2020-11-23 16:21 ` Simon Marchi
2020-12-06 7:29 ` Joel Brobecker
2020-11-23 16:21 ` [PATCH 4/4] gdb: fix value_subscript when array upper bound is not known Simon Marchi
3 siblings, 1 reply; 21+ messages in thread
From: Simon Marchi @ 2020-11-23 16:21 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
get_discrete_bounds is not flexible for ranges (TYPE_CODE_RANGE), in the
sense that it returns true (success) only if both bounds are present and
constant values.
This is a problem for code that only needs to know the low bound and
fails unnecessarily if the high bound is unknown.
Split the function in two, get_discrete_low_bound and
get_discrete_high_bound, that both return an optional. Provide a new
implementation of get_discrete_bounds based on the two others, so the
callers don't have to be changed.
gdb/ChangeLog:
* gdbtypes.c (get_discrete_bounds): Implement with
get_discrete_low_bound and get_discrete_high_bound.
(get_discrete_low_bound): New.
(get_discrete_high_bound): New.
Change-Id: I986b5e9c0dd969800e3fb9546af9c827d52e80d0
---
gdb/gdbtypes.c | 187 ++++++++++++++++++++++++++++++++++---------------
1 file changed, 130 insertions(+), 57 deletions(-)
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index b47bd28945d..4df23cfe0fe 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1036,71 +1036,127 @@ has_static_range (const struct range_bounds *bounds)
&& bounds->stride.kind () == PROP_CONST);
}
-/* See gdbtypes.h. */
+/* If TYPE's low bound is a known constant, return it, else return nullopt. */
-bool
-get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
+static gdb::optional<LONGEST>
+get_discrete_low_bound (struct type *type)
{
type = check_typedef (type);
switch (type->code ())
{
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)
+ {
+ /* This function only works for ranges with a constant low bound. */
+ if (type->bounds ()->low.kind () != PROP_CONST)
+ return {};
+
+ LONGEST low = type->bounds ()->low.const_val ();
+
+ if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
+ {
+ gdb::optional<LONGEST> low_pos
+ = discrete_position (TYPE_TARGET_TYPE (type), low);
+
+ if (low_pos.has_value ())
+ low = *low_pos;
+ }
+
+ return low;
+ }
+
+ case TYPE_CODE_ENUM:
+ {
+ if (type->num_fields () > 0)
+ {
+ /* The enums may not be sorted by value, so search all
+ entries. */
+ LONGEST low = TYPE_FIELD_ENUMVAL (type, 0);
+
+ for (int i = 0; i < type->num_fields (); i++)
+ {
+ if (TYPE_FIELD_ENUMVAL (type, i) < low)
+ low = TYPE_FIELD_ENUMVAL (type, i);
+ }
+
+ /* Set unsigned indicator if warranted. */
+ if (low >= 0)
+ type->set_is_unsigned (true);
+
+ return low;
+ }
+ else
+ return 0;
+ }
+
+ case TYPE_CODE_BOOL:
+ return 0;
+
+ case TYPE_CODE_INT:
+ if (TYPE_LENGTH (type) > sizeof (LONGEST)) /* Too big */
return false;
- *lowp = type->bounds ()->low.const_val ();
- *highp = type->bounds ()->high.const_val ();
+ if (!type->is_unsigned ())
+ return -(1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1));
- if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
- {
- gdb::optional<LONGEST> low_pos
- = discrete_position (TYPE_TARGET_TYPE (type), *lowp);
+ /* fall through */
+ case TYPE_CODE_CHAR:
+ return 0;
- if (low_pos.has_value ())
- *lowp = *low_pos;
+ default:
+ return false;
+ }
+}
- gdb::optional<LONGEST> high_pos
- = discrete_position (TYPE_TARGET_TYPE (type), *highp);
+/* If TYPE's high bound is a known constant, return it, else return nullopt. */
- if (high_pos.has_value ())
- *highp = *high_pos;
- }
- return true;
+static gdb::optional<LONGEST>
+get_discrete_high_bound (struct type *type)
+{
+ type = check_typedef (type);
+ switch (type->code ())
+ {
+ case TYPE_CODE_RANGE:
+ {
+ /* This function only works for ranges with a constant high bound. */
+ if (type->bounds ()->high.kind () != PROP_CONST)
+ return {};
+
+ LONGEST high = type->bounds ()->high.const_val ();
+
+ if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
+ {
+ gdb::optional<LONGEST> high_pos
+ = discrete_position (TYPE_TARGET_TYPE (type), high);
+
+ if (high_pos.has_value ())
+ high = *high_pos;
+ }
+
+ return high;
+ }
case TYPE_CODE_ENUM:
- if (type->num_fields () > 0)
- {
- /* The enums may not be sorted by value, so search all
- entries. */
- int i;
+ {
+ if (type->num_fields () > 0)
+ {
+ /* The enums may not be sorted by value, so search all
+ entries. */
+ LONGEST high = TYPE_FIELD_ENUMVAL (type, 0);
- *lowp = *highp = TYPE_FIELD_ENUMVAL (type, 0);
- for (i = 0; i < type->num_fields (); i++)
- {
- if (TYPE_FIELD_ENUMVAL (type, i) < *lowp)
- *lowp = TYPE_FIELD_ENUMVAL (type, i);
- if (TYPE_FIELD_ENUMVAL (type, i) > *highp)
- *highp = TYPE_FIELD_ENUMVAL (type, i);
- }
+ for (int i = 0; i < type->num_fields (); i++)
+ {
+ if (TYPE_FIELD_ENUMVAL (type, i) > high)
+ high = TYPE_FIELD_ENUMVAL (type, i);
+ }
- /* Set unsigned indicator if warranted. */
- if (*lowp >= 0)
- type->set_is_unsigned (true);
- }
- else
- {
- *lowp = 0;
- *highp = -1;
- }
- return true;
+ return high;
+ }
+ else
+ return -1;
+ }
case TYPE_CODE_BOOL:
- *lowp = 0;
- *highp = 1;
- return true;
+ return 1;
case TYPE_CODE_INT:
if (TYPE_LENGTH (type) > sizeof (LONGEST)) /* Too big */
@@ -1108,25 +1164,42 @@ get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
if (!type->is_unsigned ())
{
- *lowp = -(1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1));
- *highp = -*lowp - 1;
- return true;
+ LONGEST low = -(1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1));
+ return -low - 1;
}
+
/* fall through */
case TYPE_CODE_CHAR:
- *lowp = 0;
- /* This round-about calculation is to avoid shifting by
- TYPE_LENGTH (type) * TARGET_CHAR_BIT, which will not work
- if TYPE_LENGTH (type) == sizeof (LONGEST). */
- *highp = 1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1);
- *highp = (*highp - 1) | *highp;
- return true;
+ {
+ /* This round-about calculation is to avoid shifting by
+ TYPE_LENGTH (type) * TARGET_CHAR_BIT, which will not work
+ if TYPE_LENGTH (type) == sizeof (LONGEST). */
+ LONGEST high = 1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1);
+ return (high - 1) | high;
+ }
default:
return false;
}
}
+/* See gdbtypes.h. */
+
+bool
+get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
+{
+ gdb::optional<LONGEST> low = get_discrete_low_bound (type);
+ gdb::optional<LONGEST> high = get_discrete_high_bound (type);
+
+ if (!low.has_value () || !high.has_value ())
+ return false;
+
+ *lowp = *low;
+ *highp = *high;
+
+ return true;
+}
+
/* See gdbtypes.h */
bool
--
2.29.2
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/4] gdb: split get_discrete_bounds in two
2020-11-23 16:21 ` [PATCH 3/4] gdb: split get_discrete_bounds in two Simon Marchi
@ 2020-12-06 7:29 ` Joel Brobecker
2020-12-07 15:49 ` Simon Marchi
0 siblings, 1 reply; 21+ messages in thread
From: Joel Brobecker @ 2020-12-06 7:29 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi
Hi Simon,
On Mon, Nov 23, 2020 at 11:21:19AM -0500, Simon Marchi via Gdb-patches wrote:
> get_discrete_bounds is not flexible for ranges (TYPE_CODE_RANGE), in the
> sense that it returns true (success) only if both bounds are present and
> constant values.
>
> This is a problem for code that only needs to know the low bound and
> fails unnecessarily if the high bound is unknown.
>
> Split the function in two, get_discrete_low_bound and
> get_discrete_high_bound, that both return an optional. Provide a new
> implementation of get_discrete_bounds based on the two others, so the
> callers don't have to be changed.
>
> gdb/ChangeLog:
>
> * gdbtypes.c (get_discrete_bounds): Implement with
> get_discrete_low_bound and get_discrete_high_bound.
> (get_discrete_low_bound): New.
> (get_discrete_high_bound): New.
>
> Change-Id: I986b5e9c0dd969800e3fb9546af9c827d52e80d0
Small reminder to remove the Change-Id in the ChangeLog.
I agree with the problem, and modulo the minor question I had below
(which might not actually be an issue), the patch looks good to me.
I do have one minor concern. On the one hand, the code seems to be
slightly simpler and clearer. On the other hand, I worry a bit about
keeping the two functions in sync. I'm wondering if it might be worth
investigating ways to have achieve the same end result without
duplicating the skeleton of the code. I tried thinking about it
for a while, and the only option that might be viable that I could
think of was to allow users of get_discrete_bounds to pass nullptr
for either LOWP or HIGHP, and then only handle the bounds whose
parameter is not null. Then we can have the low/high functions
if we wanted. In trying to visualize how this would look like,
I found that two scenarios:
- I would be bracketing small pieces of code with lots of
"if <param> != nullptr" (just for TYPE_CODE_RANGE, I counted
6 instances of an addional condition); FTR, this is what it
would look like for TYPE_CODE_RANGE:
| case TYPE_CODE_RANGE:
| if ((lowp != nullptr && type->bounds ()->low.kind () != PROP_CONST)
| || (highp != nullptr && type->bounds ()->high.kind () != PROP_CONST))
| return false;
| if (lowp != nullptr)
| *lowp = type->bounds ()->low.const_val ();
| if (highp != nullptr)
| *highp = type->bounds ()->high.const_val ();
| if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
| {
| if (lowp != nullptr)
| {
| gdb::optional<LONGEST> low_pos
| = discrete_position (TYPE_TARGET_TYPE (type), *lowp);
| if (low_pos.has_value ())
| *lowp = *low_pos;
| }
| if (highp != nullptr)
| {
| gdb::optional<LONGEST> high_pos
| = discrete_position (TYPE_TARGET_TYPE (type), *highp);
| if (high_pos.has_value ())
| *highp = *high_pos;
| }
| }
| return true;
- I would be splitting the lowp and highp code in two inside
each case; in this case, when one looks at how things look like,
it takes away half of the reasons why we might want to keep
the code together. E.g. for TYPE_CODE_RANGE, it woudl look like this:
| case TYPE_CODE_RANGE:
| if (lowp != nullptr)
| {
| if (type->bounds ()->low.kind () != PROP_CONST)
| return false;
| *lowp = type->bounds ()->low.const_val ();
| if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
| {
| gdb::optional<LONGEST> low_pos
| = discrete_position (TYPE_TARGET_TYPE (type), *lowp);
| if (low_pos.has_value ())
| *lowp = *low_pos;
| }
| }
| if (lowp != nullptr)
| {
| if (type->bounds ()->high.kind () != PROP_CONST)
| return false;
| *highp = type->bounds ()->high.const_val ();
| if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
| {
| gdb::optional<LONGEST> high_pos
| = discrete_position (TYPE_TARGET_TYPE (type), *highp);
| if (high_pos.has_value ())
| *highp = *high_pos;
| }
| }
| return true;
For the record, I thought about perhaps using templates, which would
work, but I think at the cost of the templated code being more complex.
So, in the end, unless you have some better ideas, I am find with
either your solution (splitting), or the solution above which has
the property of better keeping the code together, but is far from
perfect because it is at the cost of complexifying the implementation
a bit (one has to always check that LOWP/HIGHP is not nullptr).
See comments below, though.
> ---
> gdb/gdbtypes.c | 187 ++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 130 insertions(+), 57 deletions(-)
>
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index b47bd28945d..4df23cfe0fe 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -1036,71 +1036,127 @@ has_static_range (const struct range_bounds *bounds)
> && bounds->stride.kind () == PROP_CONST);
> }
>
> -/* See gdbtypes.h. */
> +/* If TYPE's low bound is a known constant, return it, else return nullopt. */
>
> -bool
> -get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
> +static gdb::optional<LONGEST>
> +get_discrete_low_bound (struct type *type)
> {
> type = check_typedef (type);
> switch (type->code ())
> {
> 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)
> + {
> + /* This function only works for ranges with a constant low bound. */
> + if (type->bounds ()->low.kind () != PROP_CONST)
> + return {};
> +
> + LONGEST low = type->bounds ()->low.const_val ();
> +
> + if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
> + {
> + gdb::optional<LONGEST> low_pos
> + = discrete_position (TYPE_TARGET_TYPE (type), low);
> +
> + if (low_pos.has_value ())
> + low = *low_pos;
> + }
> +
> + return low;
> + }
> +
> + case TYPE_CODE_ENUM:
> + {
> + if (type->num_fields () > 0)
> + {
> + /* The enums may not be sorted by value, so search all
> + entries. */
> + LONGEST low = TYPE_FIELD_ENUMVAL (type, 0);
> +
> + for (int i = 0; i < type->num_fields (); i++)
> + {
> + if (TYPE_FIELD_ENUMVAL (type, i) < low)
> + low = TYPE_FIELD_ENUMVAL (type, i);
> + }
> +
> + /* Set unsigned indicator if warranted. */
> + if (low >= 0)
> + type->set_is_unsigned (true);
> +
> + return low;
> + }
> + else
> + return 0;
> + }
> +
> + case TYPE_CODE_BOOL:
> + return 0;
> +
> + case TYPE_CODE_INT:
> + if (TYPE_LENGTH (type) > sizeof (LONGEST)) /* Too big */
> return false;
Should this be "return {};" here?
>
> - *lowp = type->bounds ()->low.const_val ();
> - *highp = type->bounds ()->high.const_val ();
> + if (!type->is_unsigned ())
> + return -(1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1));
>
> - if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
> - {
> - gdb::optional<LONGEST> low_pos
> - = discrete_position (TYPE_TARGET_TYPE (type), *lowp);
> + /* fall through */
> + case TYPE_CODE_CHAR:
> + return 0;
>
> - if (low_pos.has_value ())
> - *lowp = *low_pos;
> + default:
> + return false;
Same as above.
> + }
> +}
>
> - gdb::optional<LONGEST> high_pos
> - = discrete_position (TYPE_TARGET_TYPE (type), *highp);
> +/* If TYPE's high bound is a known constant, return it, else return nullopt. */
>
> - if (high_pos.has_value ())
> - *highp = *high_pos;
> - }
> - return true;
> +static gdb::optional<LONGEST>
> +get_discrete_high_bound (struct type *type)
> +{
> + type = check_typedef (type);
> + switch (type->code ())
> + {
> + case TYPE_CODE_RANGE:
> + {
> + /* This function only works for ranges with a constant high bound. */
> + if (type->bounds ()->high.kind () != PROP_CONST)
> + return {};
> +
> + LONGEST high = type->bounds ()->high.const_val ();
> +
> + if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
> + {
> + gdb::optional<LONGEST> high_pos
> + = discrete_position (TYPE_TARGET_TYPE (type), high);
> +
> + if (high_pos.has_value ())
> + high = *high_pos;
> + }
> +
> + return high;
> + }
>
> case TYPE_CODE_ENUM:
> - if (type->num_fields () > 0)
> - {
> - /* The enums may not be sorted by value, so search all
> - entries. */
> - int i;
> + {
> + if (type->num_fields () > 0)
> + {
> + /* The enums may not be sorted by value, so search all
> + entries. */
> + LONGEST high = TYPE_FIELD_ENUMVAL (type, 0);
>
> - *lowp = *highp = TYPE_FIELD_ENUMVAL (type, 0);
> - for (i = 0; i < type->num_fields (); i++)
> - {
> - if (TYPE_FIELD_ENUMVAL (type, i) < *lowp)
> - *lowp = TYPE_FIELD_ENUMVAL (type, i);
> - if (TYPE_FIELD_ENUMVAL (type, i) > *highp)
> - *highp = TYPE_FIELD_ENUMVAL (type, i);
> - }
> + for (int i = 0; i < type->num_fields (); i++)
> + {
> + if (TYPE_FIELD_ENUMVAL (type, i) > high)
> + high = TYPE_FIELD_ENUMVAL (type, i);
> + }
>
> - /* Set unsigned indicator if warranted. */
> - if (*lowp >= 0)
> - type->set_is_unsigned (true);
> - }
> - else
> - {
> - *lowp = 0;
> - *highp = -1;
> - }
> - return true;
> + return high;
> + }
> + else
> + return -1;
> + }
>
> case TYPE_CODE_BOOL:
> - *lowp = 0;
> - *highp = 1;
> - return true;
> + return 1;
>
> case TYPE_CODE_INT:
> if (TYPE_LENGTH (type) > sizeof (LONGEST)) /* Too big */
In that section, there is a "return false" that I'm wondering
needs to be changed to "return {}" as well...
> @@ -1108,25 +1164,42 @@ get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
>
> if (!type->is_unsigned ())
> {
> - *lowp = -(1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1));
> - *highp = -*lowp - 1;
> - return true;
> + LONGEST low = -(1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1));
> + return -low - 1;
> }
> +
> /* fall through */
> case TYPE_CODE_CHAR:
> - *lowp = 0;
> - /* This round-about calculation is to avoid shifting by
> - TYPE_LENGTH (type) * TARGET_CHAR_BIT, which will not work
> - if TYPE_LENGTH (type) == sizeof (LONGEST). */
> - *highp = 1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1);
> - *highp = (*highp - 1) | *highp;
> - return true;
> + {
> + /* This round-about calculation is to avoid shifting by
> + TYPE_LENGTH (type) * TARGET_CHAR_BIT, which will not work
> + if TYPE_LENGTH (type) == sizeof (LONGEST). */
> + LONGEST high = 1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1);
> + return (high - 1) | high;
> + }
>
> default:
> return false;
Same as above.
> }
> }
>
> +/* See gdbtypes.h. */
> +
> +bool
> +get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
> +{
> + gdb::optional<LONGEST> low = get_discrete_low_bound (type);
> + gdb::optional<LONGEST> high = get_discrete_high_bound (type);
> +
> + if (!low.has_value () || !high.has_value ())
> + return false;
What do you think of computing high only after having verified
that "low" has a value? It would only be a small optimization,
but since it doesn't really complexify the code, and only adds
a couple of extra lines of code, it seems worth having?
One thing also that this patch makes me realize is that it answers
one question I had in the back of my mind: LOWP and HIGHP are only
set if both bounds can be computed -- otherwise, they are both
left untouched. Perhaps we can in a followup patch update the function's
doc to mention that?
> + *lowp = *low;
> + *highp = *high;
> +
> + return true;
> +}
> +
> /* See gdbtypes.h */
--
Joel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/4] gdb: split get_discrete_bounds in two
2020-12-06 7:29 ` Joel Brobecker
@ 2020-12-07 15:49 ` Simon Marchi
0 siblings, 0 replies; 21+ messages in thread
From: Simon Marchi @ 2020-12-07 15:49 UTC (permalink / raw)
To: Joel Brobecker, Simon Marchi via Gdb-patches
On 2020-12-06 2:29 a.m., Joel Brobecker wrote:
> Hi Simon,
>
> On Mon, Nov 23, 2020 at 11:21:19AM -0500, Simon Marchi via Gdb-patches wrote:
>> get_discrete_bounds is not flexible for ranges (TYPE_CODE_RANGE), in the
>> sense that it returns true (success) only if both bounds are present and
>> constant values.
>>
>> This is a problem for code that only needs to know the low bound and
>> fails unnecessarily if the high bound is unknown.
>>
>> Split the function in two, get_discrete_low_bound and
>> get_discrete_high_bound, that both return an optional. Provide a new
>> implementation of get_discrete_bounds based on the two others, so the
>> callers don't have to be changed.
>>
>> gdb/ChangeLog:
>>
>> * gdbtypes.c (get_discrete_bounds): Implement with
>> get_discrete_low_bound and get_discrete_high_bound.
>> (get_discrete_low_bound): New.
>> (get_discrete_high_bound): New.
>>
>> Change-Id: I986b5e9c0dd969800e3fb9546af9c827d52e80d0
>
> Small reminder to remove the Change-Id in the ChangeLog.
>
> I agree with the problem, and modulo the minor question I had below
> (which might not actually be an issue), the patch looks good to me.
>
> I do have one minor concern. On the one hand, the code seems to be
> slightly simpler and clearer. On the other hand, I worry a bit about
> keeping the two functions in sync. I'm wondering if it might be worth
> investigating ways to have achieve the same end result without
> duplicating the skeleton of the code. I tried thinking about it
> for a while, and the only option that might be viable that I could
> think of was to allow users of get_discrete_bounds to pass nullptr
> for either LOWP or HIGHP, and then only handle the bounds whose
> parameter is not null. Then we can have the low/high functions
> if we wanted. In trying to visualize how this would look like,
> I found that two scenarios:
>
> - I would be bracketing small pieces of code with lots of
> "if <param> != nullptr" (just for TYPE_CODE_RANGE, I counted
> 6 instances of an addional condition); FTR, this is what it
> would look like for TYPE_CODE_RANGE:
>
> | case TYPE_CODE_RANGE:
> | if ((lowp != nullptr && type->bounds ()->low.kind () != PROP_CONST)
> | || (highp != nullptr && type->bounds ()->high.kind () != PROP_CONST))
> | return false;
>
> | if (lowp != nullptr)
> | *lowp = type->bounds ()->low.const_val ();
> | if (highp != nullptr)
> | *highp = type->bounds ()->high.const_val ();
>
> | if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
> | {
> | if (lowp != nullptr)
> | {
> | gdb::optional<LONGEST> low_pos
> | = discrete_position (TYPE_TARGET_TYPE (type), *lowp);
>
> | if (low_pos.has_value ())
> | *lowp = *low_pos;
> | }
>
> | if (highp != nullptr)
> | {
> | gdb::optional<LONGEST> high_pos
> | = discrete_position (TYPE_TARGET_TYPE (type), *highp);
>
> | if (high_pos.has_value ())
> | *highp = *high_pos;
> | }
> | }
> | return true;
>
>
> - I would be splitting the lowp and highp code in two inside
> each case; in this case, when one looks at how things look like,
> it takes away half of the reasons why we might want to keep
> the code together. E.g. for TYPE_CODE_RANGE, it woudl look like this:
>
> | case TYPE_CODE_RANGE:
> | if (lowp != nullptr)
> | {
> | if (type->bounds ()->low.kind () != PROP_CONST)
> | return false;
>
> | *lowp = type->bounds ()->low.const_val ();
>
> | if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
> | {
> | gdb::optional<LONGEST> low_pos
> | = discrete_position (TYPE_TARGET_TYPE (type), *lowp);
>
> | if (low_pos.has_value ())
> | *lowp = *low_pos;
> | }
> | }
> | if (lowp != nullptr)
> | {
> | if (type->bounds ()->high.kind () != PROP_CONST)
> | return false;
>
> | *highp = type->bounds ()->high.const_val ();
>
> | if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
> | {
> | gdb::optional<LONGEST> high_pos
> | = discrete_position (TYPE_TARGET_TYPE (type), *highp);
>
> | if (high_pos.has_value ())
> | *highp = *high_pos;
> | }
> | }
> | return true;
>
> For the record, I thought about perhaps using templates, which would
> work, but I think at the cost of the templated code being more complex.
>
> So, in the end, unless you have some better ideas, I am find with
> either your solution (splitting), or the solution above which has
> the property of better keeping the code together, but is far from
> perfect because it is at the cost of complexifying the implementation
> a bit (one has to always check that LOWP/HIGHP is not nullptr).
Yeah, I tried several solutions and splitting was the one I preferred,
as I preferred having two simpler functions than one more complex one.
>
> See comments below, though.
>
>> ---
>> gdb/gdbtypes.c | 187 ++++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 130 insertions(+), 57 deletions(-)
>>
>> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
>> index b47bd28945d..4df23cfe0fe 100644
>> --- a/gdb/gdbtypes.c
>> +++ b/gdb/gdbtypes.c
>> @@ -1036,71 +1036,127 @@ has_static_range (const struct range_bounds *bounds)
>> && bounds->stride.kind () == PROP_CONST);
>> }
>>
>> -/* See gdbtypes.h. */
>> +/* If TYPE's low bound is a known constant, return it, else return nullopt. */
>>
>> -bool
>> -get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
>> +static gdb::optional<LONGEST>
>> +get_discrete_low_bound (struct type *type)
>> {
>> type = check_typedef (type);
>> switch (type->code ())
>> {
>> 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)
>> + {
>> + /* This function only works for ranges with a constant low bound. */
>> + if (type->bounds ()->low.kind () != PROP_CONST)
>> + return {};
>> +
>> + LONGEST low = type->bounds ()->low.const_val ();
>> +
>> + if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
>> + {
>> + gdb::optional<LONGEST> low_pos
>> + = discrete_position (TYPE_TARGET_TYPE (type), low);
>> +
>> + if (low_pos.has_value ())
>> + low = *low_pos;
>> + }
>> +
>> + return low;
>> + }
>> +
>> + case TYPE_CODE_ENUM:
>> + {
>> + if (type->num_fields () > 0)
>> + {
>> + /* The enums may not be sorted by value, so search all
>> + entries. */
>> + LONGEST low = TYPE_FIELD_ENUMVAL (type, 0);
>> +
>> + for (int i = 0; i < type->num_fields (); i++)
>> + {
>> + if (TYPE_FIELD_ENUMVAL (type, i) < low)
>> + low = TYPE_FIELD_ENUMVAL (type, i);
>> + }
>> +
>> + /* Set unsigned indicator if warranted. */
>> + if (low >= 0)
>> + type->set_is_unsigned (true);
>> +
>> + return low;
>> + }
>> + else
>> + return 0;
>> + }
>> +
>> + case TYPE_CODE_BOOL:
>> + return 0;
>> +
>> + case TYPE_CODE_INT:
>> + if (TYPE_LENGTH (type) > sizeof (LONGEST)) /* Too big */
>> return false;
>
> Should this be "return {};" here?
Hmm, yes, nice catch!
>>
>> - *lowp = type->bounds ()->low.const_val ();
>> - *highp = type->bounds ()->high.const_val ();
>> + if (!type->is_unsigned ())
>> + return -(1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1));
>>
>> - if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
>> - {
>> - gdb::optional<LONGEST> low_pos
>> - = discrete_position (TYPE_TARGET_TYPE (type), *lowp);
>> + /* fall through */
>> + case TYPE_CODE_CHAR:
>> + return 0;
>>
>> - if (low_pos.has_value ())
>> - *lowp = *low_pos;
>> + default:
>> + return false;
>
> Same as above.
Yes.
>
>> + }
>> +}
>>
>> - gdb::optional<LONGEST> high_pos
>> - = discrete_position (TYPE_TARGET_TYPE (type), *highp);
>> +/* If TYPE's high bound is a known constant, return it, else return nullopt. */
>>
>> - if (high_pos.has_value ())
>> - *highp = *high_pos;
>> - }
>> - return true;
>> +static gdb::optional<LONGEST>
>> +get_discrete_high_bound (struct type *type)
>> +{
>> + type = check_typedef (type);
>> + switch (type->code ())
>> + {
>> + case TYPE_CODE_RANGE:
>> + {
>> + /* This function only works for ranges with a constant high bound. */
>> + if (type->bounds ()->high.kind () != PROP_CONST)
>> + return {};
>> +
>> + LONGEST high = type->bounds ()->high.const_val ();
>> +
>> + if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
>> + {
>> + gdb::optional<LONGEST> high_pos
>> + = discrete_position (TYPE_TARGET_TYPE (type), high);
>> +
>> + if (high_pos.has_value ())
>> + high = *high_pos;
>> + }
>> +
>> + return high;
>> + }
>>
>> case TYPE_CODE_ENUM:
>> - if (type->num_fields () > 0)
>> - {
>> - /* The enums may not be sorted by value, so search all
>> - entries. */
>> - int i;
>> + {
>> + if (type->num_fields () > 0)
>> + {
>> + /* The enums may not be sorted by value, so search all
>> + entries. */
>> + LONGEST high = TYPE_FIELD_ENUMVAL (type, 0);
>>
>> - *lowp = *highp = TYPE_FIELD_ENUMVAL (type, 0);
>> - for (i = 0; i < type->num_fields (); i++)
>> - {
>> - if (TYPE_FIELD_ENUMVAL (type, i) < *lowp)
>> - *lowp = TYPE_FIELD_ENUMVAL (type, i);
>> - if (TYPE_FIELD_ENUMVAL (type, i) > *highp)
>> - *highp = TYPE_FIELD_ENUMVAL (type, i);
>> - }
>> + for (int i = 0; i < type->num_fields (); i++)
>> + {
>> + if (TYPE_FIELD_ENUMVAL (type, i) > high)
>> + high = TYPE_FIELD_ENUMVAL (type, i);
>> + }
>>
>> - /* Set unsigned indicator if warranted. */
>> - if (*lowp >= 0)
>> - type->set_is_unsigned (true);
>> - }
>> - else
>> - {
>> - *lowp = 0;
>> - *highp = -1;
>> - }
>> - return true;
>> + return high;
>> + }
>> + else
>> + return -1;
>> + }
>>
>> case TYPE_CODE_BOOL:
>> - *lowp = 0;
>> - *highp = 1;
>> - return true;
>> + return 1;
>>
>> case TYPE_CODE_INT:
>> if (TYPE_LENGTH (type) > sizeof (LONGEST)) /* Too big */
>
> In that section, there is a "return false" that I'm wondering
> needs to be changed to "return {}" as well...
There were 2 in get_discrete_low_bound and 2 in get_discrete_high_bound,
which I have now fixed locally, I think that's it.
The TYPE_CODE_ENUM case also looks suspicious. When the enum type has
no enumerators, the old code returns 0/-1, which looks like some kind of
error value, but also returns true, which means "success". I kept the
existing behavior for now, but I am wondering if this should be turned
into an error (returning nullopt) - maybe as a follow-up patch. I would
need to investigate all callers that might handle enums.
>
>> @@ -1108,25 +1164,42 @@ get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
>>
>> if (!type->is_unsigned ())
>> {
>> - *lowp = -(1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1));
>> - *highp = -*lowp - 1;
>> - return true;
>> + LONGEST low = -(1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1));
>> + return -low - 1;
>> }
>> +
>> /* fall through */
>> case TYPE_CODE_CHAR:
>> - *lowp = 0;
>> - /* This round-about calculation is to avoid shifting by
>> - TYPE_LENGTH (type) * TARGET_CHAR_BIT, which will not work
>> - if TYPE_LENGTH (type) == sizeof (LONGEST). */
>> - *highp = 1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1);
>> - *highp = (*highp - 1) | *highp;
>> - return true;
>> + {
>> + /* This round-about calculation is to avoid shifting by
>> + TYPE_LENGTH (type) * TARGET_CHAR_BIT, which will not work
>> + if TYPE_LENGTH (type) == sizeof (LONGEST). */
>> + LONGEST high = 1 << (TYPE_LENGTH (type) * TARGET_CHAR_BIT - 1);
>> + return (high - 1) | high;
>> + }
>>
>> default:
>> return false;
>
> Same as above.
Fixed.
>
>> }
>> }
>>
>> +/* See gdbtypes.h. */
>> +
>> +bool
>> +get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
>> +{
>> + gdb::optional<LONGEST> low = get_discrete_low_bound (type);
>> + gdb::optional<LONGEST> high = get_discrete_high_bound (type);
>> +
>> + if (!low.has_value () || !high.has_value ())
>> + return false;
>
> What do you think of computing high only after having verified
> that "low" has a value? It would only be a small optimization,
> but since it doesn't really complexify the code, and only adds
> a couple of extra lines of code, it seems worth having?
Ok, I'll do that.
>
> One thing also that this patch makes me realize is that it answers
> one question I had in the back of my mind: LOWP and HIGHP are only
> set if both bounds can be computed -- otherwise, they are both
> left untouched. Perhaps we can in a followup patch update the function's
> doc to mention that?
I'll just add it in this patch, since I am changing this function in
this patch.
Thanks,
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/4] gdb: fix value_subscript when array upper bound is not known
2020-11-23 16:21 [PATCH 0/4] Fix bug in value_subscript when range's high bound is not known Simon Marchi
` (2 preceding siblings ...)
2020-11-23 16:21 ` [PATCH 3/4] gdb: split get_discrete_bounds in two Simon Marchi
@ 2020-11-23 16:21 ` Simon Marchi
2020-12-06 7:54 ` Joel Brobecker
3 siblings, 1 reply; 21+ messages in thread
From: Simon Marchi @ 2020-11-23 16:21 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
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
---
gdb/gdbtypes.c | 8 +--
gdb/gdbtypes.h | 8 +++
.../gdb.base/flexible-array-member.c | 70 +++++++++++++++++++
.../gdb.base/flexible-array-member.exp | 66 +++++++++++++++++
gdb/valarith.c | 22 ++++--
5 files changed, 163 insertions(+), 11 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/flexible-array-member.c
create mode 100644 gdb/testsuite/gdb.base/flexible-array-member.exp
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 4df23cfe0fe..041ef677d61 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1036,9 +1036,9 @@ has_static_range (const struct range_bounds *bounds)
&& bounds->stride.kind () == PROP_CONST);
}
-/* If TYPE's low bound is a known constant, return it, else return nullopt. */
+/* See gdbtypes.h. */
-static gdb::optional<LONGEST>
+gdb::optional<LONGEST>
get_discrete_low_bound (struct type *type)
{
type = check_typedef (type);
@@ -1107,9 +1107,9 @@ get_discrete_low_bound (struct type *type)
}
}
-/* If TYPE's high bound is a known constant, return it, else return nullopt. */
+/* See gdbtypes.h. */
-static gdb::optional<LONGEST>
+gdb::optional<LONGEST>
get_discrete_high_bound (struct type *type)
{
type = check_typedef (type);
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index dd63ab8d876..6d750a3b183 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -2442,6 +2442,14 @@ extern int get_vptr_fieldno (struct type *, struct type **);
extern bool get_discrete_bounds (struct type *type, LONGEST *lowp,
LONGEST *highp);
+/* If TYPE's low bound is a known constant, return it, else return nullopt. */
+
+extern gdb::optional<LONGEST> get_discrete_low_bound (struct type *type);
+
+/* If TYPE's high bound is a known constant, return it, else return nullopt. */
+
+extern gdb::optional<LONGEST> get_discrete_high_bound (struct type *type);
+
/* Assuming TYPE is a simple, non-empty array type, compute its upper
and lower bound. Save the low bound into LOW_BOUND if not NULL.
Save the high bound into HIGH_BOUND if not NULL.
diff --git a/gdb/testsuite/gdb.base/flexible-array-member.c b/gdb/testsuite/gdb.base/flexible-array-member.c
new file mode 100644
index 00000000000..1d8bb06b514
--- /dev/null
+++ b/gdb/testsuite/gdb.base/flexible-array-member.c
@@ -0,0 +1,70 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2020 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <stdlib.h>
+
+struct no_size
+{
+ int n;
+ int items[];
+};
+
+struct zero_size
+{
+ int n;
+ int items[0];
+};
+
+struct zero_size_only
+{
+ int items[0];
+};
+
+struct no_size *ns;
+struct zero_size *zs;
+struct zero_size_only *zso;
+
+static void
+break_here (void)
+{
+}
+
+int
+main (void)
+{
+ ns = (struct no_size *) malloc (sizeof (*ns) + 3 * sizeof (int));
+ zs = (struct zero_size *) malloc (sizeof (*zs) + 3 * sizeof (int));
+ zso = (struct zero_size_only *) malloc (sizeof (*zso) + 3 * sizeof (int));
+
+ ns->n = 3;
+ ns->items[0] = 101;
+ ns->items[1] = 102;
+ ns->items[2] = 103;
+
+ zs->n = 3;
+ zs->items[0] = 201;
+ zs->items[1] = 202;
+ zs->items[2] = 203;
+
+ zso->items[0] = 301;
+ zso->items[1] = 302;
+ zso->items[2] = 303;
+
+ break_here ();
+
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.base/flexible-array-member.exp b/gdb/testsuite/gdb.base/flexible-array-member.exp
new file mode 100644
index 00000000000..973a248c5b6
--- /dev/null
+++ b/gdb/testsuite/gdb.base/flexible-array-member.exp
@@ -0,0 +1,66 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# Test printing and subscripting flexible array members.
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" \
+ ${testfile} ${srcfile}] } {
+ return
+}
+
+if { ![runto break_here] } {
+ untested "could not run to break_here"
+ return
+}
+
+# The various cases are:
+#
+# - ns: flexible array member with no size
+# - zs: flexible array member with size 0 (GNU C extension that predates the
+# standardization of the feature, but widely supported)
+# - zso: zero-size only, a corner case where the array is the sole member of
+# the structure
+
+# Print the whole structure.
+
+gdb_test "print *ns" " = {n = 3, items = $hex}"
+gdb_test "print *zs" " = {n = 3, items = $hex}"
+gdb_test "print *zso" " = {items = $hex}"
+
+# Print all items.
+
+gdb_test "print ns->items\[0\]" " = 101"
+gdb_test "print ns->items\[1\]" " = 102"
+gdb_test "print ns->items\[2\]" " = 103"
+
+gdb_test "print zs->items\[0\]" " = 201"
+gdb_test "print zs->items\[1\]" " = 202"
+gdb_test "print zs->items\[2\]" " = 203"
+
+gdb_test "print zso->items\[0\]" " = 301"
+gdb_test "print zso->items\[1\]" " = 302"
+gdb_test "print zso->items\[2\]" " = 303"
+
+# Check taking the address of array elements (how PR 28675 was originally
+# reported).
+
+gdb_test "print ns->items == &ns->items\[0\]" " = 1"
+gdb_test "print ns->items + 1 == &ns->items\[1\]" " = 1"
+gdb_test "print zs->items == &zs->items\[0\]" " = 1"
+gdb_test "print zs->items + 1 == &zs->items\[1\]" " = 1"
+gdb_test "print zso->items == &zso->items\[0\]" " = 1"
+gdb_test "print zso->items + 1 == &zso->items\[1\]" " = 1"
diff --git a/gdb/valarith.c b/gdb/valarith.c
index a44d198a1de..80ff5ee559b 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -150,25 +150,33 @@ value_subscript (struct value *array, LONGEST index)
|| tarray->code () == TYPE_CODE_STRING)
{
struct type *range_type = tarray->index_type ();
- LONGEST lowerbound, upperbound;
+ gdb::optional<LONGEST> lowerbound = get_discrete_low_bound (range_type);
+ if (!lowerbound.has_value ())
+ lowerbound = 0;
- get_discrete_bounds (range_type, &lowerbound, &upperbound);
if (VALUE_LVAL (array) != lval_memory)
- return value_subscripted_rvalue (array, index, lowerbound);
+ return value_subscripted_rvalue (array, index, *lowerbound);
if (!c_style)
{
- if (index >= lowerbound && index <= upperbound)
- return value_subscripted_rvalue (array, index, lowerbound);
+ gdb::optional<LONGEST> upperbound
+ = get_discrete_high_bound (range_type);
+
+ if (!upperbound.has_value ())
+ upperbound = 0;
+
+ if (index >= *lowerbound && index <= *upperbound)
+ return value_subscripted_rvalue (array, index, *lowerbound);
+
/* Emit warning unless we have an array of unknown size.
An array of unknown size has lowerbound 0 and upperbound -1. */
- if (upperbound > -1)
+ if (*upperbound > -1)
warning (_("array or string index out of range"));
/* fall doing C stuff */
c_style = true;
}
- index -= lowerbound;
+ index -= *lowerbound;
array = value_coerce_array (array);
}
--
2.29.2
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] gdb: fix value_subscript when array upper bound is not known
2020-11-23 16:21 ` [PATCH 4/4] gdb: fix value_subscript when array upper bound is not known Simon Marchi
@ 2020-12-06 7:54 ` Joel Brobecker
2020-12-07 16:06 ` Simon Marchi
2020-12-09 19:57 ` Simon Marchi
0 siblings, 2 replies; 21+ messages in thread
From: Joel Brobecker @ 2020-12-06 7:54 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi
On Mon, Nov 23, 2020 at 11:21:20AM -0500, Simon Marchi via Gdb-patches wrote:
> 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
^^
"an" -> "and"
> 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?
Good question!
In thinking about it, I think there is a bigger question. But before
asking the bigger question, why would we treat an unknown high bound
differently from an unknown high bound? I don't really have a solid
answer to that question. I almost want to say that we want to treat
both the same, which argues in favor of the approach you took in
this patch, and absent a good answer to that question, because that's
what we were already doing in the past, it's another justification
for the status quo.
Now, the bigger question is: Why are we even getting to this point?
Shouldn't the bounds simply be resolved before we do the subscripting?
Thinking about this questions brings me back to the difficulties
that AdaCore had with dealing with this kind of situation where
resolving the array bounds can lead to values whose size if ginormous,
and thus the (impossible) need to be careful to never actually fetch
the value -- something I believe we side-stepped by replacing the value
by a pointer to the array itself. It was tricky, but the question of
whether we should only subscript arrays remains open, especially when
the actual low bound after bound resolution ends up being nonzero,
because it then affects which element of the array you display.
Another smaller question is about what we should be doing if we detect
a situation when the user tries to subscript an array at an index
which is outside the array's range. I believe we should let the user
try it, but should we warn, for instance?
While those are interesting discussions, given the impact of this
regressions, and the fact that this patch is restoring the status quo,
I'm in favor of discussing the above separately from this patch.
> 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
Small reminder to remove this Change-Id.
Other than that, the change looks OK to me.
I believe, between the impact of the fix, the fact that it is
a recent regression, and the relatively straightforward nature
of your patch series, I believe it will be safe to backport to
the gdb-10-branch.
Thanks Simon!
--
Joel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] gdb: fix value_subscript when array upper bound is not known
2020-12-06 7:54 ` Joel Brobecker
@ 2020-12-07 16:06 ` Simon Marchi
2020-12-08 3:14 ` Joel Brobecker
2020-12-09 19:57 ` Simon Marchi
1 sibling, 1 reply; 21+ messages in thread
From: Simon Marchi @ 2020-12-07 16:06 UTC (permalink / raw)
To: Joel Brobecker, Simon Marchi via Gdb-patches; +Cc: Simon Marchi
On 2020-12-06 2:54 a.m., Joel Brobecker wrote:
> On Mon, Nov 23, 2020 at 11:21:20AM -0500, Simon Marchi via Gdb-patches wrote:
>> 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
> ^^
>
> "an" -> "and"
>
>> 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?
>
> Good question!
>
> In thinking about it, I think there is a bigger question. But before
> asking the bigger question, why would we treat an unknown high bound
> differently from an unknown high bound?
Do you mean "unknown low bound differently from an unknown high bound"?
I don't really have a solid
> answer to that question. I almost want to say that we want to treat
> both the same, which argues in favor of the approach you took in
> this patch, and absent a good answer to that question, because that's
> what we were already doing in the past, it's another justification
> for the status quo.
>
> Now, the bigger question is: Why are we even getting to this point?
> Shouldn't the bounds simply be resolved before we do the subscripting?
If the array is c-style, there's no concept of high bound, so I don't
think there's a point resolving the high bound then.
> Thinking about this questions brings me back to the difficulties
> that AdaCore had with dealing with this kind of situation where
> resolving the array bounds can lead to values whose size if ginormous,
> and thus the (impossible) need to be careful to never actually fetch
> the value -- something I believe we side-stepped by replacing the value
> by a pointer to the array itself. It was tricky, but the question of
> whether we should only subscript arrays remains open, especially when
> the actual low bound after bound resolution ends up being nonzero,
> because it then affects which element of the array you display.
I don't have much experience with languages others than C/C++ that have
smarter array ranges, so I don't have any meaningful opinion on this.
> Another smaller question is about what we should be doing if we detect
> a situation when the user tries to subscript an array at an index
> which is outside the array's range. I believe we should let the user
> try it, but should we warn, for instance?
Again, I've never really been a user of such a language. Intuitively,
I'd like the debugger to work as close to the source language as
possible, so I'd like it to validate the bounds in my expression. But
it might be that people who use these languages sometimes need to
side-step the language's protections.
>
> While those are interesting discussions, given the impact of this
> regressions, and the fact that this patch is restoring the status quo,
> I'm in favor of discussing the above separately from this patch.
Ok, thanks.
>
>> 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
>
> Small reminder to remove this Change-Id.
As mentioned in my reply on patch 1, I'd prefer to keep those if it
doesn't bother you too much, as they are useful for bookkeeping on my end.
>
> Other than that, the change looks OK to me.
>
> I believe, between the impact of the fix, the fact that it is
> a recent regression, and the relatively straightforward nature
> of your patch series, I believe it will be safe to backport to
> the gdb-10-branch.
Thanks for the review!
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] gdb: fix value_subscript when array upper bound is not known
2020-12-07 16:06 ` Simon Marchi
@ 2020-12-08 3:14 ` Joel Brobecker
2020-12-09 18:50 ` Simon Marchi
0 siblings, 1 reply; 21+ messages in thread
From: Joel Brobecker @ 2020-12-08 3:14 UTC (permalink / raw)
To: Simon Marchi; +Cc: Simon Marchi via Gdb-patches, Simon Marchi
[just for the sake of discussing generalities, rather than suggesting
modifications to your patch]
> > Now, the bigger question is: Why are we even getting to this point?
> > Shouldn't the bounds simply be resolved before we do the subscripting?
>
> If the array is c-style, there's no concept of high bound, so I don't think
> there's a point resolving the high bound then.
I believe you sometimes do, for intance in...
char something[5];
> Again, I've never really been a user of such a language. Intuitively, I'd
> like the debugger to work as close to the source language as possible, so
> I'd like it to validate the bounds in my expression. But it might be that
> people who use these languages sometimes need to side-step the language's
> protections.
Speaking for myself, sometimes I indeed want to go beyond.
Even in C, when we have structures looking like this (don't know if we
have any of those in our code anymore):
struct something
{
size_t n_elems;
int buf[0];
};
You'd know that you need to go beyond the first element of the array
to get to the rest of the data...
--
Joel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] gdb: fix value_subscript when array upper bound is not known
2020-12-08 3:14 ` Joel Brobecker
@ 2020-12-09 18:50 ` Simon Marchi
0 siblings, 0 replies; 21+ messages in thread
From: Simon Marchi @ 2020-12-09 18:50 UTC (permalink / raw)
To: Joel Brobecker, Simon Marchi; +Cc: Simon Marchi via Gdb-patches
On 2020-12-07 10:14 p.m., Joel Brobecker wrote:
> [just for the sake of discussing generalities, rather than suggesting
> modifications to your patch]
>
>>> Now, the bigger question is: Why are we even getting to this point?
>>> Shouldn't the bounds simply be resolved before we do the subscripting?
>>
>> If the array is c-style, there's no concept of high bound, so I don't think
>> there's a point resolving the high bound then.
>
> I believe you sometimes do, for intance in...
>
> char something[5];
Right, but the language doesn't do bound checks, so I don't think the debugger
should either. What I meant is that there's no need to resolve the upper bound
for c-style arrays, because we don't check the upper bound.
>> Again, I've never really been a user of such a language. Intuitively, I'd
>> like the debugger to work as close to the source language as possible, so
>> I'd like it to validate the bounds in my expression. But it might be that
>> people who use these languages sometimes need to side-step the language's
>> protections.
>
> Speaking for myself, sometimes I indeed want to go beyond.
>
> Even in C, when we have structures looking like this (don't know if we
> have any of those in our code anymore):
>
> struct something
> {
> size_t n_elems;
> int buf[0];
> };
>
> You'd know that you need to go beyond the first element of the array
> to get to the rest of the data...
I would imagine that with a language that is more strict and does bound-checks
when accessing arrays, this kind of trick wouldn't exist. So there probably
wouldn't be legitimate use cases for doing out-of-bounds accesses.
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] gdb: fix value_subscript when array upper bound is not known
2020-12-06 7:54 ` Joel Brobecker
2020-12-07 16:06 ` Simon Marchi
@ 2020-12-09 19:57 ` Simon Marchi
1 sibling, 0 replies; 21+ messages in thread
From: Simon Marchi @ 2020-12-09 19:57 UTC (permalink / raw)
To: Joel Brobecker, Simon Marchi via Gdb-patches; +Cc: Simon Marchi
On 2020-12-06 2:54 a.m., Joel Brobecker wrote:
> Other than that, the change looks OK to me.
>
> I believe, between the impact of the fix, the fact that it is
> a recent regression, and the relatively straightforward nature
> of your patch series, I believe it will be safe to backport to
> the gdb-10-branch.
FYI, I pushed this to the master branch, and I am in the process of
backporting it to the gdb-10-branch. There are a few conflicts, so
I'll give it a test run before pushing it there.
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread