Re: [PATCH master + gdb-10-branch] gdb: fix getting range of flexible array member in Python

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-04-22 13:19:07 -0400]:

> On 2021-04-22 12:48 p.m., Andrew Burgess wrote:
> > * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-04-22 11:59:50 -0400]:
> > 
> >> As reported in bug 27757, we get an internal error when doing:
> >>
> >>     $ cat test.c
> >>     struct foo {
> >>         int len;
> >>         int items[];
> >>     };
> >>
> >>     struct foo *p;
> >>
> >>     int main() {
> >>         return 0;
> >>     }
> >>     $ gcc test.c -g -O0 -o test
> >>     $ ./gdb -q -nx --data-directory=data-directory ./test -ex 'python gdb.parse_and_eval("p").type.target()["items"].type.range()'
> >>     Reading symbols from ./test...
> >>     /home/simark/src/binutils-gdb/gdb/gdbtypes.h:435: internal-error: LONGEST dynamic_prop::const_val() const: Assertion `m_kind == PROP_CONST' failed.
> >>     A problem internal to GDB has been detected,
> >>     further debugging may prove unreliable.
> >>     Quit this debugging session? (y or n)
> >>
> >> This is because the Python code (typy_range) blindly reads the high
> >> bound of the type of `items` as a constant value.  Since it is a
> >> flexible array member, it has no high bound, the property is undefined.
> >> Since commit 8c2e4e0689 ("gdb: add accessors to struct dynamic_prop"),
> >> the getters check that you are not getting a property value of the wrong
> >> kind, so this causes a failed assertion.
> >>
> >> Fix it by checking if the property is indeed a constant value before
> >> accessing it as such.  Otherwise, use 0.  This restores the previous GDB
> >> behavior: because the structure was zero-initialized, this is what was
> >> returned before.  But now this behavior is explicit and not accidental.
> >>
> >> Add a test, gdb.python/flexible-array-member.exp, that is derived from
> >> gdb.base/flexible-array-member.exp.  It tests the same things, but
> >> through the Python API.  It also specifically tests getting the range
> >> from the various kinds of flexible array member types (AFAIK it wasn't
> >> possible to do the equivalent through the CLI).
> >>
> >> gdb/ChangeLog:
> >>
> >> 	PR gdb/27757
> >> 	* python/py-type.c (typy_range): Check that bounds are constant
> >> 	before accessing them as such.
> >>
> >> gdb/testsuite/ChangeLog:
> >>
> >> 	PR gdb/27757
> >> 	* gdb.python/flexible-array-member.c: New test.
> >> 	* gdb.python/flexible-array-member.exp: New test.
> >>
> >> Change-Id: Ibef92ee5fd871ecb7c791db2a788f203dff2b841
> >> ---
> >>  gdb/python/py-type.c                          | 11 ++-
> >>  .../gdb.python/flexible-array-member.c        | 70 ++++++++++++++++
> >>  .../gdb.python/flexible-array-member.exp      | 81 +++++++++++++++++++
> >>  3 files changed, 160 insertions(+), 2 deletions(-)
> >>  create mode 100644 gdb/testsuite/gdb.python/flexible-array-member.c
> >>  create mode 100644 gdb/testsuite/gdb.python/flexible-array-member.exp
> >>
> >> diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
> >> index bf707078841f..148e4a6aa3a0 100644
> >> --- a/gdb/python/py-type.c
> >> +++ b/gdb/python/py-type.c
> >> @@ -592,8 +592,15 @@ typy_range (PyObject *self, PyObject *args)
> >>      case TYPE_CODE_ARRAY:
> >>      case TYPE_CODE_STRING:
> >>      case TYPE_CODE_RANGE:
> >> -      low = type->bounds ()->low.const_val ();
> >> -      high = type->bounds ()->high.const_val ();;
> > 
> > I think the same bug exists in guile/scm-type.c, even if we don't add
> > a test it is probably worth duplicating the fix there too.
> 
> Thanks, you are right.  I added a test too (although the most
> minimalistic one that would trigger the bug).  See updated patch below.
> 
> > Also I wonder if, now we have defined this behaviour, should we add
> > this to the docs?
> 
> As you can see in the Python test, we get two different results for the
> high bound (0 and -1) for cases where I think we should get the same
> result.  So before formalizing it in the doc, I think we should address
> that, make it always 0 or always -1.  I don't know if that would be an
> unacceptable backwards-incompatible breakage though.  To be clear, I
> would suggest changing the behavior in master, not gdb-10-branch.
> 
> Simon
> 
> 
> From edf11b9916c87a67fb1c6aa662c4c5eaed271445 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Thu, 22 Apr 2021 11:59:50 -0400
> Subject: [PATCH] gdb: fix getting range of flexible array member in Python
> 
> As reported in bug 27757, we get an internal error when doing:
> 
>     $ cat test.c
>     struct foo {
>         int len;
>         int items[];
>     };
> 
>     struct foo *p;
> 
>     int main() {
>         return 0;
>     }
>     $ gcc test.c -g -O0 -o test
>     $ ./gdb -q -nx --data-directory=data-directory ./test -ex 'python gdb.parse_and_eval("p").type.target()["items"].type.range()'
>     Reading symbols from ./test...
>     /home/simark/src/binutils-gdb/gdb/gdbtypes.h:435: internal-error: LONGEST dynamic_prop::const_val() const: Assertion `m_kind == PROP_CONST' failed.
>     A problem internal to GDB has been detected,
>     further debugging may prove unreliable.
>     Quit this debugging session? (y or n)
> 
> This is because the Python code (typy_range) blindly reads the high
> bound of the type of `items` as a constant value.  Since it is a
> flexible array member, it has no high bound, the property is undefined.
> Since commit 8c2e4e0689 ("gdb: add accessors to struct dynamic_prop"),
> the getters check that you are not getting a property value of the wrong
> kind, so this causes a failed assertion.
> 
> Fix it by checking if the property is indeed a constant value before
> accessing it as such.  Otherwise, use 0.  This restores the previous GDB
> behavior: because the structure was zero-initialized, this is what was
> returned before.  But now this behavior is explicit and not accidental.
> 
> Add a test, gdb.python/flexible-array-member.exp, that is derived from
> gdb.base/flexible-array-member.exp.  It tests the same things, but
> through the Python API.  It also specifically tests getting the range
> from the various kinds of flexible array member types (AFAIK it wasn't
> possible to do the equivalent through the CLI).
> 
> gdb/ChangeLog:
> 
> 	PR gdb/27757
> 	* python/py-type.c (typy_range): Check that bounds are constant
> 	before accessing them as such.
> 	* guile/scm-type.c (gdbscm_type_range): Likewise.
> 
> gdb/testsuite/ChangeLog:
> 
> 	PR gdb/27757
> 	* gdb.python/flexible-array-member.c: New test.
> 	* gdb.python/flexible-array-member.exp: New test.
> 	* gdb.guile/scm-type.exp (test_range): Add test for flexible
> 	array member.
> 	* gdb.guile/scm-type.c (struct flex_member): New.
> 	(main): Use it.

Looks good, just one minor nit...

> 
> Change-Id: Ibef92ee5fd871ecb7c791db2a788f203dff2b841
> ---
>  gdb/guile/scm-type.c                          | 11 ++-
>  gdb/python/py-type.c                          | 11 ++-
>  gdb/testsuite/gdb.guile/scm-type.c            | 12 +++
>  gdb/testsuite/gdb.guile/scm-type.exp          | 12 +++
>  .../gdb.python/flexible-array-member.c        | 70 ++++++++++++++++
>  .../gdb.python/flexible-array-member.exp      | 81 +++++++++++++++++++
>  6 files changed, 193 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.python/flexible-array-member.c
>  create mode 100644 gdb/testsuite/gdb.python/flexible-array-member.exp
> 
> diff --git a/gdb/guile/scm-type.c b/gdb/guile/scm-type.c
> index dd45d8e8b35c..11693844edcb 100644
> --- a/gdb/guile/scm-type.c
> +++ b/gdb/guile/scm-type.c
> @@ -823,8 +823,15 @@ gdbscm_type_range (SCM self)
>      case TYPE_CODE_ARRAY:
>      case TYPE_CODE_STRING:
>      case TYPE_CODE_RANGE:
> -      low = type->bounds ()->low.const_val ();
> -      high = type->bounds ()->high.const_val ();
> +      if (type->bounds ()->low.kind () == PROP_CONST)
> +	low = type->bounds ()->low.const_val ();
> +      else
> +	low = 0;
> +
> +      if (type->bounds ()->high.kind () == PROP_CONST)
> +	high = type->bounds ()->high.const_val ();
> +      else
> +	high = 0;
>        break;
>      }
>  
> diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
> index bf707078841f..148e4a6aa3a0 100644
> --- a/gdb/python/py-type.c
> +++ b/gdb/python/py-type.c
> @@ -592,8 +592,15 @@ typy_range (PyObject *self, PyObject *args)
>      case TYPE_CODE_ARRAY:
>      case TYPE_CODE_STRING:
>      case TYPE_CODE_RANGE:
> -      low = type->bounds ()->low.const_val ();
> -      high = type->bounds ()->high.const_val ();;
> +      if (type->bounds ()->low.kind () == PROP_CONST)
> +	low = type->bounds ()->low.const_val ();
> +      else
> +	low = 0;
> +
> +      if (type->bounds ()->high.kind () == PROP_CONST)
> +	high = type->bounds ()->high.const_val ();
> +      else
> +	high = 0;
>        break;
>      }
>  
> diff --git a/gdb/testsuite/gdb.guile/scm-type.c b/gdb/testsuite/gdb.guile/scm-type.c
> index 782b8fe70cf9..64f5f02efedc 100644
> --- a/gdb/testsuite/gdb.guile/scm-type.c
> +++ b/gdb/testsuite/gdb.guile/scm-type.c
> @@ -15,6 +15,8 @@
>     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 s
>  {
>    int a;
> @@ -53,6 +55,12 @@ enum E
>  struct s vec_data_1 = {1, 1};
>  struct s vec_data_2 = {1, 2};
>  
> +struct flex_member
> +{
> +  int n;
> +  int items[];
> +};
> +
>  int
>  main ()
>  {
> @@ -72,6 +80,10 @@ main ()
>    st.b = 5;
>  
>    e = v2;
> +
> +  struct flex_member *f = (struct flex_member *) malloc (100);
> +  f->items[0] = 111;
> +  f->items[1] = 222;
>    
>    return 0;      /* break to inspect struct and array.  */
>  }
> diff --git a/gdb/testsuite/gdb.guile/scm-type.exp b/gdb/testsuite/gdb.guile/scm-type.exp
> index 8778cdb36cb1..517c99f8369d 100644
> --- a/gdb/testsuite/gdb.guile/scm-type.exp
> +++ b/gdb/testsuite/gdb.guile/scm-type.exp
> @@ -261,6 +261,18 @@ proc test_range {} {
>  		"ERROR: .*: Wrong type argument in position 1 \\(expecting ranged type\\): .*" \
>  		"check range for non ranged type"
>  	}
> +
> +	with_test_prefix "on flexible array member" {
> +	    gdb_scm_test_silent_cmd "print f" "print value (f)"
> +	    gdb_scm_test_silent_cmd "guile (define f (history-ref 0))" \
> +		"get value (f) from history"
> +	    gdb_test "guile (print (type-range (field-type (type-field (value-type (value-dereference f)) \"items\"))))" \
> +		"= \\(0 0\\)"
> +	    gdb_test "guile (print (value-subscript (value-field (value-dereference f) \"items\") 0))" \
> +		"= 111"
> +	    gdb_test "guile (print (value-subscript (value-field (value-dereference f) \"items\") 1))" \
> +		"= 222"
> +	}
>      }
>  }
>  
> diff --git a/gdb/testsuite/gdb.python/flexible-array-member.c b/gdb/testsuite/gdb.python/flexible-array-member.c
> new file mode 100644
> index 000000000000..79815e2d38e0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/flexible-array-member.c
> @@ -0,0 +1,70 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2020-2021 Free Software Foundation, Inc.

In this file and the .exp the date range is 2020-2021, is this
correct?

Otherwise, looks good.

Thanks,
Andrew


> +
> +   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.python/flexible-array-member.exp b/gdb/testsuite/gdb.python/flexible-array-member.exp
> new file mode 100644
> index 000000000000..3739c9a9e5c0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/flexible-array-member.exp
> @@ -0,0 +1,81 @@
> +# Copyright 2020-2021 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 getting the range of flexible array members in Python.
> +
> +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
> +
> +gdb_test "python ns = gdb.parse_and_eval('ns').dereference()"
> +gdb_test "python zs = gdb.parse_and_eval('zs').dereference()"
> +gdb_test "python zso = gdb.parse_and_eval('zso').dereference()"
> +
> +# Print the whole structure.
> +
> +gdb_test "python print(ns)" "{n = 3, items = $hex}"
> +gdb_test "python print(zs)" "{n = 3, items = $hex}"
> +gdb_test "python print(zso)" "{items = $hex}"
> +
> +# Print all items.
> +
> +gdb_test "python print(ns\['items'\])" "$hex"
> +gdb_test "python print(ns\['items'\]\[0\])" "101"
> +gdb_test "python print(ns\['items'\]\[1\])" "102"
> +gdb_test "python print(ns\['items'\]\[2\])" "103"
> +
> +gdb_test "python print(zs\['items'\])" "$hex"
> +gdb_test "python print(zs\['items'\]\[0\])" "201"
> +gdb_test "python print(zs\['items'\]\[1\])" "202"
> +gdb_test "python print(zs\['items'\]\[2\])" "203"
> +
> +gdb_test "python print(zso\['items'\])" "$hex"
> +gdb_test "python print(zso\['items'\]\[0\])" "301"
> +gdb_test "python print(zso\['items'\]\[1\])" "302"
> +gdb_test "python print(zso\['items'\]\[2\])" "303"
> +
> +# Check taking the address of array elements (how PR 28675 was originally
> +# reported).
> +
> +gdb_test "python print(ns\['items'\] == ns\['items'\]\[0\].address)" "True"
> +gdb_test "python print(ns\['items'\]\[0\].address + 1 == ns\['items'\]\[1\].address)" "True"
> +gdb_test "python print(zs\['items'\] == zs\['items'\]\[0\].address)" "True"
> +gdb_test "python print(zs\['items'\]\[0\].address + 1 == zs\['items'\]\[1\].address)" "True"
> +gdb_test "python print(zso\['items'\] == zso\['items'\]\[0\].address)" "True"
> +gdb_test "python print(zso\['items'\]\[0\].address + 1 == zso\['items'\]\[1\].address)" "True"
> +
> +# Verify the range attribute.  It looks a bit inconsistent that the high bound
> +# is sometimes 0, sometimes -1, but that's what GDB produces today, so that's
> +# what we test.
> +
> +gdb_test "python print(ns\['items'\].type.range())" "\\(0, 0\\)"
> +gdb_test "python print(zs\['items'\].type.range())" "\\(0, -1\\)"
> +gdb_test "python print(zso\['items'\].type.range())" "\\(0, -1\\)"
> -- 
> 2.30.1
>