[01/11] Dwarf: Fix dynamic properties with neg. value.

Message ID 20181127183139.71170-2-sbasierski@pl.sii.eu
State New
Headers show
Series
  • Adds functionality and fixes some code
Related show

Commit Message

Sebastian Basierski Nov. 27, 2018, 6:31 p.m.
From: Bernhard Heckel <bernhard.heckel@intel.com>


Evaluating of neg. value of 32bit inferiours running on 64bit plattform
causes issues because of the missing sign bits.

Bernhard Heckel  <bernhard.heckel@intel.com>

gdb/Changelog
	* dwarf2loc.h: Declare
	* dwarf2loc.c (dwarf2_evaluate_property_signed): New.
	  (dwarf2_evaluate_property): Delegate tasks to
	  dwarf2_evaluate_property_signed.
---
 gdb/dwarf2loc.c | 44 +++++++++++++++++++++++++++++++++++---------
 gdb/dwarf2loc.h |  7 +++++++
 2 files changed, 42 insertions(+), 9 deletions(-)

-- 
2.17.1

Comments

Andrew Burgess March 2, 2019, 6:23 p.m. | #1
* Sebastian Basierski <sbasierski@pl.sii.eu> [2018-11-27 19:31:29 +0100]:

> From: Bernhard Heckel <bernhard.heckel@intel.com>

> 

> Evaluating of neg. value of 32bit inferiours running on 64bit plattform

> causes issues because of the missing sign bits.

> 

> Bernhard Heckel  <bernhard.heckel@intel.com>

> 

> gdb/Changelog

> 	* dwarf2loc.h: Declare

> 	* dwarf2loc.c (dwarf2_evaluate_property_signed): New.

> 	  (dwarf2_evaluate_property): Delegate tasks to

> 	  dwarf2_evaluate_property_signed.

> ---

>  gdb/dwarf2loc.c | 44 +++++++++++++++++++++++++++++++++++---------

>  gdb/dwarf2loc.h |  7 +++++++

>  2 files changed, 42 insertions(+), 9 deletions(-)

> 

> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c

> index ee6a8e277c..c94bdc60f2 100644

> --- a/gdb/dwarf2loc.c

> +++ b/gdb/dwarf2loc.c

> @@ -2659,11 +2659,13 @@ dwarf2_locexpr_baton_eval (const struct dwarf2_locexpr_baton *dlbaton,

>  /* See dwarf2loc.h.  */

>  

>  int

> -dwarf2_evaluate_property (const struct dynamic_prop *prop,

> -			  struct frame_info *frame,

> -			  struct property_addr_info *addr_stack,

> -			  CORE_ADDR *value)

> +dwarf2_evaluate_property_signed (const struct dynamic_prop *prop,

> +				 struct frame_info *frame,

> +				 struct property_addr_info *addr_stack,

> +				 CORE_ADDR *value, int is_signed)


I don't like this renaming, the function now has a '_signed' suffix,
but also now takes a flag 'is_signed', there seems to be some
confusion here.

Additional 'is_signed' should be a bool.

>  {

> +  int rc = 0;

> +

>    if (prop == NULL)

>      return 0;

>  

> @@ -2687,7 +2689,7 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,

>  

>  		*value = value_as_address (val);

>  	      }

> -	    return 1;

> +	    rc = 1;

>  	  }

>        }

>        break;

> @@ -2709,7 +2711,7 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,

>  	    if (!value_optimized_out (val))

>  	      {

>  		*value = value_as_address (val);

> -		return 1;

> +		rc = 1;

>  	      }

>  	  }

>        }

> @@ -2717,7 +2719,8 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop,

>  

>      case PROP_CONST:

>        *value = prop->data.const_val;

> -      return 1;

> +      rc = 1;

> +      break;

>  

>      case PROP_ADDR_OFFSET:

>        {

> @@ -2739,11 +2742,34 @@ 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;

> +	rc = 1;

>        }

> +      break;

>      }

>  

> -  return 0;

> +  if (rc == 1 && is_signed == 1)

> +    {

> +      /* 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 ();


Getting the gdbarch from the frame would probably be better.

> +      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 );


I notice that there's no tests included in this patch, I tried the
entire series with this new code commented out (except for patch #9
which wouldn't merge for me) and I couldn't see any failures (I only
ran the gdb.fortran/*.exp tests though, so its not clear if this code
is tested at all.

What I do see is the new code triggering (that is sign extending a
value) but this doesn't seem to be required.  I haven't dug into why
this doesn't make a difference yet though.

FYI I tried running these tests using 'gfortran -m32' on an x86-64
machine, which I think is the setup that you're expecting to see
failures on.

Ideally I like to see this new code covered with a test.

> +    }

> +  return rc;

> +}

> +

> +int

> +dwarf2_evaluate_property (const struct dynamic_prop *prop,

> +			  struct frame_info *frame,

> +			  struct property_addr_info *addr_stack,

> +			  CORE_ADDR *value)

> +{

> +  return dwarf2_evaluate_property_signed (prop, frame, addr_stack, value, 0);

>  }


The 'dwarf2_evaluate_property' function isn't used that often, I think
it might be worth sticking with the existing
'dwarf2_evaluate_property' name, and just adding the 'is_signed' flag
there, then update all of the users, that way users are forced to
think about what the correct value for the flag should be.

>  

>  /* See dwarf2loc.h.  */

> diff --git a/gdb/dwarf2loc.h b/gdb/dwarf2loc.h

> index d7a56db05d..408e1904cd 100644

> --- a/gdb/dwarf2loc.h

> +++ b/gdb/dwarf2loc.h

> @@ -143,6 +143,13 @@ int dwarf2_evaluate_property (const struct dynamic_prop *prop,

>  			      struct property_addr_info *addr_stack,

>  			      CORE_ADDR *value);

>  

> +int dwarf2_evaluate_property_signed (const struct dynamic_prop *prop,

> +			      struct frame_info *frame,

> +			      struct property_addr_info *addr_stack,

> +			      CORE_ADDR *value,

> +			      int is_signed);


I don't think this will be needed, but new functions should have a
header comment.

Thanks,
Andrew

> +

> +

>  /* A helper for the compiler interface that compiles a single dynamic

>     property to C code.

>  

> -- 

> 2.17.1

>

Patch

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index ee6a8e277c..c94bdc60f2 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -2659,11 +2659,13 @@  dwarf2_locexpr_baton_eval (const struct dwarf2_locexpr_baton *dlbaton,
 /* See dwarf2loc.h.  */
 
 int
-dwarf2_evaluate_property (const struct dynamic_prop *prop,
-			  struct frame_info *frame,
-			  struct property_addr_info *addr_stack,
-			  CORE_ADDR *value)
+dwarf2_evaluate_property_signed (const struct dynamic_prop *prop,
+				 struct frame_info *frame,
+				 struct property_addr_info *addr_stack,
+				 CORE_ADDR *value, int is_signed)
 {
+  int rc = 0;
+
   if (prop == NULL)
     return 0;
 
@@ -2687,7 +2689,7 @@  dwarf2_evaluate_property (const struct dynamic_prop *prop,
 
 		*value = value_as_address (val);
 	      }
-	    return 1;
+	    rc = 1;
 	  }
       }
       break;
@@ -2709,7 +2711,7 @@  dwarf2_evaluate_property (const struct dynamic_prop *prop,
 	    if (!value_optimized_out (val))
 	      {
 		*value = value_as_address (val);
-		return 1;
+		rc = 1;
 	      }
 	  }
       }
@@ -2717,7 +2719,8 @@  dwarf2_evaluate_property (const struct dynamic_prop *prop,
 
     case PROP_CONST:
       *value = prop->data.const_val;
-      return 1;
+      rc = 1;
+      break;
 
     case PROP_ADDR_OFFSET:
       {
@@ -2739,11 +2742,34 @@  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;
+	rc = 1;
       }
+      break;
     }
 
-  return 0;
+  if (rc == 1 && is_signed == 1)
+    {
+      /* 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 rc;
+}
+
+int
+dwarf2_evaluate_property (const struct dynamic_prop *prop,
+			  struct frame_info *frame,
+			  struct property_addr_info *addr_stack,
+			  CORE_ADDR *value)
+{
+  return dwarf2_evaluate_property_signed (prop, frame, addr_stack, value, 0);
 }
 
 /* See dwarf2loc.h.  */
diff --git a/gdb/dwarf2loc.h b/gdb/dwarf2loc.h
index d7a56db05d..408e1904cd 100644
--- a/gdb/dwarf2loc.h
+++ b/gdb/dwarf2loc.h
@@ -143,6 +143,13 @@  int dwarf2_evaluate_property (const struct dynamic_prop *prop,
 			      struct property_addr_info *addr_stack,
 			      CORE_ADDR *value);
 
+int dwarf2_evaluate_property_signed (const struct dynamic_prop *prop,
+			      struct frame_info *frame,
+			      struct property_addr_info *addr_stack,
+			      CORE_ADDR *value,
+			      int is_signed);
+
+
 /* A helper for the compiler interface that compiles a single dynamic
    property to C code.