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

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

* 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
>