[PATCHv2 0/5] Improve handling of negative dynamic properties

Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed

* [PATCHv2 0/5] Improve handling of negative dynamic properties
  2019-05-05 20:57 ` [PATCH 0/3] " Andrew Burgess
@ 2019-05-09 22:22 Andrew Burgess
  2019-05-05 20:57 ` [PATCH 0/3] " Andrew Burgess
  7 siblings, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2019-05-09 22:22 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

Patches #1 and #2 are unchanged from the previous version.

Path #3 to #5 are new in this version.which aims to address Tom's
feedback on the first version.

The approach in this new series is to have dynamic properties
understand what type they should be, this means that when the dynamic
properties are resolved they will get back a correctly signed /
unsigned value without the higher level code having to know which type
to ask for.

---

Andrew Burgess (5):
  gdb: Update type of lower bound in value_subscripted_rvalue
  gdb: Convert dwarf2_evaluate_property to return bool
  gdb/dwarf: Ensure the target type of ranges is not void
  gdb: Carry default property type around with dynamic properties
  gdb: Better support for dynamic properties with negative values

 gdb/ChangeLog                            |  47 ++++++++
 gdb/dwarf2loc.c                          |  46 ++++++--
 gdb/dwarf2loc.h                          |  36 +++---
 gdb/dwarf2read.c                         | 194 ++++++++++++++++++++++---------
 gdb/gdbtypes.c                           |  10 +-
 gdb/testsuite/ChangeLog                  |  11 ++
 gdb/testsuite/gdb.fortran/vla-ptype.exp  |  12 ++
 gdb/testsuite/gdb.fortran/vla-sizeof.exp |  10 ++
 gdb/testsuite/gdb.fortran/vla-value.exp  |  27 +++++
 gdb/testsuite/gdb.fortran/vla.f90        |  15 +++
 gdb/valarith.c                           |   2 +-
 gdb/value.h                              |   3 +-
 12 files changed, 328 insertions(+), 85 deletions(-)

-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 0/3] Improve handling of negative dynamic properties
@ 2019-05-05 20:57 ` Andrew Burgess
  2019-05-05 20:57   ` [PATCH 2/3] gdb: Convert dwarf2_evaluate_property to return bool Andrew Burgess
                     ` (7 more replies)
  0 siblings, 8 replies; 20+ messages in thread
From: Andrew Burgess @ 2019-05-05 20:57 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

Patch #3 from this series is a refresh of patch #1 and #2 from this
series:

  https://sourceware.org/ml/gdb-patches/2018-11/msg00465.html

I merged the two patches as they seem closly related, otherwise my
changes were:

  + Change from usinng 'int' to 'bool' where appropriate.

  + Updated the name of the API from 'dwarf2_evaluate_property_signed'
  to 'dwarf2_evaluate_property'.

  + Added more tests.

I originally planned to work through the whole of the original series,
however some of the patches need more work so I thought I'd push the
parts as I managed to prepare them.

Thanks,
Andrew

---

Andrew Burgess (3):
  gdb: Update type of lower bound in value_subscripted_rvalue
  gdb: Convert dwarf2_evaluate_property to return bool
  gdb: Handle dynamic properties with negative values

 gdb/ChangeLog                            | 27 +++++++++++++++++++++++++
 gdb/dwarf2loc.c                          | 34 ++++++++++++++++++++++++--------
 gdb/dwarf2loc.h                          | 19 +++++++++++-------
 gdb/findvar.c                            |  2 +-
 gdb/gdbtypes.c                           | 15 ++++++--------
 gdb/testsuite/ChangeLog                  | 11 +++++++++++
 gdb/testsuite/gdb.fortran/vla-ptype.exp  | 12 +++++++++++
 gdb/testsuite/gdb.fortran/vla-sizeof.exp | 10 ++++++++++
 gdb/testsuite/gdb.fortran/vla-value.exp  | 27 +++++++++++++++++++++++++
 gdb/testsuite/gdb.fortran/vla.f90        | 15 ++++++++++++++
 gdb/valarith.c                           |  2 +-
 gdb/value.h                              |  3 ++-
 12 files changed, 150 insertions(+), 27 deletions(-)

-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 2/3] gdb: Convert dwarf2_evaluate_property to return bool
  2019-05-05 20:57 ` [PATCH 0/3] " Andrew Burgess
@ 2019-05-05 20:57   ` Andrew Burgess
  2019-05-06 13:57     ` Tom Tromey
  2019-05-05 20:57   ` [PATCH 3/3] gdb: Handle dynamic properties with negative values Andrew Burgess
                     ` (6 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2019-05-05 20:57 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

Convert dwarf2_evaluate_property to return a bool, there should be no
user visible change after this commit.

gdb/ChangeLog:

	* dwarf2loc.c (dwarf2_evaluate_property): Change return type, and
	update return statements.
	* dwarf2loc.h (dwarf2_evaluate_property): Update return type on
	declaration, and update comment to match.
	* gdbtypes.c (resolve_dynamic_array): Update call to
	dwarf2_evaluate_property to match new return type.
---
 gdb/ChangeLog   |  9 +++++++++
 gdb/dwarf2loc.c | 14 +++++++-------
 gdb/dwarf2loc.h | 12 ++++++------
 gdb/gdbtypes.c  |  5 +----
 4 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index bd630ee0588..411bf2912b2 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -2424,14 +2424,14 @@ dwarf2_locexpr_baton_eval (const struct dwarf2_locexpr_baton *dlbaton,
 
 /* See dwarf2loc.h.  */
 
-int
+bool
 dwarf2_evaluate_property (const struct dynamic_prop *prop,
 			  struct frame_info *frame,
 			  struct property_addr_info *addr_stack,
 			  CORE_ADDR *value)
 {
   if (prop == NULL)
-    return 0;
+    return false;
 
   if (frame == NULL && has_stack_frames ())
     frame = get_selected_frame (NULL);
@@ -2453,7 +2453,7 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 
 		*value = value_as_address (val);
 	      }
-	    return 1;
+	    return true;
 	  }
       }
       break;
@@ -2475,7 +2475,7 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 	    if (!value_optimized_out (val))
 	      {
 		*value = value_as_address (val);
-		return 1;
+		return true;
 	      }
 	  }
       }
@@ -2483,7 +2483,7 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 
     case PROP_CONST:
       *value = prop->data.const_val;
-      return 1;
+      return true;
 
     case PROP_ADDR_OFFSET:
       {
@@ -2505,11 +2505,11 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 	  val = value_at (baton->offset_info.type,
 			  pinfo->addr + baton->offset_info.offset);
 	*value = value_as_address (val);
-	return 1;
+	return true;
       }
     }
 
-  return 0;
+  return false;
 }
 
 /* See dwarf2loc.h.  */
diff --git a/gdb/dwarf2loc.h b/gdb/dwarf2loc.h
index 955e6f1b48a..ac1a771a9f3 100644
--- a/gdb/dwarf2loc.h
+++ b/gdb/dwarf2loc.h
@@ -135,13 +135,13 @@ struct property_addr_info
    property. When evaluating a property that is not related to a type, it can
    be NULL.
 
-   Returns 1 if PROP could be converted and the static value is passed back
-   into VALUE, otherwise returns 0.  */
+   Returns true if PROP could be converted and the static value is passed
+   back into VALUE, otherwise returns false.  */
 
-int dwarf2_evaluate_property (const struct dynamic_prop *prop,
-			      struct frame_info *frame,
-			      struct property_addr_info *addr_stack,
-			      CORE_ADDR *value);
+bool dwarf2_evaluate_property (const struct dynamic_prop *prop,
+			       struct frame_info *frame,
+			       struct property_addr_info *addr_stack,
+			       CORE_ADDR *value);
 
 /* A helper for the compiler interface that compiles a single dynamic
    property to C code.
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index b3424d81be4..fe52a64f212 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -2065,10 +2065,7 @@ resolve_dynamic_array (struct type *type,
   prop = get_dyn_prop (DYN_PROP_BYTE_STRIDE, type);
   if (prop != NULL)
     {
-      int prop_eval_ok
-	= dwarf2_evaluate_property (prop, NULL, addr_stack, &value);
-
-      if (prop_eval_ok)
+      if (dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
 	{
 	  remove_dyn_prop (DYN_PROP_BYTE_STRIDE, type);
 	  bit_stride = (unsigned int) (value * 8);
-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] gdb: Convert dwarf2_evaluate_property to return bool
  2019-05-05 20:57   ` [PATCH 2/3] gdb: Convert dwarf2_evaluate_property to return bool Andrew Burgess
@ 2019-05-06 13:57     ` Tom Tromey
  0 siblings, 0 replies; 20+ messages in thread
From: Tom Tromey @ 2019-05-06 13:57 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Convert dwarf2_evaluate_property to return a bool, there should be no
Andrew> user visible change after this commit.

Andrew> gdb/ChangeLog:

Andrew> 	* dwarf2loc.c (dwarf2_evaluate_property): Change return type, and
Andrew> 	update return statements.
Andrew> 	* dwarf2loc.h (dwarf2_evaluate_property): Update return type on
Andrew> 	declaration, and update comment to match.
Andrew> 	* gdbtypes.c (resolve_dynamic_array): Update call to
Andrew> 	dwarf2_evaluate_property to match new return type.

Thanks for the patch.  This is ok.

Tom

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 3/3] gdb: Handle dynamic properties with negative values
  2019-05-05 20:57 ` [PATCH 0/3] " Andrew Burgess
  2019-05-05 20:57   ` [PATCH 2/3] gdb: Convert dwarf2_evaluate_property to return bool Andrew Burgess
@ 2019-05-05 20:57   ` Andrew Burgess
  2019-05-06 14:55     ` Tom Tromey
  2019-05-05 20:57   ` [PATCH 1/3] gdb: Update type of lower bound in value_subscripted_rvalue Andrew Burgess
                     ` (5 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2019-05-05 20:57 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

When running a 32-bit inferior on a 64-bit platform, fetching dynamic
properties that are signed is not correctly sign extending the fetched
value from 32 to 64 bits.  As a result, for example, a Fortran array
with negative array bounds will appear to GDB to have large positive
array bounds.

gdb/Changelog:

	* dwarf2loc.h (dwarf2_evaluate_property): Add signed_p parameter.
	* dwarf2loc.c (dwarf2_evaluate_property): Add signed_p parameter,
	if this is true then sign extend the value before returning.
	* findvar.c (follow_static_link): Update call to
	dwarf2_evaluate_property.
	* gdbtypes.c (resolve_dynamic_range): Likewise.
	(resolve_dynamic_array): Likewise.
	(resolve_dynamic_type_internal): Likewise.

gdb/testsuite/ChangeLog:

	* gdb.fortran/vla-ptype.exp: Print array with negative bounds.
	* gdb.fortran/vla-sizeof.exp: Print the size of an array with
	negative bounds.
	* gdb.fortran/vla-value.exp: Print elements of an array with
	negative bounds.
	* gdb.fortran/vla.f90: Setup an array with negative bounds for
	testing.
---
 gdb/ChangeLog                            | 12 ++++++++++++
 gdb/dwarf2loc.c                          | 30 ++++++++++++++++++++++++------
 gdb/dwarf2loc.h                          |  7 ++++++-
 gdb/findvar.c                            |  2 +-
 gdb/gdbtypes.c                           | 12 ++++++------
 gdb/testsuite/ChangeLog                  | 11 +++++++++++
 gdb/testsuite/gdb.fortran/vla-ptype.exp  | 12 ++++++++++++
 gdb/testsuite/gdb.fortran/vla-sizeof.exp | 10 ++++++++++
 gdb/testsuite/gdb.fortran/vla-value.exp  | 27 +++++++++++++++++++++++++++
 gdb/testsuite/gdb.fortran/vla.f90        | 15 +++++++++++++++
 10 files changed, 124 insertions(+), 14 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 411bf2912b2..ee229ae6de3 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -2428,8 +2428,10 @@ bool
 dwarf2_evaluate_property (const struct dynamic_prop *prop,
 			  struct frame_info *frame,
 			  struct property_addr_info *addr_stack,
-			  CORE_ADDR *value)
+			  CORE_ADDR *value, bool signed_p)
 {
+  bool converted_p = false;
+
   if (prop == NULL)
     return false;
 
@@ -2453,7 +2455,7 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 
 		*value = value_as_address (val);
 	      }
-	    return true;
+	    converted_p = true;
 	  }
       }
       break;
@@ -2475,7 +2477,7 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 	    if (!value_optimized_out (val))
 	      {
 		*value = value_as_address (val);
-		return true;
+		converted_p = true;
 	      }
 	  }
       }
@@ -2483,7 +2485,8 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 
     case PROP_CONST:
       *value = prop->data.const_val;
-      return true;
+      converted_p = true;
+      break;
 
     case PROP_ADDR_OFFSET:
       {
@@ -2505,11 +2508,26 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 	  val = value_at (baton->offset_info.type,
 			  pinfo->addr + baton->offset_info.offset);
 	*value = value_as_address (val);
-	return true;
+	converted_p = true;
       }
+      break;
     }
 
-  return false;
+  if (converted_p && signed_p)
+    {
+      /* If we have a valid return candidate and it's value is signed,
+         we have to sign-extend the value because CORE_ADDR on 64bit
+         machine has 8 bytes but address size of an 32bit application
+	 is 4 bytes.  */
+      struct gdbarch * gdbarch = target_gdbarch ();
+      const int addr_bit = gdbarch_addr_bit (gdbarch);
+      const CORE_ADDR neg_mask = ((~0) <<  (addr_bit - 1));
+
+      /* Check if signed bit is set and sign-extend values.  */
+      if (*value & (neg_mask))
+	*value |= (neg_mask );
+    }
+  return converted_p;
 }
 
 /* See dwarf2loc.h.  */
diff --git a/gdb/dwarf2loc.h b/gdb/dwarf2loc.h
index ac1a771a9f3..7ebca31efc0 100644
--- a/gdb/dwarf2loc.h
+++ b/gdb/dwarf2loc.h
@@ -135,13 +135,18 @@ struct property_addr_info
    property. When evaluating a property that is not related to a type, it can
    be NULL.
 
+   SIGNED_P should be true if the property should be treated as a signed
+   value, and sign extended to the size of VALUE, or false to treat the
+   property as unsigned (for example, for address properties).
+
    Returns true if PROP could be converted and the static value is passed
    back into VALUE, otherwise returns false.  */
 
 bool dwarf2_evaluate_property (const struct dynamic_prop *prop,
 			       struct frame_info *frame,
 			       struct property_addr_info *addr_stack,
-			       CORE_ADDR *value);
+			       CORE_ADDR *value,
+			       bool signed_p);
 
 /* A helper for the compiler interface that compiles a single dynamic
    property to C code.
diff --git a/gdb/findvar.c b/gdb/findvar.c
index e89ee37ffc7..cefd0d0f2e3 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -433,7 +433,7 @@ follow_static_link (struct frame_info *frame,
 {
   CORE_ADDR upper_frame_base;
 
-  if (!dwarf2_evaluate_property (static_link, frame, NULL, &upper_frame_base))
+  if (!dwarf2_evaluate_property (static_link, frame, NULL, &upper_frame_base, false))
     return NULL;
 
   /* Now climb up the stack frame until we reach the frame we are interested
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index fe52a64f212..479d30d05d3 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1980,7 +1980,7 @@ resolve_dynamic_range (struct type *dyn_range_type,
   gdb_assert (TYPE_CODE (dyn_range_type) == TYPE_CODE_RANGE);
 
   prop = &TYPE_RANGE_DATA (dyn_range_type)->low;
-  if (dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
+  if (dwarf2_evaluate_property (prop, NULL, addr_stack, &value, true))
     {
       low_bound.kind = PROP_CONST;
       low_bound.data.const_val = value;
@@ -1992,7 +1992,7 @@ resolve_dynamic_range (struct type *dyn_range_type,
     }
 
   prop = &TYPE_RANGE_DATA (dyn_range_type)->high;
-  if (dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
+  if (dwarf2_evaluate_property (prop, NULL, addr_stack, &value, true))
     {
       high_bound.kind = PROP_CONST;
       high_bound.data.const_val = value;
@@ -2043,13 +2043,13 @@ resolve_dynamic_array (struct type *type,
   /* Resolve allocated/associated here before creating a new array type, which
      will update the length of the array accordingly.  */
   prop = TYPE_ALLOCATED_PROP (type);
-  if (prop != NULL && dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
+  if (prop != NULL && dwarf2_evaluate_property (prop, NULL, addr_stack, &value, false))
     {
       TYPE_DYN_PROP_ADDR (prop) = value;
       TYPE_DYN_PROP_KIND (prop) = PROP_CONST;
     }
   prop = TYPE_ASSOCIATED_PROP (type);
-  if (prop != NULL && dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
+  if (prop != NULL && dwarf2_evaluate_property (prop, NULL, addr_stack, &value, false))
     {
       TYPE_DYN_PROP_ADDR (prop) = value;
       TYPE_DYN_PROP_KIND (prop) = PROP_CONST;
@@ -2065,7 +2065,7 @@ resolve_dynamic_array (struct type *type,
   prop = get_dyn_prop (DYN_PROP_BYTE_STRIDE, type);
   if (prop != NULL)
     {
-      if (dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
+      if (dwarf2_evaluate_property (prop, NULL, addr_stack, &value, true))
 	{
 	  remove_dyn_prop (DYN_PROP_BYTE_STRIDE, type);
 	  bit_stride = (unsigned int) (value * 8);
@@ -2282,7 +2282,7 @@ resolve_dynamic_type_internal (struct type *type,
   /* Resolve data_location attribute.  */
   prop = TYPE_DATA_LOCATION (resolved_type);
   if (prop != NULL
-      && dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
+      && dwarf2_evaluate_property (prop, NULL, addr_stack, &value, false))
     {
       TYPE_DYN_PROP_ADDR (prop) = value;
       TYPE_DYN_PROP_KIND (prop) = PROP_CONST;
diff --git a/gdb/testsuite/gdb.fortran/vla-ptype.exp b/gdb/testsuite/gdb.fortran/vla-ptype.exp
index 0f4abb63757..7ad7ecdea65 100644
--- a/gdb/testsuite/gdb.fortran/vla-ptype.exp
+++ b/gdb/testsuite/gdb.fortran/vla-ptype.exp
@@ -98,3 +98,15 @@ gdb_test "ptype vla2" "type = <not allocated>" "ptype vla2 not allocated"
 gdb_test "ptype vla2(5, 45, 20)" \
   "no such vector element \\\(vector not allocated\\\)" \
   "ptype vla2(5, 45, 20) not allocated"
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v1"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v1"
+gdb_test "ptype vla1" \
+    "type = $real, allocatable \\(-2:-1,-5:-2,-3:-1\\)" \
+    "ptype vla1 negative bounds"
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v2"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v2"
+gdb_test "ptype vla1" \
+    "type = $real, allocatable \\(-2:1,-5:2,-3:1\\)" \
+    "ptype vla1 negative lower bounds, positive upper bounds"
diff --git a/gdb/testsuite/gdb.fortran/vla-sizeof.exp b/gdb/testsuite/gdb.fortran/vla-sizeof.exp
index 7f74a699d76..7527e0eb0b8 100644
--- a/gdb/testsuite/gdb.fortran/vla-sizeof.exp
+++ b/gdb/testsuite/gdb.fortran/vla-sizeof.exp
@@ -44,3 +44,13 @@ gdb_test "print sizeof(pvla)" " = 0" "print sizeof non-associated pvla"
 gdb_breakpoint [gdb_get_line_number "pvla-associated"]
 gdb_continue_to_breakpoint "pvla-associated"
 gdb_test "print sizeof(pvla)" " = 4000" "print sizeof associated pvla"
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v1"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v1"
+gdb_test "print sizeof(vla1)" " = 96" \
+    "print sizeof vla1 negative bounds"
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v2"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v2"
+gdb_test "print sizeof(vla1)" " = 640" \
+    "print sizeof vla1 negative lower bounds, positive upper bounds"
diff --git a/gdb/testsuite/gdb.fortran/vla-value.exp b/gdb/testsuite/gdb.fortran/vla-value.exp
index be397fd95fb..3145d21c15c 100644
--- a/gdb/testsuite/gdb.fortran/vla-value.exp
+++ b/gdb/testsuite/gdb.fortran/vla-value.exp
@@ -161,3 +161,30 @@ gdb_breakpoint [gdb_get_line_number "pvla-deassociated"]
 gdb_continue_to_breakpoint "pvla-deassociated"
 gdb_test "print \$mypvar(1,3,8)" " = 1001" \
   "print \$mypvar(1,3,8) after deallocated"
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v1"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v1"
+with_test_prefix "negative bounds" {
+    gdb_test "print vla1(-2,-5,-3)" " = 1"
+    gdb_test "print vla1(-2,-3,-1)" " = -231"
+    gdb_test "print vla1(-3,-5,-3)" "no such vector element"
+    gdb_test "print vla1(-2,-6,-3)" "no such vector element"
+    gdb_test "print vla1(-2,-5,-4)" "no such vector element"
+    gdb_test "print vla1(0,-2,-1)" "no such vector element"
+    gdb_test "print vla1(-1,-1,-1)" "no such vector element"
+    gdb_test "print vla1(-1,-2,0)" "no such vector element"
+}
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v2"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v2"
+with_test_prefix "negative lower bounds, positive upper bounds" {
+    gdb_test "print vla1(-2,-5,-3)" " = 2"
+    gdb_test "print vla1(-2,-3,-1)" " = 2"
+    gdb_test "print vla1(-2,-4,-2)" " = -242"
+    gdb_test "print vla1(-3,-5,-3)" "no such vector element"
+    gdb_test "print vla1(-2,-6,-3)" "no such vector element"
+    gdb_test "print vla1(-2,-5,-4)" "no such vector element"
+    gdb_test "print vla1(2,2,1)" "no such vector element"
+    gdb_test "print vla1(1,3,1)" "no such vector element"
+    gdb_test "print vla1(1,2,2)" "no such vector element"
+}
diff --git a/gdb/testsuite/gdb.fortran/vla.f90 b/gdb/testsuite/gdb.fortran/vla.f90
index 5bc608744b3..0ccb5c90d93 100644
--- a/gdb/testsuite/gdb.fortran/vla.f90
+++ b/gdb/testsuite/gdb.fortran/vla.f90
@@ -54,4 +54,19 @@ program vla
 
   allocate (vla3 (2,2))               ! vla2-deallocated
   vla3(:,:) = 13
+
+  allocate (vla1 (-2:-1, -5:-2, -3:-1))
+  vla1(:, :, :) = 1
+  vla1(-2, -3, -1) = -231
+
+  deallocate (vla1)                   ! vla1-neg-bounds-v1
+  l = allocated(vla1)
+
+  allocate (vla1 (-2:1, -5:2, -3:1))
+  vla1(:, :, :) = 2
+  vla1(-2, -4, -2) = -242
+
+  deallocate (vla1)                   ! vla1-neg-bounds-v2
+  l = allocated(vla1)
+
 end program vla
-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] gdb: Handle dynamic properties with negative values
  2019-05-05 20:57   ` [PATCH 3/3] gdb: Handle dynamic properties with negative values Andrew Burgess
@ 2019-05-06 14:55     ` Tom Tromey
  0 siblings, 0 replies; 20+ messages in thread
From: Tom Tromey @ 2019-05-06 14:55 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> 	* dwarf2loc.h (dwarf2_evaluate_property): Add signed_p parameter.
Andrew> 	* dwarf2loc.c (dwarf2_evaluate_property): Add signed_p parameter,

Shouldn't the signed-ness of the array bounds come from the DWARF?
It seems fine to me for an array to have unsigned bounds, but this
approach, where the caller decides the signed-ness, precludes this.

Tom

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/3] gdb: Update type of lower bound in value_subscripted_rvalue
  2019-05-05 20:57 ` [PATCH 0/3] " Andrew Burgess
  2019-05-05 20:57   ` [PATCH 2/3] gdb: Convert dwarf2_evaluate_property to return bool Andrew Burgess
  2019-05-05 20:57   ` [PATCH 3/3] gdb: Handle dynamic properties with negative values Andrew Burgess
@ 2019-05-05 20:57   ` Andrew Burgess
  2019-05-06 13:57     ` Tom Tromey
  2019-05-09 22:22   ` [PATCHv2 5/5] gdb: Better support for dynamic properties with negative values Andrew Burgess
                     ` (4 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2019-05-05 20:57 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

The dynamic lower (and upper) bounds of ranges are stored as type
LONGEST (see union dynamic_prop_data in gdbtypes.h).  In most places
that range bounds are handled they are held in a LONGEST, however in
value_subscripted_rvalue the bound is placed into an int.

This commit changes value_subscripted_rvalue to use LONGEST, there
should be no user visible changes after this commit.

gdb/ChangeLog:

	* valarith.c (value_subscripted_rvalue): Change lowerbound
	parameter type from int to LONGEST.
	* value.h (value_subscripted_rvalue): Likewise in declaration.
---
 gdb/ChangeLog  | 6 ++++++
 gdb/valarith.c | 2 +-
 gdb/value.h    | 3 ++-
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/gdb/valarith.c b/gdb/valarith.c
index 8d310b504a2..f4372123e67 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -182,7 +182,7 @@ value_subscript (struct value *array, LONGEST index)
    to doubles, but no longer does.  */
 
 struct value *
-value_subscripted_rvalue (struct value *array, LONGEST index, int lowerbound)
+value_subscripted_rvalue (struct value *array, LONGEST index, LONGEST lowerbound)
 {
   struct type *array_type = check_typedef (value_type (array));
   struct type *elt_type = check_typedef (TYPE_TARGET_TYPE (array_type));
diff --git a/gdb/value.h b/gdb/value.h
index 0756d13b6d7..d505dc9af06 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -1149,7 +1149,8 @@ extern struct value *find_function_in_inferior (const char *,
 extern struct value *value_allocate_space_in_inferior (int);
 
 extern struct value *value_subscripted_rvalue (struct value *array,
-					       LONGEST index, int lowerbound);
+					       LONGEST index,
+					       LONGEST lowerbound);
 
 /* User function handler.  */
 
-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/3] gdb: Update type of lower bound in value_subscripted_rvalue
  2019-05-05 20:57   ` [PATCH 1/3] gdb: Update type of lower bound in value_subscripted_rvalue Andrew Burgess
@ 2019-05-06 13:57     ` Tom Tromey
  0 siblings, 0 replies; 20+ messages in thread
From: Tom Tromey @ 2019-05-06 13:57 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> The dynamic lower (and upper) bounds of ranges are stored as type
Andrew> LONGEST (see union dynamic_prop_data in gdbtypes.h).  In most places
Andrew> that range bounds are handled they are held in a LONGEST, however in
Andrew> value_subscripted_rvalue the bound is placed into an int.

Andrew> This commit changes value_subscripted_rvalue to use LONGEST, there
Andrew> should be no user visible changes after this commit.

Andrew> gdb/ChangeLog:

Andrew> 	* valarith.c (value_subscripted_rvalue): Change lowerbound
Andrew> 	parameter type from int to LONGEST.
Andrew> 	* value.h (value_subscripted_rvalue): Likewise in declaration.

Thanks, this looks good to me.

Tom

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCHv2 5/5] gdb: Better support for dynamic properties with negative values
  2019-05-05 20:57 ` [PATCH 0/3] " Andrew Burgess
                     ` (2 preceding siblings ...)
  2019-05-05 20:57   ` [PATCH 1/3] gdb: Update type of lower bound in value_subscripted_rvalue Andrew Burgess
@ 2019-05-09 22:22   ` Andrew Burgess
  2019-05-22 19:37     ` Tom Tromey
  2019-05-09 22:22   ` [PATCHv2 4/5] gdb: Carry default property type around with dynamic properties Andrew Burgess
                     ` (3 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2019-05-09 22:22 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

When the type of a property is smaller than the CORE_ADDR in which the
property value has been placed, and if the property is signed, then
sign extend the property value from its actual type up to the size of
CORE_ADDR.

gdb/ChangeLog:

	* dwarf2loc.c (dwarf2_evaluate_property): Sign extend property
	value if its desired type is smaller than a CORE_ADDR and signed.

gdb/testsuite/ChangeLog:

	* gdb.fortran/vla-ptype.exp: Print array with negative bounds.
	* gdb.fortran/vla-sizeof.exp: Print the size of an array with
	negative bounds.
	* gdb.fortran/vla-value.exp: Print elements of an array with
	negative bounds.
	* gdb.fortran/vla.f90: Setup an array with negative bounds for
	testing.
---
 gdb/ChangeLog                            |  5 +++++
 gdb/dwarf2loc.c                          | 22 ++++++++++++++++++++++
 gdb/testsuite/ChangeLog                  | 11 +++++++++++
 gdb/testsuite/gdb.fortran/vla-ptype.exp  | 12 ++++++++++++
 gdb/testsuite/gdb.fortran/vla-sizeof.exp | 10 ++++++++++
 gdb/testsuite/gdb.fortran/vla-value.exp  | 27 +++++++++++++++++++++++++++
 gdb/testsuite/gdb.fortran/vla.f90        | 15 +++++++++++++++
 7 files changed, 102 insertions(+)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 88d34eb8660..0a25a7a64b0 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -2454,6 +2454,28 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 		struct value *val = value_at (baton->property_type, *value);
 		*value = value_as_address (val);
 	      }
+	    else
+	      {
+		gdb_assert (baton->property_type != NULL);
+
+		struct type *type = check_typedef (baton->property_type);
+		if (TYPE_LENGTH (type) < sizeof (CORE_ADDR)
+		    && !TYPE_UNSIGNED (type))
+		  {
+		    /* If we have a valid return candidate and it's value
+		       is signed, we have to sign-extend the value because
+		       CORE_ADDR on 64bit machine has 8 bytes but address
+		       size of an 32bit application is bytes.  */
+		    const int addr_size
+		      = (dwarf2_per_cu_addr_size (baton->locexpr.per_cu)
+			 * TARGET_CHAR_BIT);
+		    const CORE_ADDR neg_mask = ((~0) <<  (addr_size - 1));
+
+		    /* Check if signed bit is set and sign-extend values.  */
+		    if (*value & (neg_mask))
+		      *value |= (neg_mask );
+		  }
+	      }
 	    return true;
 	  }
       }
diff --git a/gdb/testsuite/gdb.fortran/vla-ptype.exp b/gdb/testsuite/gdb.fortran/vla-ptype.exp
index 0f4abb63757..7ad7ecdea65 100644
--- a/gdb/testsuite/gdb.fortran/vla-ptype.exp
+++ b/gdb/testsuite/gdb.fortran/vla-ptype.exp
@@ -98,3 +98,15 @@ gdb_test "ptype vla2" "type = <not allocated>" "ptype vla2 not allocated"
 gdb_test "ptype vla2(5, 45, 20)" \
   "no such vector element \\\(vector not allocated\\\)" \
   "ptype vla2(5, 45, 20) not allocated"
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v1"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v1"
+gdb_test "ptype vla1" \
+    "type = $real, allocatable \\(-2:-1,-5:-2,-3:-1\\)" \
+    "ptype vla1 negative bounds"
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v2"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v2"
+gdb_test "ptype vla1" \
+    "type = $real, allocatable \\(-2:1,-5:2,-3:1\\)" \
+    "ptype vla1 negative lower bounds, positive upper bounds"
diff --git a/gdb/testsuite/gdb.fortran/vla-sizeof.exp b/gdb/testsuite/gdb.fortran/vla-sizeof.exp
index 7f74a699d76..7527e0eb0b8 100644
--- a/gdb/testsuite/gdb.fortran/vla-sizeof.exp
+++ b/gdb/testsuite/gdb.fortran/vla-sizeof.exp
@@ -44,3 +44,13 @@ gdb_test "print sizeof(pvla)" " = 0" "print sizeof non-associated pvla"
 gdb_breakpoint [gdb_get_line_number "pvla-associated"]
 gdb_continue_to_breakpoint "pvla-associated"
 gdb_test "print sizeof(pvla)" " = 4000" "print sizeof associated pvla"
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v1"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v1"
+gdb_test "print sizeof(vla1)" " = 96" \
+    "print sizeof vla1 negative bounds"
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v2"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v2"
+gdb_test "print sizeof(vla1)" " = 640" \
+    "print sizeof vla1 negative lower bounds, positive upper bounds"
diff --git a/gdb/testsuite/gdb.fortran/vla-value.exp b/gdb/testsuite/gdb.fortran/vla-value.exp
index be397fd95fb..3145d21c15c 100644
--- a/gdb/testsuite/gdb.fortran/vla-value.exp
+++ b/gdb/testsuite/gdb.fortran/vla-value.exp
@@ -161,3 +161,30 @@ gdb_breakpoint [gdb_get_line_number "pvla-deassociated"]
 gdb_continue_to_breakpoint "pvla-deassociated"
 gdb_test "print \$mypvar(1,3,8)" " = 1001" \
   "print \$mypvar(1,3,8) after deallocated"
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v1"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v1"
+with_test_prefix "negative bounds" {
+    gdb_test "print vla1(-2,-5,-3)" " = 1"
+    gdb_test "print vla1(-2,-3,-1)" " = -231"
+    gdb_test "print vla1(-3,-5,-3)" "no such vector element"
+    gdb_test "print vla1(-2,-6,-3)" "no such vector element"
+    gdb_test "print vla1(-2,-5,-4)" "no such vector element"
+    gdb_test "print vla1(0,-2,-1)" "no such vector element"
+    gdb_test "print vla1(-1,-1,-1)" "no such vector element"
+    gdb_test "print vla1(-1,-2,0)" "no such vector element"
+}
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v2"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v2"
+with_test_prefix "negative lower bounds, positive upper bounds" {
+    gdb_test "print vla1(-2,-5,-3)" " = 2"
+    gdb_test "print vla1(-2,-3,-1)" " = 2"
+    gdb_test "print vla1(-2,-4,-2)" " = -242"
+    gdb_test "print vla1(-3,-5,-3)" "no such vector element"
+    gdb_test "print vla1(-2,-6,-3)" "no such vector element"
+    gdb_test "print vla1(-2,-5,-4)" "no such vector element"
+    gdb_test "print vla1(2,2,1)" "no such vector element"
+    gdb_test "print vla1(1,3,1)" "no such vector element"
+    gdb_test "print vla1(1,2,2)" "no such vector element"
+}
diff --git a/gdb/testsuite/gdb.fortran/vla.f90 b/gdb/testsuite/gdb.fortran/vla.f90
index 5bc608744b3..0ccb5c90d93 100644
--- a/gdb/testsuite/gdb.fortran/vla.f90
+++ b/gdb/testsuite/gdb.fortran/vla.f90
@@ -54,4 +54,19 @@ program vla
 
   allocate (vla3 (2,2))               ! vla2-deallocated
   vla3(:,:) = 13
+
+  allocate (vla1 (-2:-1, -5:-2, -3:-1))
+  vla1(:, :, :) = 1
+  vla1(-2, -3, -1) = -231
+
+  deallocate (vla1)                   ! vla1-neg-bounds-v1
+  l = allocated(vla1)
+
+  allocate (vla1 (-2:1, -5:2, -3:1))
+  vla1(:, :, :) = 2
+  vla1(-2, -4, -2) = -242
+
+  deallocate (vla1)                   ! vla1-neg-bounds-v2
+  l = allocated(vla1)
+
 end program vla
-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCHv2 5/5] gdb: Better support for dynamic properties with negative values
  2019-05-09 22:22   ` [PATCHv2 5/5] gdb: Better support for dynamic properties with negative values Andrew Burgess
@ 2019-05-22 19:37     ` Tom Tromey
  2019-06-10 22:17       ` Andrew Burgess
  0 siblings, 1 reply; 20+ messages in thread
From: Tom Tromey @ 2019-05-22 19:37 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> When the type of a property is smaller than the CORE_ADDR in which the
Andrew> property value has been placed, and if the property is signed, then
Andrew> sign extend the property value from its actual type up to the size of
Andrew> CORE_ADDR.

I wonder whether this should be using ULONGEST rather than CORE_ADDR
now.

Andrew> +		    /* If we have a valid return candidate and it's value
Andrew> +		       is signed, we have to sign-extend the value because
Andrew> +		       CORE_ADDR on 64bit machine has 8 bytes but address
Andrew> +		       size of an 32bit application is bytes.  */
Andrew> +		    const int addr_size
Andrew> +		      = (dwarf2_per_cu_addr_size (baton->locexpr.per_cu)
Andrew> +			 * TARGET_CHAR_BIT);

I'm somewhat surprised there isn't an existing sign_extend function
somewhere.

Andrew> +		    const CORE_ADDR neg_mask = ((~0) <<  (addr_size - 1));

I tend to think this should say ~(CORE_ADDR) 0 rather than just ~0.
Otherwise won't this do the wrong thing if sizeof(int) < sizeof(CORE_ADDR)?

Andrew> +		    /* Check if signed bit is set and sign-extend values.  */
Andrew> +		    if (*value & (neg_mask))
Andrew> +		      *value |= (neg_mask );

Extra parens and an extra space here.

mips-tdep.c has the fun sign extension idiom:

	    value = inst & 0x7ff;
	    value = (value ^ 0x400) - 0x400;		/* Sign-extend.  */

(If the high bits are 0 you can skip the "&".)
This one avoids the ~0 business.

Another approach is to let the compiler handle it.

   LONGEST l = *value;
   l = (l << nbits) >> nbits;

However this may be relying on undefined behavior.

Yours is fine, though, I just noticed these while digging around and
wanted to point them out :-)

Tom

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCHv2 5/5] gdb: Better support for dynamic properties with negative values
  2019-05-22 19:37     ` Tom Tromey
@ 2019-06-10 22:17       ` Andrew Burgess
  2019-07-10 15:03         ` Tom Tromey
  0 siblings, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2019-06-10 22:17 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

* Tom Tromey <tom@tromey.com> [2019-05-22 15:19:01 -0400]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> When the type of a property is smaller than the CORE_ADDR in which the
> Andrew> property value has been placed, and if the property is signed, then
> Andrew> sign extend the property value from its actual type up to the size of
> Andrew> CORE_ADDR.
> 
> I wonder whether this should be using ULONGEST rather than CORE_ADDR
> now.

I don't think you're wrong, but I'm hoping I can convince you to let
me punt this change for now, it would be fairly big change to the
dwarf property fetching code.  What we have seems to hang together for
now, so I'd prefer to get this change in first.

But I guess if you think changing to ULONGEST should come first then I
make that change a higher priority...

> 
> Andrew> +		    /* If we have a valid return candidate and it's value
> Andrew> +		       is signed, we have to sign-extend the value because
> Andrew> +		       CORE_ADDR on 64bit machine has 8 bytes but address
> Andrew> +		       size of an 32bit application is bytes.  */
> Andrew> +		    const int addr_size
> Andrew> +		      = (dwarf2_per_cu_addr_size (baton->locexpr.per_cu)
> Andrew> +			 * TARGET_CHAR_BIT);
> 
> I'm somewhat surprised there isn't an existing sign_extend function
> somewhere.
> 
> Andrew> +		    const CORE_ADDR neg_mask = ((~0) <<  (addr_size - 1));
> 
> I tend to think this should say ~(CORE_ADDR) 0 rather than just ~0.
> Otherwise won't this do the wrong thing if sizeof(int) < sizeof(CORE_ADDR)?
> 
> Andrew> +		    /* Check if signed bit is set and sign-extend values.  */
> Andrew> +		    if (*value & (neg_mask))
> Andrew> +		      *value |= (neg_mask );
> 
> Extra parens and an extra space here.
> 
> mips-tdep.c has the fun sign extension idiom:
> 
> 	    value = inst & 0x7ff;
> 	    value = (value ^ 0x400) - 0x400;		/* Sign-extend.  */
> 
> (If the high bits are 0 you can skip the "&".)
> This one avoids the ~0 business.
> 
> Another approach is to let the compiler handle it.
> 
>    LONGEST l = *value;
>    l = (l << nbits) >> nbits;
> 
> However this may be relying on undefined behavior.
> 
> Yours is fine, though, I just noticed these while digging around and
> wanted to point them out :-)

I stuck with what I had (your undefined behavior comment made me
reluctant to change), but cleaned it up inline with your comments.

How's this new version?

Thanks,
Andrew

---

commit e6968f8f31b4a0015476712f7c5f397a26e97a51
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Fri Mar 1 11:06:23 2019 +0000

    gdb: Better support for dynamic properties with negative values
    
    When the type of a property is smaller than the CORE_ADDR in which the
    property value has been placed, and if the property is signed, then
    sign extend the property value from its actual type up to the size of
    CORE_ADDR.
    
    gdb/ChangeLog:
    
            * dwarf2loc.c (dwarf2_evaluate_property): Sign extend property
            value if its desired type is smaller than a CORE_ADDR and signed.
    
    gdb/testsuite/ChangeLog:
    
            * gdb.fortran/vla-ptype.exp: Print array with negative bounds.
            * gdb.fortran/vla-sizeof.exp: Print the size of an array with
            negative bounds.
            * gdb.fortran/vla-value.exp: Print elements of an array with
            negative bounds.
            * gdb.fortran/vla.f90: Setup an array with negative bounds for
            testing.

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 88d34eb8660..c14097feff1 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -2454,6 +2454,29 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 		struct value *val = value_at (baton->property_type, *value);
 		*value = value_as_address (val);
 	      }
+	    else
+	      {
+		gdb_assert (baton->property_type != NULL);
+
+		struct type *type = check_typedef (baton->property_type);
+		if (TYPE_LENGTH (type) < sizeof (CORE_ADDR)
+		    && !TYPE_UNSIGNED (type))
+		  {
+		    /* If we have a valid return candidate and it's value
+		       is signed, we have to sign-extend the value because
+		       CORE_ADDR on 64bit machine has 8 bytes but address
+		       size of an 32bit application is bytes.  */
+		    const int addr_size
+		      = (dwarf2_per_cu_addr_size (baton->locexpr.per_cu)
+			 * TARGET_CHAR_BIT);
+		    const CORE_ADDR neg_mask
+		      = (~((CORE_ADDR) 0) <<  (addr_size - 1));
+
+		    /* Check if signed bit is set and sign-extend values.  */
+		    if (*value & neg_mask)
+		      *value |= neg_mask;
+		  }
+	      }
 	    return true;
 	  }
       }
diff --git a/gdb/testsuite/gdb.fortran/vla-ptype.exp b/gdb/testsuite/gdb.fortran/vla-ptype.exp
index 0f4abb63757..7ad7ecdea65 100644
--- a/gdb/testsuite/gdb.fortran/vla-ptype.exp
+++ b/gdb/testsuite/gdb.fortran/vla-ptype.exp
@@ -98,3 +98,15 @@ gdb_test "ptype vla2" "type = <not allocated>" "ptype vla2 not allocated"
 gdb_test "ptype vla2(5, 45, 20)" \
   "no such vector element \\\(vector not allocated\\\)" \
   "ptype vla2(5, 45, 20) not allocated"
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v1"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v1"
+gdb_test "ptype vla1" \
+    "type = $real, allocatable \\(-2:-1,-5:-2,-3:-1\\)" \
+    "ptype vla1 negative bounds"
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v2"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v2"
+gdb_test "ptype vla1" \
+    "type = $real, allocatable \\(-2:1,-5:2,-3:1\\)" \
+    "ptype vla1 negative lower bounds, positive upper bounds"
diff --git a/gdb/testsuite/gdb.fortran/vla-sizeof.exp b/gdb/testsuite/gdb.fortran/vla-sizeof.exp
index b6fdaebbf51..1340ccbcd0c 100644
--- a/gdb/testsuite/gdb.fortran/vla-sizeof.exp
+++ b/gdb/testsuite/gdb.fortran/vla-sizeof.exp
@@ -59,3 +59,13 @@ gdb_test "print sizeof(pvla)" " = 4000" "print sizeof associated pvla"
 gdb_test "print sizeof(pvla(3,2,1))" "4" \
     "print sizeof element from associated pvla"
 gdb_test "print sizeof(pvla(3:4,2,1))" "800" "print sizeof sliced pvla"
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v1"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v1"
+gdb_test "print sizeof(vla1)" " = 96" \
+    "print sizeof vla1 negative bounds"
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v2"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v2"
+gdb_test "print sizeof(vla1)" " = 640" \
+    "print sizeof vla1 negative lower bounds, positive upper bounds"
diff --git a/gdb/testsuite/gdb.fortran/vla-value.exp b/gdb/testsuite/gdb.fortran/vla-value.exp
index be397fd95fb..3145d21c15c 100644
--- a/gdb/testsuite/gdb.fortran/vla-value.exp
+++ b/gdb/testsuite/gdb.fortran/vla-value.exp
@@ -161,3 +161,30 @@ gdb_breakpoint [gdb_get_line_number "pvla-deassociated"]
 gdb_continue_to_breakpoint "pvla-deassociated"
 gdb_test "print \$mypvar(1,3,8)" " = 1001" \
   "print \$mypvar(1,3,8) after deallocated"
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v1"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v1"
+with_test_prefix "negative bounds" {
+    gdb_test "print vla1(-2,-5,-3)" " = 1"
+    gdb_test "print vla1(-2,-3,-1)" " = -231"
+    gdb_test "print vla1(-3,-5,-3)" "no such vector element"
+    gdb_test "print vla1(-2,-6,-3)" "no such vector element"
+    gdb_test "print vla1(-2,-5,-4)" "no such vector element"
+    gdb_test "print vla1(0,-2,-1)" "no such vector element"
+    gdb_test "print vla1(-1,-1,-1)" "no such vector element"
+    gdb_test "print vla1(-1,-2,0)" "no such vector element"
+}
+
+gdb_breakpoint [gdb_get_line_number "vla1-neg-bounds-v2"]
+gdb_continue_to_breakpoint "vla1-neg-bounds-v2"
+with_test_prefix "negative lower bounds, positive upper bounds" {
+    gdb_test "print vla1(-2,-5,-3)" " = 2"
+    gdb_test "print vla1(-2,-3,-1)" " = 2"
+    gdb_test "print vla1(-2,-4,-2)" " = -242"
+    gdb_test "print vla1(-3,-5,-3)" "no such vector element"
+    gdb_test "print vla1(-2,-6,-3)" "no such vector element"
+    gdb_test "print vla1(-2,-5,-4)" "no such vector element"
+    gdb_test "print vla1(2,2,1)" "no such vector element"
+    gdb_test "print vla1(1,3,1)" "no such vector element"
+    gdb_test "print vla1(1,2,2)" "no such vector element"
+}
diff --git a/gdb/testsuite/gdb.fortran/vla.f90 b/gdb/testsuite/gdb.fortran/vla.f90
index 5bc608744b3..0ccb5c90d93 100644
--- a/gdb/testsuite/gdb.fortran/vla.f90
+++ b/gdb/testsuite/gdb.fortran/vla.f90
@@ -54,4 +54,19 @@ program vla
 
   allocate (vla3 (2,2))               ! vla2-deallocated
   vla3(:,:) = 13
+
+  allocate (vla1 (-2:-1, -5:-2, -3:-1))
+  vla1(:, :, :) = 1
+  vla1(-2, -3, -1) = -231
+
+  deallocate (vla1)                   ! vla1-neg-bounds-v1
+  l = allocated(vla1)
+
+  allocate (vla1 (-2:1, -5:2, -3:1))
+  vla1(:, :, :) = 2
+  vla1(-2, -4, -2) = -242
+
+  deallocate (vla1)                   ! vla1-neg-bounds-v2
+  l = allocated(vla1)
+
 end program vla


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCHv2 5/5] gdb: Better support for dynamic properties with negative values
  2019-06-10 22:17       ` Andrew Burgess
@ 2019-07-10 15:03         ` Tom Tromey
  0 siblings, 0 replies; 20+ messages in thread
From: Tom Tromey @ 2019-07-10 15:03 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: Tom Tromey, gdb-patches

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

I meant to reply to this but forgot to.  I'm sorry about that.

>> I wonder whether this should be using ULONGEST rather than CORE_ADDR
>> now.

Andrew> I don't think you're wrong, but I'm hoping I can convince you to let
Andrew> me punt this change for now, it would be fairly big change to the
Andrew> dwarf property fetching code.  What we have seems to hang together for
Andrew> now, so I'd prefer to get this change in first.

Yes, I think it's fine to just move forward with the patch you've
written.

Tom

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCHv2 4/5] gdb: Carry default property type around with dynamic properties
  2019-05-05 20:57 ` [PATCH 0/3] " Andrew Burgess
                     ` (3 preceding siblings ...)
  2019-05-09 22:22   ` [PATCHv2 5/5] gdb: Better support for dynamic properties with negative values Andrew Burgess
@ 2019-05-09 22:22   ` Andrew Burgess
  2019-05-22 19:05     ` Tom Tromey
  2019-05-09 22:22   ` [PATCHv2 2/5] gdb: Convert dwarf2_evaluate_property to return bool Andrew Burgess
                     ` (2 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2019-05-09 22:22 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

This commit is preparation for the next one, with the aim of better
supporting signed dynamic properties on targets where the address size
specified in the DWARF headers is smaller than a CORE_ADDR, for
example debugging an i386 application on x86-64.

Consider this small Fortran program 'bounds.f90':

    program test
      integer, allocatable :: array (:)
      allocate (array (-5:5))
      array(3) = 1
    end program test

Compiled with 'gfortran -m32 -g3 -O0 -o bounds bounds.f90'.  The DWARF
for 'array' looks like this:

   <2><97>: Abbrev Number: 10 (DW_TAG_variable)
      <98>   DW_AT_name        : (indirect string, offset: 0x0): array
      <9c>   DW_AT_decl_file   : 1
      <9d>   DW_AT_decl_line   : 2
      <9e>   DW_AT_type        : <0xaf>
      <a2>   DW_AT_location    : 2 byte block: 91 58              (DW_OP_fbreg: -40)
   <2><a5>: Abbrev Number: 11 (DW_TAG_lexical_block)
      <a6>   DW_AT_low_pc      : 0x80485c3
      <aa>   DW_AT_high_pc     : 0x8b
   <2><ae>: Abbrev Number: 0
   <1><af>: Abbrev Number: 12 (DW_TAG_array_type)
      <b0>   DW_AT_data_location: 2 byte block: 97 6              (DW_OP_push_object_address; DW_OP_deref)
      <b3>   DW_AT_allocated   : 4 byte block: 97 6 30 2e         (DW_OP_push_object_address; DW_OP_deref; DW_OP_lit0; DW_OP_ne)
      <b8>   DW_AT_type        : <0x2a>
   <2><bc>: Abbrev Number: 13 (DW_TAG_subrange_type)
      <bd>   DW_AT_lower_bound : 4 byte block: 97 23 10 6         (DW_OP_push_object_address; DW_OP_plus_uconst: 16; DW_OP_deref)
      <c2>   DW_AT_upper_bound : 4 byte block: 97 23 14 6         (DW_OP_push_object_address; DW_OP_plus_uconst: 20; DW_OP_deref)
      <c7>   DW_AT_byte_stride : 6 byte block: 97 23 c 6 34 1e    (DW_OP_push_object_address; DW_OP_plus_uconst: 12; DW_OP_deref; DW_OP_lit4; DW_OP_mul)
   <2><ce>: Abbrev Number: 0

If we look at the DW_AT_lower_bound attribute, which will become a
dynamic property that GDB evaluates when needed by calling
dwarf2_evaluate_property.

The process of evaluating a dynamic property requires GDB to execute
each DW_OP_* operation, the results of these operations is held on a
stack of 'struct value *'s.

When the entire expression is evaluated the result is on top of the
stack.

If we look at DW_AT_lower_bound then the last operation is
DW_OP_deref, this loads a signed address the size of which matches the
DWARF address size, and so in our i386 on x86-64 situation, the top of
the stack will be a signed 4-byte value.

The problem is how these values are fetched from the stack.  Currently
they are always fetched by a call to dwarf_expr_context::fetch_address,
which converts the value to an unsigned value with a length matching
the values current length, before converting to a CORE_ADDR.  This
means we loose the signed nature of the property.

I wonder if the best solution for dealing with signed properties will
be to move away from an over reliance on fetch_address, and instead
come up with a new solution that considers the current type of the
value on the stack, and the type that the value needs to become;
basically a solution built around casting rather than assuming we
always want an address.

However, before we can start to even think about moving away from
fetch_address, there is a more urgent issue to fix, which is we don't
currently know what type each property should be.  We just hold the
value of the property in a CORE_ADDR as returned by fetch_address, and
rely on higher level code (outside of the DWARF expression evaluation
code) to fix things up for us.  This is what this patch aims to
address.

When creating a dynamic property (see attr_to_dynamic_prop in
dwarf2read.c) we can sometimes figure out the type of a property; if
the property is a reference to another DIE then it will have a
DW_AT_type attribute.

However, the DW_AT_lower_bound case above isn't a reference to another
DIE, it's just a DWARF expression.  We don't have any indication for
what type the property should have.

Luckily, the DWARF spec helps us out, for the lower and upper bounds
5.13 of the DWARFv5 spec tells us that without any other type
information the bounds are signed integers the same size as a DWARF
address.

It is my belief that we can find a suitable default type for every
dynamic property, either specified explicitly in the DWARF spec, or we
can infer an obvious choice if the spec doesn't help us.

This commit extends the creation of all dynamic properties to include
suggesting a suitable default type, all dynamic properties now always
carry their type around with them.

In later commits we can use this property type to ensure that the
value we extract from the DWARF stack is handled in a suitable manor
to correctly maintain its sign extension.

There should be no user visible changes from this commit.  The actual
fix to correctly support negative array bounds will come later.

gdb/ChangeLog:

	* dwarf2loc.c (dwarf2_evaluate_property): Update to take account
	of changes to field names, and use new is_reference field to
	decide if a property is a reference or not.
	* dwarf2loc.h (struct dwarf2_locexpr_baton): Add 'is_reference'
	field.
	(struct dwarf2_property_baton): Update header comment, rename
	'referenced_type' to 'property_type' and update comments.
	* dwarf2read.c (attr_to_dynamic_prop): Add extra parameter to hold
	default property type, store in property baton, update to take
	accound of renamed field.
	(read_func_scope): Update call to attr_to_dynamic_prop.
	(read_array_type): Likewise.
	(dwarf2_per_cu_addr_sized_int_type): New function.
	(read_subrange_index_type): Move type finding code to
	dwarf2_per_cu_addr_sized_int_type.
	(read_subrange_type): Update calls to attr_to_dynamic_prop.
	(dwarf2_per_cu_addr_type): New function.
	(set_die_type): Update calls to attr_to_dynamic_prop.
---
 gdb/ChangeLog    |  21 ++++++++
 gdb/dwarf2loc.c  |  10 ++--
 gdb/dwarf2loc.h  |  24 ++++++---
 gdb/dwarf2read.c | 150 +++++++++++++++++++++++++++++++++++++++++--------------
 4 files changed, 155 insertions(+), 50 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index c03f261f1e1..88d34eb8660 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -2443,15 +2443,15 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
       {
 	const struct dwarf2_property_baton *baton
 	  = (const struct dwarf2_property_baton *) prop->data.baton;
+	gdb_assert (baton->property_type != NULL);
 
 	if (dwarf2_locexpr_baton_eval (&baton->locexpr, frame,
 				       addr_stack ? addr_stack->addr : 0,
 				       value))
 	  {
-	    if (baton->referenced_type)
+	    if (baton->locexpr.is_reference)
 	      {
-		struct value *val = value_at (baton->referenced_type, *value);
-
+		struct value *val = value_at (baton->property_type, *value);
 		*value = value_as_address (val);
 	      }
 	    return true;
@@ -2471,7 +2471,7 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 	data = dwarf2_find_location_expression (&baton->loclist, &size, pc);
 	if (data != NULL)
 	  {
-	    val = dwarf2_evaluate_loc_desc (baton->referenced_type, frame, data,
+	    val = dwarf2_evaluate_loc_desc (baton->property_type, frame, data,
 					    size, baton->loclist.per_cu);
 	    if (!value_optimized_out (val))
 	      {
@@ -2497,7 +2497,7 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 	  {
 	    /* This approach lets us avoid checking the qualifiers.  */
 	    if (TYPE_MAIN_TYPE (pinfo->type)
-		== TYPE_MAIN_TYPE (baton->referenced_type))
+		== TYPE_MAIN_TYPE (baton->property_type))
 	      break;
 	  }
 	if (pinfo == NULL)
diff --git a/gdb/dwarf2loc.h b/gdb/dwarf2loc.h
index ac1a771a9f3..baa5762003d 100644
--- a/gdb/dwarf2loc.h
+++ b/gdb/dwarf2loc.h
@@ -183,6 +183,12 @@ struct dwarf2_locexpr_baton
      zero.  */
   size_t size;
 
+  /* When true this location expression is a reference and actually
+     describes the address at which the value of the attribute can be
+     found.  When false the expression provides the value of the attribute
+     directly.  */
+  bool is_reference;
+
   /* The compilation unit containing the symbol whose location
      we're computing.  */
   struct dwarf2_per_cu_data *per_cu;
@@ -228,23 +234,27 @@ struct dwarf2_offset_baton
 
 /* A dynamic property is either expressed as a single location expression
    or a location list.  If the property is an indirection, pointing to
-   another die, keep track of the targeted type in REFERENCED_TYPE.  */
+   another die, keep track of the targeted type in PROPERTY_TYPE.
+   Alternatively, if the property location gives the property value
+   directly then it will have PROPERTY_TYPE.  */
 
 struct dwarf2_property_baton
 {
   /* If the property is an indirection, we need to evaluate the location
-     in the context of the type REFERENCED_TYPE.
-     If NULL, the location is the actual value of the property.  */
-  struct type *referenced_type;
+     in the context of the type PROPERTY_TYPE.  If the property is supplied
+     by value then it will be of PROPERTY_TYPE.  This field should never be
+     NULL.  */
+  struct type *property_type;
   union
   {
-    /* Location expression.  */
+    /* Location expression either evaluated in the context of
+       PROPERTY_TYPE, or a value of type PROPERTY_TYPE.  */
     struct dwarf2_locexpr_baton locexpr;
 
-    /* Location list to be evaluated in the context of REFERENCED_TYPE.  */
+    /* Location list to be evaluated in the context of PROPERTY_TYPE.  */
     struct dwarf2_loclist_baton loclist;
 
-    /* The location is an offset to REFERENCED_TYPE.  */
+    /* The location is an offset to PROPERTY_TYPE.  */
     struct dwarf2_offset_baton offset_info;
   };
 };
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index e9c53148961..76f017daa1a 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1816,7 +1816,7 @@ static void read_signatured_type (struct signatured_type *);
 
 static int attr_to_dynamic_prop (const struct attribute *attr,
 				 struct die_info *die, struct dwarf2_cu *cu,
-				 struct dynamic_prop *prop);
+				 struct dynamic_prop *prop, struct type *type);
 
 /* memory allocation interface */
 
@@ -1896,6 +1896,10 @@ static void queue_comp_unit (struct dwarf2_per_cu_data *per_cu,
 
 static void process_queue (struct dwarf2_per_objfile *dwarf2_per_objfile);
 
+static struct type *dwarf2_per_cu_addr_type (struct dwarf2_per_cu_data *per_cu);
+static struct type *dwarf2_per_cu_addr_sized_int_type
+  (struct dwarf2_per_cu_data *per_cu);
+
 /* Class, the destructor of which frees all allocated queue entries.  This
    will only have work to do if an error was thrown while processing the
    dwarf.  If no error was thrown then the queue entries should have all
@@ -13737,7 +13741,8 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
     {
       newobj->static_link
 	= XOBNEW (&objfile->objfile_obstack, struct dynamic_prop);
-      attr_to_dynamic_prop (attr, die, cu, newobj->static_link);
+      attr_to_dynamic_prop (attr, die, cu, newobj->static_link,
+			    dwarf2_per_cu_addr_type (cu->per_cu));
     }
 
   cu->list_in_scope = cu->get_builder ()->get_local_symbols ();
@@ -16493,10 +16498,13 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
   if (attr != NULL)
     {
       int stride_ok;
+      struct type *prop_type
+	= dwarf2_per_cu_addr_sized_int_type (cu->per_cu);
 
       byte_stride_prop
 	= (struct dynamic_prop *) alloca (sizeof (struct dynamic_prop));
-      stride_ok = attr_to_dynamic_prop (attr, die, cu, byte_stride_prop);
+      stride_ok = attr_to_dynamic_prop (attr, die, cu, byte_stride_prop,
+					prop_type);
       if (!stride_ok)
 	{
 	  complaint (_("unable to read array DW_AT_byte_stride "
@@ -17711,22 +17719,26 @@ read_base_type (struct die_info *die, struct dwarf2_cu *cu)
 
 static int
 attr_to_dynamic_prop (const struct attribute *attr, struct die_info *die,
-		      struct dwarf2_cu *cu, struct dynamic_prop *prop)
+		      struct dwarf2_cu *cu, struct dynamic_prop *prop,
+		      struct type *default_type)
 {
   struct dwarf2_property_baton *baton;
   struct obstack *obstack
     = &cu->per_cu->dwarf2_per_objfile->objfile->objfile_obstack;
 
+  gdb_assert (default_type != NULL);
+
   if (attr == NULL || prop == NULL)
     return 0;
 
   if (attr_form_is_block (attr))
     {
       baton = XOBNEW (obstack, struct dwarf2_property_baton);
-      baton->referenced_type = NULL;
+      baton->property_type = default_type;
       baton->locexpr.per_cu = cu->per_cu;
       baton->locexpr.size = DW_BLOCK (attr)->size;
       baton->locexpr.data = DW_BLOCK (attr)->data;
+      baton->locexpr.is_reference = false;
       prop->data.baton = baton;
       prop->kind = PROP_LOCEXPR;
       gdb_assert (prop->data.baton != NULL);
@@ -17751,7 +17763,7 @@ attr_to_dynamic_prop (const struct attribute *attr, struct die_info *die,
 	    if (attr_form_is_section_offset (target_attr))
 	      {
 		baton = XOBNEW (obstack, struct dwarf2_property_baton);
-		baton->referenced_type = die_type (target_die, target_cu);
+		baton->property_type = die_type (target_die, target_cu);
 		fill_in_loclist_baton (cu, &baton->loclist, target_attr);
 		prop->data.baton = baton;
 		prop->kind = PROP_LOCLIST;
@@ -17760,10 +17772,11 @@ attr_to_dynamic_prop (const struct attribute *attr, struct die_info *die,
 	    else if (attr_form_is_block (target_attr))
 	      {
 		baton = XOBNEW (obstack, struct dwarf2_property_baton);
-		baton->referenced_type = die_type (target_die, target_cu);
+		baton->property_type = die_type (target_die, target_cu);
 		baton->locexpr.per_cu = cu->per_cu;
 		baton->locexpr.size = DW_BLOCK (target_attr)->size;
 		baton->locexpr.data = DW_BLOCK (target_attr)->data;
+		baton->locexpr.is_reference = true;
 		prop->data.baton = baton;
 		prop->kind = PROP_LOCEXPR;
 		gdb_assert (prop->data.baton != NULL);
@@ -17784,7 +17797,7 @@ attr_to_dynamic_prop (const struct attribute *attr, struct die_info *die,
 		return 0;
 
 	      baton = XOBNEW (obstack, struct dwarf2_property_baton);
-	      baton->referenced_type = read_type_die (target_die->parent,
+	      baton->property_type = read_type_die (target_die->parent,
 						      target_cu);
 	      baton->offset_info.offset = offset;
 	      baton->offset_info.type = die_type (target_die, target_cu);
@@ -17809,6 +17822,35 @@ attr_to_dynamic_prop (const struct attribute *attr, struct die_info *die,
   return 1;
 }
 
+/* Find a signed integer type the same size as the address size given in
+   the compilation unit header for PER_CU.  */
+
+static struct type *
+dwarf2_per_cu_addr_sized_int_type (struct dwarf2_per_cu_data *per_cu)
+{
+  struct objfile *objfile = per_cu->dwarf2_per_objfile->objfile;
+  int addr_size = dwarf2_per_cu_addr_size (per_cu);
+  struct type *int_type;
+
+  int_type = objfile_type (objfile)->builtin_short;
+  if (int_type != NULL && TYPE_LENGTH (int_type) == addr_size)
+    return int_type;
+
+  int_type = objfile_type (objfile)->builtin_int;
+  if (int_type != NULL && TYPE_LENGTH (int_type) == addr_size)
+    return int_type;
+
+  int_type = objfile_type (objfile)->builtin_long;
+  if (int_type != NULL && TYPE_LENGTH (int_type) == addr_size)
+    return int_type;
+
+  int_type = objfile_type (objfile)->builtin_long_long;
+  if (int_type != NULL && TYPE_LENGTH (int_type) == addr_size)
+    return int_type;
+
+  gdb_assert_not_reached ("unable to find suitable integer type");
+}
+
 /* Read the DW_AT_type attribute for a sub-range.  If this attribute is not
    present (which is valid) then compute the default type based on the
    compilation units address size.  */
@@ -17831,30 +17873,7 @@ read_subrange_index_type (struct die_info *die, struct dwarf2_cu *cu)
      FIXME: muller/2010-05-28: Possible references to object for low bound,
      high bound or count are not yet handled by this code.  */
   if (TYPE_CODE (index_type) == TYPE_CODE_VOID)
-    {
-      struct objfile *objfile = cu->per_cu->dwarf2_per_objfile->objfile;
-      struct gdbarch *gdbarch = get_objfile_arch (objfile);
-      int addr_size = gdbarch_addr_bit (gdbarch) /8;
-      struct type *int_type = objfile_type (objfile)->builtin_int;
-
-      /* Test "int", "long int", and "long long int" objfile types,
-	 and select the first one having a size above or equal to the
-	 architecture address size.  */
-      if (int_type && TYPE_LENGTH (int_type) >= addr_size)
-	index_type = int_type;
-      else
-	{
-	  int_type = objfile_type (objfile)->builtin_long;
-	  if (int_type && TYPE_LENGTH (int_type) >= addr_size)
-	    index_type = int_type;
-	  else
-	    {
-	      int_type = objfile_type (objfile)->builtin_long_long;
-	      if (int_type && TYPE_LENGTH (int_type) >= addr_size)
-		index_type = int_type;
-	    }
-	}
-    }
+    index_type = dwarf2_per_cu_addr_sized_int_type (cu->per_cu);
 
   return index_type;
 }
@@ -17923,7 +17942,7 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
 
   attr = dwarf2_attr (die, DW_AT_lower_bound, cu);
   if (attr)
-    attr_to_dynamic_prop (attr, die, cu, &low);
+    attr_to_dynamic_prop (attr, die, cu, &low, base_type);
   else if (!low_default_is_valid)
     complaint (_("Missing DW_AT_lower_bound "
 				      "- DIE at %s [in module %s]"),
@@ -17932,10 +17951,10 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
 
   struct attribute *attr_ub, *attr_count;
   attr = attr_ub = dwarf2_attr (die, DW_AT_upper_bound, cu);
-  if (!attr_to_dynamic_prop (attr, die, cu, &high))
+  if (!attr_to_dynamic_prop (attr, die, cu, &high, base_type))
     {
       attr = attr_count = dwarf2_attr (die, DW_AT_count, cu);
-      if (attr_to_dynamic_prop (attr, die, cu, &high))
+      if (attr_to_dynamic_prop (attr, die, cu, &high, base_type))
 	{
 	  /* If bounds are constant do the final calculation here.  */
 	  if (low.kind == PROP_CONST && high.kind == PROP_CONST)
@@ -25256,6 +25275,58 @@ dwarf2_per_cu_text_offset (struct dwarf2_per_cu_data *per_cu)
   return ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
 }
 
+/* Return a type that is a generic pointer type, the size of which matches
+   the address size given in the compilation unit header for PER_CU.  */
+static struct type *
+dwarf2_per_cu_addr_type (struct dwarf2_per_cu_data *per_cu)
+{
+  struct objfile *objfile = per_cu->dwarf2_per_objfile->objfile;
+  struct type *void_type = objfile_type (objfile)->builtin_void;
+  struct type *addr_type = lookup_pointer_type (void_type);
+  int addr_size = dwarf2_per_cu_addr_size (per_cu);
+
+  if (TYPE_LENGTH (addr_type) == addr_size)
+    return addr_type;
+
+  /* Yuck! We currently only support one address size per architecture in
+     GDB, which should usually match the address size encoded into the
+     compilation unit header.  However... we have a few tests where this is
+     not the case, these are mostly test cases where the DWARF is hand
+     written and includes a fixed address size, for example 8-bytes.  When
+     we compile these tests on a 32-bit i386 target the gdbarch address
+     size is 4-bytes and the above attempt to create a suitable address
+     type fails.
+
+     As we can't currently create an address type of a different size, we
+     instead substitute an unsigned integer for an address.
+
+     I don't know if there are targets that have signed addresses and if
+     they would need a signed integer here.  I figure we'll handle that
+     case when it presents itself as a problem.  */
+
+  addr_type = objfile_type (objfile)->builtin_unsigned_char;
+  if (addr_type != NULL && TYPE_LENGTH (addr_type) == addr_size)
+    return addr_type;
+
+  addr_type = objfile_type (objfile)->builtin_unsigned_short;
+  if (addr_type != NULL && TYPE_LENGTH (addr_type) == addr_size)
+    return addr_type;
+
+  addr_type = objfile_type (objfile)->builtin_unsigned_int;
+  if (addr_type != NULL && TYPE_LENGTH (addr_type) == addr_size)
+    return addr_type;
+
+  addr_type = objfile_type (objfile)->builtin_unsigned_long;
+  if (addr_type != NULL && TYPE_LENGTH (addr_type) == addr_size)
+    return addr_type;
+
+  addr_type = objfile_type (objfile)->builtin_unsigned_long_long;
+  if (addr_type != NULL && TYPE_LENGTH (addr_type) == addr_size)
+    return addr_type;
+
+  gdb_assert_not_reached ("failed to find address type");
+}
+
 /* Return DWARF version number of PER_CU.  */
 
 short
@@ -25522,7 +25593,8 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
   attr = dwarf2_attr (die, DW_AT_allocated, cu);
   if (attr_form_is_block (attr))
     {
-      if (attr_to_dynamic_prop (attr, die, cu, &prop))
+      struct type *prop_type = dwarf2_per_cu_addr_sized_int_type (cu->per_cu);
+      if (attr_to_dynamic_prop (attr, die, cu, &prop, prop_type))
         add_dyn_prop (DYN_PROP_ALLOCATED, prop, type);
     }
   else if (attr != NULL)
@@ -25536,7 +25608,8 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
   attr = dwarf2_attr (die, DW_AT_associated, cu);
   if (attr_form_is_block (attr))
     {
-      if (attr_to_dynamic_prop (attr, die, cu, &prop))
+      struct type *prop_type = dwarf2_per_cu_addr_sized_int_type (cu->per_cu);
+      if (attr_to_dynamic_prop (attr, die, cu, &prop, prop_type))
         add_dyn_prop (DYN_PROP_ASSOCIATED, prop, type);
     }
   else if (attr != NULL)
@@ -25548,7 +25621,8 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
 
   /* Read DW_AT_data_location and set in type.  */
   attr = dwarf2_attr (die, DW_AT_data_location, cu);
-  if (attr_to_dynamic_prop (attr, die, cu, &prop))
+  if (attr_to_dynamic_prop (attr, die, cu, &prop,
+			    dwarf2_per_cu_addr_type (cu->per_cu)))
     add_dyn_prop (DYN_PROP_DATA_LOCATION, prop, type);
 
   if (dwarf2_per_objfile->die_type_hash == NULL)
-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCHv2 4/5] gdb: Carry default property type around with dynamic properties
  2019-05-09 22:22   ` [PATCHv2 4/5] gdb: Carry default property type around with dynamic properties Andrew Burgess
@ 2019-05-22 19:05     ` Tom Tromey
  2019-06-10 22:29       ` Andrew Burgess
  0 siblings, 1 reply; 20+ messages in thread
From: Tom Tromey @ 2019-05-22 19:05 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

Andrew>    <1><af>: Abbrev Number: 12 (DW_TAG_array_type)
Andrew>       <b0>   DW_AT_data_location: 2 byte block: 97 6              (DW_OP_push_object_address; DW_OP_deref)
Andrew>       <b3>   DW_AT_allocated   : 4 byte block: 97 6 30 2e         (DW_OP_push_object_address; DW_OP_deref; DW_OP_lit0; DW_OP_ne)
Andrew>       <b8>   DW_AT_type        : <0x2a>
Andrew>    <2><bc>: Abbrev Number: 13 (DW_TAG_subrange_type)
Andrew>       <bd>   DW_AT_lower_bound : 4 byte block: 97 23 10 6         (DW_OP_push_object_address; DW_OP_plus_uconst: 16; DW_OP_deref)
Andrew>       <c2>   DW_AT_upper_bound : 4 byte block: 97 23 14 6         (DW_OP_push_object_address; DW_OP_plus_uconst: 20; DW_OP_deref)
Andrew>       <c7>   DW_AT_byte_stride : 6 byte block: 97 23 c 6 34 1e    (DW_OP_push_object_address; DW_OP_plus_uconst: 12; DW_OP_deref; DW_OP_lit4; DW_OP_mul)

This is a funny coincidence, because we were discussing a similar case
with Ada just this week.  Gnat can generate very similar expressions in
some cases:

 <1><11b5>: Abbrev Number: 8 (DW_TAG_array_type)
    <11b6>   DW_AT_name        : (indirect string, offset: 0x1ba2): string
    <11ba>   DW_AT_data_location: 2 byte block: 97 6 	(DW_OP_push_object_address; DW_OP_deref)
    <11bd>   DW_AT_type        : <0x113c>
    <11c1>   DW_AT_sibling     : <0x11db>
 <2><11c5>: Abbrev Number: 9 (DW_TAG_subrange_type)
    <11c6>   DW_AT_type        : <0x11e0>
    <11ca>   DW_AT_lower_bound : 6 byte block: 97 23 8 6 94 4 	(DW_OP_push_object_address; DW_OP_plus_uconst: 8; DW_OP_deref; DW_OP_deref_size: 4)
    <11d1>   DW_AT_upper_bound : 8 byte block: 97 23 8 6 23 4 94 4 	(DW_OP_push_object_address; DW_OP_plus_uconst: 8; DW_OP_deref; DW_OP_plus_uconst: 4; DW_OP_deref_size: 4)

Essentially what's going on here is that the compiler represents a
pointer-to-array as a structure that holds a pointer and the array
bounds; and then chooses to represent this in DWARF as the above.

This in turn causes problems in gdb.  In the above case, it's not
possible to make a call like `do_something("string")', because gdb
doesn't know that constructing one of these arrays at runtime actually
requires two allocations and some special messing around.

So, this week we've been talking about changing the representation of
these things to be more explicit, that is, make the hidden struct type
explicit (and marked DW_AT_artificial).

I wonder how Fortran will handle this, or whether it even needs to.  Is
it possible for gdb to construct one of these arrays at runtime?

Another option, besides changing the representation, would be to teach
gdb how to create one of the above by recognizing the expressions as a
kind of special case.  This seemed uglier and more fragile, though.

For Ada at least, this sort of DWARF isn't the default yet.  The default
is this Ada-specific name encodings scheme; you have to opt-in to DWARF
using a compiler command-line flag; but one of the things I'm looking
into is getting gdb to the point where we can flip the default.

Andrew> I wonder if the best solution for dealing with signed properties will
Andrew> be to move away from an over reliance on fetch_address, and instead
Andrew> come up with a new solution that considers the current type of the
Andrew> value on the stack, and the type that the value needs to become;
Andrew> basically a solution built around casting rather than assuming we
Andrew> always want an address.

DWARF now has typed expressions as well, so gcc could just emit this
explicitly, I think.  I didn't look at gcc's dwarf2out.c for this but
maybe you need -gdwarf-5 to make this happen.

Andrew> It is my belief that we can find a suitable default type for every
Andrew> dynamic property, either specified explicitly in the DWARF spec, or we
Andrew> can infer an obvious choice if the spec doesn't help us.

Seems reasonable.

Andrew> +static struct type *
Andrew> +dwarf2_per_cu_addr_sized_int_type (struct dwarf2_per_cu_data *per_cu)
Andrew> +{
Andrew> +  struct objfile *objfile = per_cu->dwarf2_per_objfile->objfile;
Andrew> +  int addr_size = dwarf2_per_cu_addr_size (per_cu);
Andrew> +  struct type *int_type;
Andrew> +
Andrew> +  int_type = objfile_type (objfile)->builtin_short;
Andrew> +  if (int_type != NULL && TYPE_LENGTH (int_type) == addr_size)
Andrew> +    return int_type;

dwarf2_per_cu_addr_type handles unsigned char here, but this one does not.
I doubt it matters but I wonder why the difference.

Andrew> +/* Return a type that is a generic pointer type, the size of which matches
Andrew> +   the address size given in the compilation unit header for PER_CU.  */
Andrew> +static struct type *
Andrew> +dwarf2_per_cu_addr_type (struct dwarf2_per_cu_data *per_cu)
Andrew> +{
Andrew> +  struct objfile *objfile = per_cu->dwarf2_per_objfile->objfile;
Andrew> +  struct type *void_type = objfile_type (objfile)->builtin_void;
Andrew> +  struct type *addr_type = lookup_pointer_type (void_type);
Andrew> +  int addr_size = dwarf2_per_cu_addr_size (per_cu);
Andrew> +
Andrew> +  if (TYPE_LENGTH (addr_type) == addr_size)
Andrew> +    return addr_type;
Andrew> +
Andrew> +  /* Yuck! We currently only support one address size per architecture in
Andrew> +     GDB, which should usually match the address size encoded into the
Andrew> +     compilation unit header.  However... we have a few tests where this is
Andrew> +     not the case, these are mostly test cases where the DWARF is hand
Andrew> +     written and includes a fixed address size, for example 8-bytes.  When
Andrew> +     we compile these tests on a 32-bit i386 target the gdbarch address
Andrew> +     size is 4-bytes and the above attempt to create a suitable address
Andrew> +     type fails.
Andrew> +
Andrew> +     As we can't currently create an address type of a different size, we
Andrew> +     instead substitute an unsigned integer for an address.
Andrew> +
Andrew> +     I don't know if there are targets that have signed addresses and if
Andrew> +     they would need a signed integer here.  I figure we'll handle that
Andrew> +     case when it presents itself as a problem.  */

gdbarch.sh makes a distinction between addresses and pointers which, I
confess, I have never understood.  However, based on this, I think at
least MIPS may have signed pointers.  From mips-tdep.c:

  set_gdbarch_pointer_to_address (gdbarch, signed_pointer_to_address);
  set_gdbarch_address_to_pointer (gdbarch, address_to_signed_pointer);

Also, I wonder whether it wouldn't be simpler to just provide a way to
create a TYPE_CODE_PTR type with a specified size.

Tom

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCHv2 4/5] gdb: Carry default property type around with dynamic properties
  2019-05-22 19:05     ` Tom Tromey
@ 2019-06-10 22:29       ` Andrew Burgess
       [not found]         ` <20190710141321.GL23204@embecosm.com>
  0 siblings, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2019-06-10 22:29 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

* Tom Tromey <tom@tromey.com> [2019-05-22 15:05:48 -0400]:

> Andrew>    <1><af>: Abbrev Number: 12 (DW_TAG_array_type)
> Andrew>       <b0>   DW_AT_data_location: 2 byte block: 97 6              (DW_OP_push_object_address; DW_OP_deref)
> Andrew>       <b3>   DW_AT_allocated   : 4 byte block: 97 6 30 2e         (DW_OP_push_object_address; DW_OP_deref; DW_OP_lit0; DW_OP_ne)
> Andrew>       <b8>   DW_AT_type        : <0x2a>
> Andrew>    <2><bc>: Abbrev Number: 13 (DW_TAG_subrange_type)
> Andrew>       <bd>   DW_AT_lower_bound : 4 byte block: 97 23 10 6         (DW_OP_push_object_address; DW_OP_plus_uconst: 16; DW_OP_deref)
> Andrew>       <c2>   DW_AT_upper_bound : 4 byte block: 97 23 14 6         (DW_OP_push_object_address; DW_OP_plus_uconst: 20; DW_OP_deref)
> Andrew>       <c7>   DW_AT_byte_stride : 6 byte block: 97 23 c 6 34 1e    (DW_OP_push_object_address; DW_OP_plus_uconst: 12; DW_OP_deref; DW_OP_lit4; DW_OP_mul)
> 
> This is a funny coincidence, because we were discussing a similar case
> with Ada just this week.  Gnat can generate very similar expressions in
> some cases:
> 
>  <1><11b5>: Abbrev Number: 8 (DW_TAG_array_type)
>     <11b6>   DW_AT_name        : (indirect string, offset: 0x1ba2): string
>     <11ba>   DW_AT_data_location: 2 byte block: 97 6 	(DW_OP_push_object_address; DW_OP_deref)
>     <11bd>   DW_AT_type        : <0x113c>
>     <11c1>   DW_AT_sibling     : <0x11db>
>  <2><11c5>: Abbrev Number: 9 (DW_TAG_subrange_type)
>     <11c6>   DW_AT_type        : <0x11e0>
>     <11ca>   DW_AT_lower_bound : 6 byte block: 97 23 8 6 94 4 	(DW_OP_push_object_address; DW_OP_plus_uconst: 8; DW_OP_deref; DW_OP_deref_size: 4)
>     <11d1>   DW_AT_upper_bound : 8 byte block: 97 23 8 6 23 4 94 4 	(DW_OP_push_object_address; DW_OP_plus_uconst: 8; DW_OP_deref; DW_OP_plus_uconst: 4; DW_OP_deref_size: 4)
> 
> 
> Essentially what's going on here is that the compiler represents a
> pointer-to-array as a structure that holds a pointer and the array
> bounds; and then chooses to represent this in DWARF as the above.
> 
> This in turn causes problems in gdb.  In the above case, it's not
> possible to make a call like `do_something("string")', because gdb
> doesn't know that constructing one of these arrays at runtime actually
> requires two allocations and some special messing around.
> 
> So, this week we've been talking about changing the representation of
> these things to be more explicit, that is, make the hidden struct type
> explicit (and marked DW_AT_artificial).
> 
> I wonder how Fortran will handle this, or whether it even needs to.  Is
> it possible for gdb to construct one of these arrays at runtime?
> 
> Another option, besides changing the representation, would be to teach
> gdb how to create one of the above by recognizing the expressions as a
> kind of special case.  This seemed uglier and more fragile, though.
> 
> For Ada at least, this sort of DWARF isn't the default yet.  The default
> is this Ada-specific name encodings scheme; you have to opt-in to DWARF
> using a compiler command-line flag; but one of the things I'm looking
> into is getting gdb to the point where we can flip the default.
> 
> Andrew> I wonder if the best solution for dealing with signed properties will
> Andrew> be to move away from an over reliance on fetch_address, and instead
> Andrew> come up with a new solution that considers the current type of the
> Andrew> value on the stack, and the type that the value needs to become;
> Andrew> basically a solution built around casting rather than assuming we
> Andrew> always want an address.
> 
> DWARF now has typed expressions as well, so gcc could just emit this
> explicitly, I think.  I didn't look at gcc's dwarf2out.c for this but
> maybe you need -gdwarf-5 to make this happen.

I've seen the typed expressions, but my immediate focus is on
debugging GCC's default DWARF as that's what most folk seem to end up
debugging with.

> 
> Andrew> It is my belief that we can find a suitable default type for every
> Andrew> dynamic property, either specified explicitly in the DWARF spec, or we
> Andrew> can infer an obvious choice if the spec doesn't help us.
> 
> Seems reasonable.
> 
> Andrew> +static struct type *
> Andrew> +dwarf2_per_cu_addr_sized_int_type (struct dwarf2_per_cu_data *per_cu)
> Andrew> +{
> Andrew> +  struct objfile *objfile = per_cu->dwarf2_per_objfile->objfile;
> Andrew> +  int addr_size = dwarf2_per_cu_addr_size (per_cu);
> Andrew> +  struct type *int_type;
> Andrew> +
> Andrew> +  int_type = objfile_type (objfile)->builtin_short;
> Andrew> +  if (int_type != NULL && TYPE_LENGTH (int_type) == addr_size)
> Andrew> +    return int_type;
> 
> dwarf2_per_cu_addr_type handles unsigned char here, but this one does not.
> I doubt it matters but I wonder why the difference.

In the places where this is used the DWARF standard specifies that the
property is signed, so there was no need to handle unsigned.

> 
> Andrew> +/* Return a type that is a generic pointer type, the size of which matches
> Andrew> +   the address size given in the compilation unit header for PER_CU.  */
> Andrew> +static struct type *
> Andrew> +dwarf2_per_cu_addr_type (struct dwarf2_per_cu_data *per_cu)
> Andrew> +{
> Andrew> +  struct objfile *objfile = per_cu->dwarf2_per_objfile->objfile;
> Andrew> +  struct type *void_type = objfile_type (objfile)->builtin_void;
> Andrew> +  struct type *addr_type = lookup_pointer_type (void_type);
> Andrew> +  int addr_size = dwarf2_per_cu_addr_size (per_cu);
> Andrew> +
> Andrew> +  if (TYPE_LENGTH (addr_type) == addr_size)
> Andrew> +    return addr_type;
> Andrew> +
> Andrew> +  /* Yuck! We currently only support one address size per architecture in
> Andrew> +     GDB, which should usually match the address size encoded into the
> Andrew> +     compilation unit header.  However... we have a few tests where this is
> Andrew> +     not the case, these are mostly test cases where the DWARF is hand
> Andrew> +     written and includes a fixed address size, for example 8-bytes.  When
> Andrew> +     we compile these tests on a 32-bit i386 target the gdbarch address
> Andrew> +     size is 4-bytes and the above attempt to create a suitable address
> Andrew> +     type fails.
> Andrew> +
> Andrew> +     As we can't currently create an address type of a different size, we
> Andrew> +     instead substitute an unsigned integer for an address.
> Andrew> +
> Andrew> +     I don't know if there are targets that have signed addresses and if
> Andrew> +     they would need a signed integer here.  I figure we'll handle that
> Andrew> +     case when it presents itself as a problem.  */
> 
> gdbarch.sh makes a distinction between addresses and pointers which, I
> confess, I have never understood.  However, based on this, I think at
> least MIPS may have signed pointers.  From mips-tdep.c:
> 
>   set_gdbarch_pointer_to_address (gdbarch, signed_pointer_to_address);
>   set_gdbarch_address_to_pointer (gdbarch, address_to_signed_pointer);

I've extended dwarf2_per_cu_addr_sized_int_type to create signed or
unsigned types, and made dwarf2_per_cu_addr_type use this, taking the
sign of the default 'void *' type into account.  This should mean that
MIPS will do the right thing now.

> 
> Also, I wonder whether it wouldn't be simpler to just provide a way to
> create a TYPE_CODE_PTR type with a specified size.

I wondered about that, but I was worried that it might open a huge can
of worms that I'd rather not open.  If we did add multiple pointer
sizes later then it's easy to just change dwarf2_per_cu_addr_type to
do the right thing I think.

How would you feel with this version?

Thanks,
Andrew

--

commit 191bf42d636a33c374dbe0b65eb53b7950e49f48
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Wed May 8 13:16:03 2019 +0100

    gdb: Carry default property type around with dynamic properties
    
    This commit is preparation for the next one, with the aim of better
    supporting signed dynamic properties on targets where the address size
    specified in the DWARF headers is smaller than a CORE_ADDR, for
    example debugging an i386 application on x86-64.
    
    Consider this small Fortran program 'bounds.f90':
    
        program test
          integer, allocatable :: array (:)
          allocate (array (-5:5))
          array(3) = 1
        end program test
    
    Compiled with 'gfortran -m32 -g3 -O0 -o bounds bounds.f90'.  The DWARF
    for 'array' looks like this:
    
       <2><97>: Abbrev Number: 10 (DW_TAG_variable)
          <98>   DW_AT_name        : (indirect string, offset: 0x0): array
          <9c>   DW_AT_decl_file   : 1
          <9d>   DW_AT_decl_line   : 2
          <9e>   DW_AT_type        : <0xaf>
          <a2>   DW_AT_location    : 2 byte block: 91 58              (DW_OP_fbreg: -40)
       <2><a5>: Abbrev Number: 11 (DW_TAG_lexical_block)
          <a6>   DW_AT_low_pc      : 0x80485c3
          <aa>   DW_AT_high_pc     : 0x8b
       <2><ae>: Abbrev Number: 0
       <1><af>: Abbrev Number: 12 (DW_TAG_array_type)
          <b0>   DW_AT_data_location: 2 byte block: 97 6              (DW_OP_push_object_address; DW_OP_deref)
          <b3>   DW_AT_allocated   : 4 byte block: 97 6 30 2e         (DW_OP_push_object_address; DW_OP_deref; DW_OP_lit0; DW_OP_ne)
          <b8>   DW_AT_type        : <0x2a>
       <2><bc>: Abbrev Number: 13 (DW_TAG_subrange_type)
          <bd>   DW_AT_lower_bound : 4 byte block: 97 23 10 6         (DW_OP_push_object_address; DW_OP_plus_uconst: 16; DW_OP_deref)
          <c2>   DW_AT_upper_bound : 4 byte block: 97 23 14 6         (DW_OP_push_object_address; DW_OP_plus_uconst: 20; DW_OP_deref)
          <c7>   DW_AT_byte_stride : 6 byte block: 97 23 c 6 34 1e    (DW_OP_push_object_address; DW_OP_plus_uconst: 12; DW_OP_deref; DW_OP_lit4; DW_OP_mul)
       <2><ce>: Abbrev Number: 0
    
    If we look at the DW_AT_lower_bound attribute, which will become a
    dynamic property that GDB evaluates when needed by calling
    dwarf2_evaluate_property.
    
    The process of evaluating a dynamic property requires GDB to execute
    each DW_OP_* operation, the results of these operations is held on a
    stack of 'struct value *'s.
    
    When the entire expression is evaluated the result is on top of the
    stack.
    
    If we look at DW_AT_lower_bound then the last operation is
    DW_OP_deref, this loads a signed address the size of which matches the
    DWARF address size, and so in our i386 on x86-64 situation, the top of
    the stack will be a signed 4-byte value.
    
    The problem is how these values are fetched from the stack.  Currently
    they are always fetched by a call to dwarf_expr_context::fetch_address,
    which converts the value to an unsigned value with a length matching
    the values current length, before converting to a CORE_ADDR.  This
    means we loose the signed nature of the property.
    
    I wonder if the best solution for dealing with signed properties will
    be to move away from an over reliance on fetch_address, and instead
    come up with a new solution that considers the current type of the
    value on the stack, and the type that the value needs to become;
    basically a solution built around casting rather than assuming we
    always want an address.
    
    However, before we can start to even think about moving away from
    fetch_address, there is a more urgent issue to fix, which is we don't
    currently know what type each property should be.  We just hold the
    value of the property in a CORE_ADDR as returned by fetch_address, and
    rely on higher level code (outside of the DWARF expression evaluation
    code) to fix things up for us.  This is what this patch aims to
    address.
    
    When creating a dynamic property (see attr_to_dynamic_prop in
    dwarf2read.c) we can sometimes figure out the type of a property; if
    the property is a reference to another DIE then it will have a
    DW_AT_type attribute.
    
    However, the DW_AT_lower_bound case above isn't a reference to another
    DIE, it's just a DWARF expression.  We don't have any indication for
    what type the property should have.
    
    Luckily, the DWARF spec helps us out, for the lower and upper bounds
    5.13 of the DWARFv5 spec tells us that without any other type
    information the bounds are signed integers the same size as a DWARF
    address.
    
    It is my belief that we can find a suitable default type for every
    dynamic property, either specified explicitly in the DWARF spec, or we
    can infer an obvious choice if the spec doesn't help us.
    
    This commit extends the creation of all dynamic properties to include
    suggesting a suitable default type, all dynamic properties now always
    carry their type around with them.
    
    In later commits we can use this property type to ensure that the
    value we extract from the DWARF stack is handled in a suitable manor
    to correctly maintain its sign extension.
    
    There should be no user visible changes from this commit.  The actual
    fix to correctly support negative array bounds will come later.
    
    gdb/ChangeLog:
    
            * dwarf2loc.c (dwarf2_evaluate_property): Update to take account
            of changes to field names, and use new is_reference field to
            decide if a property is a reference or not.
            * dwarf2loc.h (struct dwarf2_locexpr_baton): Add 'is_reference'
            field.
            (struct dwarf2_property_baton): Update header comment, rename
            'referenced_type' to 'property_type' and update comments.
            * dwarf2read.c (attr_to_dynamic_prop): Add extra parameter to hold
            default property type, store in property baton, update to take
            accound of renamed field.
            (read_func_scope): Update call to attr_to_dynamic_prop.
            (read_array_type): Likewise.
            (dwarf2_per_cu_addr_sized_int_type): New function.
            (read_subrange_index_type): Move type finding code to
            dwarf2_per_cu_addr_sized_int_type.
            (read_subrange_type): Update calls to attr_to_dynamic_prop.
            (dwarf2_per_cu_addr_type): New function.
            (set_die_type): Update calls to attr_to_dynamic_prop.

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index c03f261f1e1..88d34eb8660 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -2443,15 +2443,15 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
       {
 	const struct dwarf2_property_baton *baton
 	  = (const struct dwarf2_property_baton *) prop->data.baton;
+	gdb_assert (baton->property_type != NULL);
 
 	if (dwarf2_locexpr_baton_eval (&baton->locexpr, frame,
 				       addr_stack ? addr_stack->addr : 0,
 				       value))
 	  {
-	    if (baton->referenced_type)
+	    if (baton->locexpr.is_reference)
 	      {
-		struct value *val = value_at (baton->referenced_type, *value);
-
+		struct value *val = value_at (baton->property_type, *value);
 		*value = value_as_address (val);
 	      }
 	    return true;
@@ -2471,7 +2471,7 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 	data = dwarf2_find_location_expression (&baton->loclist, &size, pc);
 	if (data != NULL)
 	  {
-	    val = dwarf2_evaluate_loc_desc (baton->referenced_type, frame, data,
+	    val = dwarf2_evaluate_loc_desc (baton->property_type, frame, data,
 					    size, baton->loclist.per_cu);
 	    if (!value_optimized_out (val))
 	      {
@@ -2497,7 +2497,7 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 	  {
 	    /* This approach lets us avoid checking the qualifiers.  */
 	    if (TYPE_MAIN_TYPE (pinfo->type)
-		== TYPE_MAIN_TYPE (baton->referenced_type))
+		== TYPE_MAIN_TYPE (baton->property_type))
 	      break;
 	  }
 	if (pinfo == NULL)
diff --git a/gdb/dwarf2loc.h b/gdb/dwarf2loc.h
index ac1a771a9f3..baa5762003d 100644
--- a/gdb/dwarf2loc.h
+++ b/gdb/dwarf2loc.h
@@ -183,6 +183,12 @@ struct dwarf2_locexpr_baton
      zero.  */
   size_t size;
 
+  /* When true this location expression is a reference and actually
+     describes the address at which the value of the attribute can be
+     found.  When false the expression provides the value of the attribute
+     directly.  */
+  bool is_reference;
+
   /* The compilation unit containing the symbol whose location
      we're computing.  */
   struct dwarf2_per_cu_data *per_cu;
@@ -228,23 +234,27 @@ struct dwarf2_offset_baton
 
 /* A dynamic property is either expressed as a single location expression
    or a location list.  If the property is an indirection, pointing to
-   another die, keep track of the targeted type in REFERENCED_TYPE.  */
+   another die, keep track of the targeted type in PROPERTY_TYPE.
+   Alternatively, if the property location gives the property value
+   directly then it will have PROPERTY_TYPE.  */
 
 struct dwarf2_property_baton
 {
   /* If the property is an indirection, we need to evaluate the location
-     in the context of the type REFERENCED_TYPE.
-     If NULL, the location is the actual value of the property.  */
-  struct type *referenced_type;
+     in the context of the type PROPERTY_TYPE.  If the property is supplied
+     by value then it will be of PROPERTY_TYPE.  This field should never be
+     NULL.  */
+  struct type *property_type;
   union
   {
-    /* Location expression.  */
+    /* Location expression either evaluated in the context of
+       PROPERTY_TYPE, or a value of type PROPERTY_TYPE.  */
     struct dwarf2_locexpr_baton locexpr;
 
-    /* Location list to be evaluated in the context of REFERENCED_TYPE.  */
+    /* Location list to be evaluated in the context of PROPERTY_TYPE.  */
     struct dwarf2_loclist_baton loclist;
 
-    /* The location is an offset to REFERENCED_TYPE.  */
+    /* The location is an offset to PROPERTY_TYPE.  */
     struct dwarf2_offset_baton offset_info;
   };
 };
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index fc3d888fadc..e8d1e3ced18 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1833,7 +1833,7 @@ static void read_signatured_type (struct signatured_type *);
 
 static int attr_to_dynamic_prop (const struct attribute *attr,
 				 struct die_info *die, struct dwarf2_cu *cu,
-				 struct dynamic_prop *prop);
+				 struct dynamic_prop *prop, struct type *type);
 
 /* memory allocation interface */
 
@@ -1913,6 +1913,10 @@ static void queue_comp_unit (struct dwarf2_per_cu_data *per_cu,
 
 static void process_queue (struct dwarf2_per_objfile *dwarf2_per_objfile);
 
+static struct type *dwarf2_per_cu_addr_type (struct dwarf2_per_cu_data *per_cu);
+static struct type *dwarf2_per_cu_addr_sized_int_type
+	(struct dwarf2_per_cu_data *per_cu, bool unsigned_p);
+
 /* Class, the destructor of which frees all allocated queue entries.  This
    will only have work to do if an error was thrown while processing the
    dwarf.  If no error was thrown then the queue entries should have all
@@ -13759,7 +13763,8 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
     {
       newobj->static_link
 	= XOBNEW (&objfile->objfile_obstack, struct dynamic_prop);
-      attr_to_dynamic_prop (attr, die, cu, newobj->static_link);
+      attr_to_dynamic_prop (attr, die, cu, newobj->static_link,
+			    dwarf2_per_cu_addr_type (cu->per_cu));
     }
 
   cu->list_in_scope = cu->get_builder ()->get_local_symbols ();
@@ -16515,10 +16520,13 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
   if (attr != NULL)
     {
       int stride_ok;
+      struct type *prop_type
+	= dwarf2_per_cu_addr_sized_int_type (cu->per_cu, false);
 
       byte_stride_prop
 	= (struct dynamic_prop *) alloca (sizeof (struct dynamic_prop));
-      stride_ok = attr_to_dynamic_prop (attr, die, cu, byte_stride_prop);
+      stride_ok = attr_to_dynamic_prop (attr, die, cu, byte_stride_prop,
+					prop_type);
       if (!stride_ok)
 	{
 	  complaint (_("unable to read array DW_AT_byte_stride "
@@ -17733,22 +17741,26 @@ read_base_type (struct die_info *die, struct dwarf2_cu *cu)
 
 static int
 attr_to_dynamic_prop (const struct attribute *attr, struct die_info *die,
-		      struct dwarf2_cu *cu, struct dynamic_prop *prop)
+		      struct dwarf2_cu *cu, struct dynamic_prop *prop,
+		      struct type *default_type)
 {
   struct dwarf2_property_baton *baton;
   struct obstack *obstack
     = &cu->per_cu->dwarf2_per_objfile->objfile->objfile_obstack;
 
+  gdb_assert (default_type != NULL);
+
   if (attr == NULL || prop == NULL)
     return 0;
 
   if (attr_form_is_block (attr))
     {
       baton = XOBNEW (obstack, struct dwarf2_property_baton);
-      baton->referenced_type = NULL;
+      baton->property_type = default_type;
       baton->locexpr.per_cu = cu->per_cu;
       baton->locexpr.size = DW_BLOCK (attr)->size;
       baton->locexpr.data = DW_BLOCK (attr)->data;
+      baton->locexpr.is_reference = false;
       prop->data.baton = baton;
       prop->kind = PROP_LOCEXPR;
       gdb_assert (prop->data.baton != NULL);
@@ -17773,7 +17785,7 @@ attr_to_dynamic_prop (const struct attribute *attr, struct die_info *die,
 	    if (attr_form_is_section_offset (target_attr))
 	      {
 		baton = XOBNEW (obstack, struct dwarf2_property_baton);
-		baton->referenced_type = die_type (target_die, target_cu);
+		baton->property_type = die_type (target_die, target_cu);
 		fill_in_loclist_baton (cu, &baton->loclist, target_attr);
 		prop->data.baton = baton;
 		prop->kind = PROP_LOCLIST;
@@ -17782,10 +17794,11 @@ attr_to_dynamic_prop (const struct attribute *attr, struct die_info *die,
 	    else if (attr_form_is_block (target_attr))
 	      {
 		baton = XOBNEW (obstack, struct dwarf2_property_baton);
-		baton->referenced_type = die_type (target_die, target_cu);
+		baton->property_type = die_type (target_die, target_cu);
 		baton->locexpr.per_cu = cu->per_cu;
 		baton->locexpr.size = DW_BLOCK (target_attr)->size;
 		baton->locexpr.data = DW_BLOCK (target_attr)->data;
+		baton->locexpr.is_reference = true;
 		prop->data.baton = baton;
 		prop->kind = PROP_LOCEXPR;
 		gdb_assert (prop->data.baton != NULL);
@@ -17806,7 +17819,7 @@ attr_to_dynamic_prop (const struct attribute *attr, struct die_info *die,
 		return 0;
 
 	      baton = XOBNEW (obstack, struct dwarf2_property_baton);
-	      baton->referenced_type = read_type_die (target_die->parent,
+	      baton->property_type = read_type_die (target_die->parent,
 						      target_cu);
 	      baton->offset_info.offset = offset;
 	      baton->offset_info.type = die_type (target_die, target_cu);
@@ -17831,6 +17844,37 @@ attr_to_dynamic_prop (const struct attribute *attr, struct die_info *die,
   return 1;
 }
 
+/* Find an integer type the same size as the address size given in the
+   compilation unit header for PER_CU.  UNSIGNED_P controls if the integer
+   is unsigned or not.  */
+
+static struct type *
+dwarf2_per_cu_addr_sized_int_type (struct dwarf2_per_cu_data *per_cu,
+				   bool unsigned_p)
+{
+  struct objfile *objfile = per_cu->dwarf2_per_objfile->objfile;
+  int addr_size = dwarf2_per_cu_addr_size (per_cu);
+  struct type *int_type;
+
+  /* Helper macro to examine the various builtin types.  */
+#define TRY_TYPE(F)						\
+  int_type = (unsigned_p					\
+	      ? objfile_type (objfile)->builtin_unsigned_ ## F	\
+	      : objfile_type (objfile)->builtin_ ## F);		\
+  if (int_type != NULL && TYPE_LENGTH (int_type) == addr_size)	\
+    return int_type
+
+  TRY_TYPE (char);
+  TRY_TYPE (short);
+  TRY_TYPE (int);
+  TRY_TYPE (long);
+  TRY_TYPE (long_long);
+
+#undef TRY_TYPE
+
+  gdb_assert_not_reached ("unable to find suitable integer type");
+}
+
 /* Read the DW_AT_type attribute for a sub-range.  If this attribute is not
    present (which is valid) then compute the default type based on the
    compilation units address size.  */
@@ -17853,30 +17897,7 @@ read_subrange_index_type (struct die_info *die, struct dwarf2_cu *cu)
      FIXME: muller/2010-05-28: Possible references to object for low bound,
      high bound or count are not yet handled by this code.  */
   if (TYPE_CODE (index_type) == TYPE_CODE_VOID)
-    {
-      struct objfile *objfile = cu->per_cu->dwarf2_per_objfile->objfile;
-      struct gdbarch *gdbarch = get_objfile_arch (objfile);
-      int addr_size = gdbarch_addr_bit (gdbarch) /8;
-      struct type *int_type = objfile_type (objfile)->builtin_int;
-
-      /* Test "int", "long int", and "long long int" objfile types,
-	 and select the first one having a size above or equal to the
-	 architecture address size.  */
-      if (int_type && TYPE_LENGTH (int_type) >= addr_size)
-	index_type = int_type;
-      else
-	{
-	  int_type = objfile_type (objfile)->builtin_long;
-	  if (int_type && TYPE_LENGTH (int_type) >= addr_size)
-	    index_type = int_type;
-	  else
-	    {
-	      int_type = objfile_type (objfile)->builtin_long_long;
-	      if (int_type && TYPE_LENGTH (int_type) >= addr_size)
-		index_type = int_type;
-	    }
-	}
-    }
+    index_type = dwarf2_per_cu_addr_sized_int_type (cu->per_cu, false);
 
   return index_type;
 }
@@ -17945,7 +17966,7 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
 
   attr = dwarf2_attr (die, DW_AT_lower_bound, cu);
   if (attr)
-    attr_to_dynamic_prop (attr, die, cu, &low);
+    attr_to_dynamic_prop (attr, die, cu, &low, base_type);
   else if (!low_default_is_valid)
     complaint (_("Missing DW_AT_lower_bound "
 				      "- DIE at %s [in module %s]"),
@@ -17954,10 +17975,10 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
 
   struct attribute *attr_ub, *attr_count;
   attr = attr_ub = dwarf2_attr (die, DW_AT_upper_bound, cu);
-  if (!attr_to_dynamic_prop (attr, die, cu, &high))
+  if (!attr_to_dynamic_prop (attr, die, cu, &high, base_type))
     {
       attr = attr_count = dwarf2_attr (die, DW_AT_count, cu);
-      if (attr_to_dynamic_prop (attr, die, cu, &high))
+      if (attr_to_dynamic_prop (attr, die, cu, &high, base_type))
 	{
 	  /* If bounds are constant do the final calculation here.  */
 	  if (low.kind == PROP_CONST && high.kind == PROP_CONST)
@@ -25310,6 +25331,24 @@ dwarf2_per_cu_text_offset (struct dwarf2_per_cu_data *per_cu)
   return ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
 }
 
+/* Return a type that is a generic pointer type, the size of which matches
+   the address size given in the compilation unit header for PER_CU.  */
+static struct type *
+dwarf2_per_cu_addr_type (struct dwarf2_per_cu_data *per_cu)
+{
+  struct objfile *objfile = per_cu->dwarf2_per_objfile->objfile;
+  struct type *void_type = objfile_type (objfile)->builtin_void;
+  struct type *addr_type = lookup_pointer_type (void_type);
+  int addr_size = dwarf2_per_cu_addr_size (per_cu);
+
+  if (TYPE_LENGTH (addr_type) == addr_size)
+    return addr_type;
+
+  addr_type
+    = dwarf2_per_cu_addr_sized_int_type (per_cu, TYPE_UNSIGNED (addr_type));
+  return addr_type;
+}
+
 /* Return DWARF version number of PER_CU.  */
 
 short
@@ -25576,7 +25615,9 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
   attr = dwarf2_attr (die, DW_AT_allocated, cu);
   if (attr_form_is_block (attr))
     {
-      if (attr_to_dynamic_prop (attr, die, cu, &prop))
+      struct type *prop_type
+	= dwarf2_per_cu_addr_sized_int_type (cu->per_cu, false);
+      if (attr_to_dynamic_prop (attr, die, cu, &prop, prop_type))
         add_dyn_prop (DYN_PROP_ALLOCATED, prop, type);
     }
   else if (attr != NULL)
@@ -25590,7 +25631,9 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
   attr = dwarf2_attr (die, DW_AT_associated, cu);
   if (attr_form_is_block (attr))
     {
-      if (attr_to_dynamic_prop (attr, die, cu, &prop))
+      struct type *prop_type
+	= dwarf2_per_cu_addr_sized_int_type (cu->per_cu, false);
+      if (attr_to_dynamic_prop (attr, die, cu, &prop, prop_type))
         add_dyn_prop (DYN_PROP_ASSOCIATED, prop, type);
     }
   else if (attr != NULL)
@@ -25602,7 +25645,8 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
 
   /* Read DW_AT_data_location and set in type.  */
   attr = dwarf2_attr (die, DW_AT_data_location, cu);
-  if (attr_to_dynamic_prop (attr, die, cu, &prop))
+  if (attr_to_dynamic_prop (attr, die, cu, &prop,
+			    dwarf2_per_cu_addr_type (cu->per_cu)))
     add_dyn_prop (DYN_PROP_DATA_LOCATION, prop, type);
 
   if (dwarf2_per_objfile->die_type_hash == NULL)


^ permalink raw reply	[flat|nested] 20+ messages in thread

[parent not found: <20190710141321.GL23204@embecosm.com>]

* Re: [PATCHv2 4/5] gdb: Carry default property type around with dynamic properties
       [not found]         ` <20190710141321.GL23204@embecosm.com>
@ 2019-07-10 15:06           ` Tom Tromey
  0 siblings, 0 replies; 20+ messages in thread
From: Tom Tromey @ 2019-07-10 15:06 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: Tom Tromey, gdb-patches

Andrew> Was there any additional feedback on this series?

Nope, sorry about that!  I think it is fine.

Tom


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCHv2 2/5] gdb: Convert dwarf2_evaluate_property to return bool
  2019-05-05 20:57 ` [PATCH 0/3] " Andrew Burgess
                     ` (4 preceding siblings ...)
  2019-05-09 22:22   ` [PATCHv2 4/5] gdb: Carry default property type around with dynamic properties Andrew Burgess
@ 2019-05-09 22:22   ` Andrew Burgess
  2019-05-09 22:22   ` [PATCHv2 1/5] gdb: Update type of lower bound in value_subscripted_rvalue Andrew Burgess
  2019-05-09 22:22   ` [PATCHv2 3/5] gdb/dwarf: Ensure the target type of ranges is not void Andrew Burgess
  7 siblings, 0 replies; 20+ messages in thread
From: Andrew Burgess @ 2019-05-09 22:22 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

Convert dwarf2_evaluate_property to return a bool, there should be no
user visible change after this commit.

gdb/ChangeLog:

	* dwarf2loc.c (dwarf2_evaluate_property): Change return type, and
	update return statements.
	* dwarf2loc.h (dwarf2_evaluate_property): Update return type on
	declaration, and update comment to match.
	* gdbtypes.c (resolve_dynamic_array): Update call to
	dwarf2_evaluate_property to match new return type.
---
 gdb/ChangeLog   |  9 +++++++++
 gdb/dwarf2loc.c | 14 +++++++-------
 gdb/dwarf2loc.h | 12 ++++++------
 gdb/gdbtypes.c  |  5 +----
 4 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 37cda40ecf3..c03f261f1e1 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -2425,14 +2425,14 @@ dwarf2_locexpr_baton_eval (const struct dwarf2_locexpr_baton *dlbaton,
 
 /* See dwarf2loc.h.  */
 
-int
+bool
 dwarf2_evaluate_property (const struct dynamic_prop *prop,
 			  struct frame_info *frame,
 			  struct property_addr_info *addr_stack,
 			  CORE_ADDR *value)
 {
   if (prop == NULL)
-    return 0;
+    return false;
 
   if (frame == NULL && has_stack_frames ())
     frame = get_selected_frame (NULL);
@@ -2454,7 +2454,7 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 
 		*value = value_as_address (val);
 	      }
-	    return 1;
+	    return true;
 	  }
       }
       break;
@@ -2476,7 +2476,7 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 	    if (!value_optimized_out (val))
 	      {
 		*value = value_as_address (val);
-		return 1;
+		return true;
 	      }
 	  }
       }
@@ -2484,7 +2484,7 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 
     case PROP_CONST:
       *value = prop->data.const_val;
-      return 1;
+      return true;
 
     case PROP_ADDR_OFFSET:
       {
@@ -2510,11 +2510,11 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,
 	  val = value_at (baton->offset_info.type,
 			  pinfo->addr + baton->offset_info.offset);
 	*value = value_as_address (val);
-	return 1;
+	return true;
       }
     }
 
-  return 0;
+  return false;
 }
 
 /* See dwarf2loc.h.  */
diff --git a/gdb/dwarf2loc.h b/gdb/dwarf2loc.h
index 955e6f1b48a..ac1a771a9f3 100644
--- a/gdb/dwarf2loc.h
+++ b/gdb/dwarf2loc.h
@@ -135,13 +135,13 @@ struct property_addr_info
    property. When evaluating a property that is not related to a type, it can
    be NULL.
 
-   Returns 1 if PROP could be converted and the static value is passed back
-   into VALUE, otherwise returns 0.  */
+   Returns true if PROP could be converted and the static value is passed
+   back into VALUE, otherwise returns false.  */
 
-int dwarf2_evaluate_property (const struct dynamic_prop *prop,
-			      struct frame_info *frame,
-			      struct property_addr_info *addr_stack,
-			      CORE_ADDR *value);
+bool dwarf2_evaluate_property (const struct dynamic_prop *prop,
+			       struct frame_info *frame,
+			       struct property_addr_info *addr_stack,
+			       CORE_ADDR *value);
 
 /* A helper for the compiler interface that compiles a single dynamic
    property to C code.
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 59456f9f3a4..ec4c4783a60 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -2065,10 +2065,7 @@ resolve_dynamic_array (struct type *type,
   prop = get_dyn_prop (DYN_PROP_BYTE_STRIDE, type);
   if (prop != NULL)
     {
-      int prop_eval_ok
-	= dwarf2_evaluate_property (prop, NULL, addr_stack, &value);
-
-      if (prop_eval_ok)
+      if (dwarf2_evaluate_property (prop, NULL, addr_stack, &value))
 	{
 	  remove_dyn_prop (DYN_PROP_BYTE_STRIDE, type);
 	  bit_stride = (unsigned int) (value * 8);
-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCHv2 1/5] gdb: Update type of lower bound in value_subscripted_rvalue
  2019-05-05 20:57 ` [PATCH 0/3] " Andrew Burgess
                     ` (5 preceding siblings ...)
  2019-05-09 22:22   ` [PATCHv2 2/5] gdb: Convert dwarf2_evaluate_property to return bool Andrew Burgess
@ 2019-05-09 22:22   ` Andrew Burgess
  2019-05-09 22:22   ` [PATCHv2 3/5] gdb/dwarf: Ensure the target type of ranges is not void Andrew Burgess
  7 siblings, 0 replies; 20+ messages in thread
From: Andrew Burgess @ 2019-05-09 22:22 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

The dynamic lower (and upper) bounds of ranges are stored as type
LONGEST (see union dynamic_prop_data in gdbtypes.h).  In most places
that range bounds are handled they are held in a LONGEST, however in
value_subscripted_rvalue the bound is placed into an int.

This commit changes value_subscripted_rvalue to use LONGEST, there
should be no user visible changes after this commit.

gdb/ChangeLog:

	* valarith.c (value_subscripted_rvalue): Change lowerbound
	parameter type from int to LONGEST.
	* value.h (value_subscripted_rvalue): Likewise in declaration.
---
 gdb/ChangeLog  | 6 ++++++
 gdb/valarith.c | 2 +-
 gdb/value.h    | 3 ++-
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/gdb/valarith.c b/gdb/valarith.c
index 8d310b504a2..f4372123e67 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -182,7 +182,7 @@ value_subscript (struct value *array, LONGEST index)
    to doubles, but no longer does.  */
 
 struct value *
-value_subscripted_rvalue (struct value *array, LONGEST index, int lowerbound)
+value_subscripted_rvalue (struct value *array, LONGEST index, LONGEST lowerbound)
 {
   struct type *array_type = check_typedef (value_type (array));
   struct type *elt_type = check_typedef (TYPE_TARGET_TYPE (array_type));
diff --git a/gdb/value.h b/gdb/value.h
index 0756d13b6d7..d505dc9af06 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -1149,7 +1149,8 @@ extern struct value *find_function_in_inferior (const char *,
 extern struct value *value_allocate_space_in_inferior (int);
 
 extern struct value *value_subscripted_rvalue (struct value *array,
-					       LONGEST index, int lowerbound);
+					       LONGEST index,
+					       LONGEST lowerbound);
 
 /* User function handler.  */
 
-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCHv2 3/5] gdb/dwarf: Ensure the target type of ranges is not void
  2019-05-05 20:57 ` [PATCH 0/3] " Andrew Burgess
                     ` (6 preceding siblings ...)
  2019-05-09 22:22   ` [PATCHv2 1/5] gdb: Update type of lower bound in value_subscripted_rvalue Andrew Burgess
@ 2019-05-09 22:22   ` Andrew Burgess
  2019-05-22 18:36     ` Tom Tromey
  7 siblings, 1 reply; 20+ messages in thread
From: Andrew Burgess @ 2019-05-09 22:22 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

If a DW_TAG_subrange_type DWARF entry has no DW_AT_type then a default
type based on the size of an address on the current target is assumed.
We store this type as the target type for GDB's range types.

Currently GDB can create ranges for which the target type is VOID,
this is incorrect but seems to cause no problems. I believe the reason
this doesn't cause any issues is because the languages (for example
Ada) that actually make use of a ranges target type also have
compilers that generate DWARF that includes a DW_AT_type attribute.

However, gfortran does not include a DW_AT_type, its DWARF instead
relies on the default target type.  This isn't currently a problem for
GDB as gfortran doesn't make use of the target type when printing
subranges, but it shouldn't hurt to fix this issue now.

I've added an assert into create_range_type that will catch this issue
if it comes up again.

This was tested on an x86-64/GNU-Linux machine with both the Ada and
gfortran compilers available with both '--target_board=unix' and
'--target_board=unix/-m32'.  There are no user visible changes after
this commit.

gdb/ChangeLog:

	* dwarf2read.c (read_subrange_index_type): New function.
	(read_subrange_type): Move code into new function and call it.
	* gdbtypes.c (create_range_type): Add some asserts.
---
 gdb/ChangeLog    |  6 ++++
 gdb/dwarf2read.c | 92 ++++++++++++++++++++++++++++++++------------------------
 gdb/gdbtypes.c   |  5 +++
 3 files changed, 63 insertions(+), 40 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index b29c089606d..e9c53148961 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -17809,6 +17809,56 @@ attr_to_dynamic_prop (const struct attribute *attr, struct die_info *die,
   return 1;
 }
 
+/* Read the DW_AT_type attribute for a sub-range.  If this attribute is not
+   present (which is valid) then compute the default type based on the
+   compilation units address size.  */
+
+static struct type *
+read_subrange_index_type (struct die_info *die, struct dwarf2_cu *cu)
+{
+  struct type *index_type = die_type (die, cu);
+
+  /* Dwarf-2 specifications explicitly allows to create subrange types
+     without specifying a base type.
+     In that case, the base type must be set to the type of
+     the lower bound, upper bound or count, in that order, if any of these
+     three attributes references an object that has a type.
+     If no base type is found, the Dwarf-2 specifications say that
+     a signed integer type of size equal to the size of an address should
+     be used.
+     For the following C code: `extern char gdb_int [];'
+     GCC produces an empty range DIE.
+     FIXME: muller/2010-05-28: Possible references to object for low bound,
+     high bound or count are not yet handled by this code.  */
+  if (TYPE_CODE (index_type) == TYPE_CODE_VOID)
+    {
+      struct objfile *objfile = cu->per_cu->dwarf2_per_objfile->objfile;
+      struct gdbarch *gdbarch = get_objfile_arch (objfile);
+      int addr_size = gdbarch_addr_bit (gdbarch) /8;
+      struct type *int_type = objfile_type (objfile)->builtin_int;
+
+      /* Test "int", "long int", and "long long int" objfile types,
+	 and select the first one having a size above or equal to the
+	 architecture address size.  */
+      if (int_type && TYPE_LENGTH (int_type) >= addr_size)
+	index_type = int_type;
+      else
+	{
+	  int_type = objfile_type (objfile)->builtin_long;
+	  if (int_type && TYPE_LENGTH (int_type) >= addr_size)
+	    index_type = int_type;
+	  else
+	    {
+	      int_type = objfile_type (objfile)->builtin_long_long;
+	      if (int_type && TYPE_LENGTH (int_type) >= addr_size)
+		index_type = int_type;
+	    }
+	}
+    }
+
+  return index_type;
+}
+
 /* Read the given DW_AT_subrange DIE.  */
 
 static struct type *
@@ -17823,7 +17873,8 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
   const char *name;
   ULONGEST negative_mask;
 
-  orig_base_type = die_type (die, cu);
+  orig_base_type = read_subrange_index_type (die, cu);
+
   /* If ORIG_BASE_TYPE is a typedef, it will not be TYPE_UNSIGNED,
      whereas the real type might be.  So, we use ORIG_BASE_TYPE when
      creating the range type, but we use the result of check_typedef
@@ -17905,45 +17956,6 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
 		       sect_offset_str (die->sect_off),
 		       objfile_name (cu->per_cu->dwarf2_per_objfile->objfile));
 	}
-	
-    }
-
-  /* Dwarf-2 specifications explicitly allows to create subrange types
-     without specifying a base type.
-     In that case, the base type must be set to the type of
-     the lower bound, upper bound or count, in that order, if any of these
-     three attributes references an object that has a type.
-     If no base type is found, the Dwarf-2 specifications say that
-     a signed integer type of size equal to the size of an address should
-     be used.
-     For the following C code: `extern char gdb_int [];'
-     GCC produces an empty range DIE.
-     FIXME: muller/2010-05-28: Possible references to object for low bound,
-     high bound or count are not yet handled by this code.  */
-  if (TYPE_CODE (base_type) == TYPE_CODE_VOID)
-    {
-      struct objfile *objfile = cu->per_cu->dwarf2_per_objfile->objfile;
-      struct gdbarch *gdbarch = get_objfile_arch (objfile);
-      int addr_size = gdbarch_addr_bit (gdbarch) /8;
-      struct type *int_type = objfile_type (objfile)->builtin_int;
-
-      /* Test "int", "long int", and "long long int" objfile types,
-	 and select the first one having a size above or equal to the
-	 architecture address size.  */
-      if (int_type && TYPE_LENGTH (int_type) >= addr_size)
-	base_type = int_type;
-      else
-	{
-	  int_type = objfile_type (objfile)->builtin_long;
-	  if (int_type && TYPE_LENGTH (int_type) >= addr_size)
-	    base_type = int_type;
-	  else
-	    {
-	      int_type = objfile_type (objfile)->builtin_long_long;
-	      if (int_type && TYPE_LENGTH (int_type) >= addr_size)
-		base_type = int_type;
-	    }
-	}
     }
 
   /* Normally, the DWARF producers are expected to use a signed
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index ec4c4783a60..c6e8aca451b 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -914,6 +914,11 @@ create_range_type (struct type *result_type, struct type *index_type,
 		   const struct dynamic_prop *low_bound,
 		   const struct dynamic_prop *high_bound)
 {
+  /* The INDEX_TYPE should be a type capable of holding the upper and lower
+     bounds, as such a zero sized, or void type makes no sense.  */
+  gdb_assert (TYPE_CODE (index_type) != TYPE_CODE_VOID);
+  gdb_assert (TYPE_LENGTH (index_type) > 0);
+
   if (result_type == NULL)
     result_type = alloc_type_copy (index_type);
   TYPE_CODE (result_type) = TYPE_CODE_RANGE;
-- 
2.14.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCHv2 3/5] gdb/dwarf: Ensure the target type of ranges is not void
  2019-05-09 22:22   ` [PATCHv2 3/5] gdb/dwarf: Ensure the target type of ranges is not void Andrew Burgess
@ 2019-05-22 18:36     ` Tom Tromey
  0 siblings, 0 replies; 20+ messages in thread
From: Tom Tromey @ 2019-05-22 18:36 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> gdb/ChangeLog:

Andrew> 	* dwarf2read.c (read_subrange_index_type): New function.
Andrew> 	(read_subrange_type): Move code into new function and call it.
Andrew> 	* gdbtypes.c (create_range_type): Add some asserts.

Thanks, this looks good to me.

Tom

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2019-07-10 15:06 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-09 22:22 [PATCHv2 0/5] Improve handling of negative dynamic properties Andrew Burgess
2019-05-05 20:57 ` [PATCH 0/3] " Andrew Burgess
2019-05-05 20:57   ` [PATCH 2/3] gdb: Convert dwarf2_evaluate_property to return bool Andrew Burgess
2019-05-06 13:57     ` Tom Tromey
2019-05-05 20:57   ` [PATCH 3/3] gdb: Handle dynamic properties with negative values Andrew Burgess
2019-05-06 14:55     ` Tom Tromey
2019-05-05 20:57   ` [PATCH 1/3] gdb: Update type of lower bound in value_subscripted_rvalue Andrew Burgess
2019-05-06 13:57     ` Tom Tromey
2019-05-09 22:22   ` [PATCHv2 5/5] gdb: Better support for dynamic properties with negative values Andrew Burgess
2019-05-22 19:37     ` Tom Tromey
2019-06-10 22:17       ` Andrew Burgess
2019-07-10 15:03         ` Tom Tromey
2019-05-09 22:22   ` [PATCHv2 4/5] gdb: Carry default property type around with dynamic properties Andrew Burgess
2019-05-22 19:05     ` Tom Tromey
2019-06-10 22:29       ` Andrew Burgess
     [not found]         ` <20190710141321.GL23204@embecosm.com>
2019-07-10 15:06           ` Tom Tromey
2019-05-09 22:22   ` [PATCHv2 2/5] gdb: Convert dwarf2_evaluate_property to return bool Andrew Burgess
2019-05-09 22:22   ` [PATCHv2 1/5] gdb: Update type of lower bound in value_subscripted_rvalue Andrew Burgess
2019-05-09 22:22   ` [PATCHv2 3/5] gdb/dwarf: Ensure the target type of ranges is not void Andrew Burgess
2019-05-22 18:36     ` Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox