[01/11] Dwarf: Fix dynamic properties with neg. value.
Commit Message
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(-)
Comments
* 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
>
@@ -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. */
@@ -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.