[PATCHv2,5/5] gdb: Better support for dynamic properties with negative values

Message ID 672dc208d6dfb9f068eb635e02bf85abb9630c74.1557439866.git.andrew.burgess@embecosm.com
State New
Headers show
Series
  • Improve handling of negative dynamic properties
Related show

Commit Message

Andrew Burgess May 9, 2019, 10:22 p.m.
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(+)

-- 
2.14.5

Comments

Tom Tromey May 22, 2019, 7:19 p.m. | #1
>>>>> "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
Andrew Burgess June 10, 2019, 10:17 p.m. | #2
* 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
Tom Tromey July 10, 2019, 3:02 p.m. | #3
>>>>> "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

Patch

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