[PATCH] Remove unused arguments to few functions in dwarf2loc.c and gdbtypes.c.

[PATCH] Remove unused arguments to few functions in dwarf2loc.c and gdbtypes.c.

[Siva Chandra]

[-- Attachment #1: Type: text/plain, Size: 1799 bytes --]

The attached patch removes unused arguments to few recently added (few
as recent as yesterday) functions. I did not see any reasoning as to
why they exist in the patch posts to gdb-patches.

I did some digging as to why the unused args could be useful at all.
For example, one function touched by the attached patch is
dwarf2loc:dwarf2_evaluate_property. Before this patch, this function
required an (eventually) unused argument of type CORE_ADDR. I think
the intention there was correct: A dynamic property is probably
embedded in the value and hence the value address is required to
evaluate it. However, the patch that added this function added c99 vla
support. I think the array lengths for VLAs are not embedded in the
(array) object but somewhere else (the stack frame for example).
Hence, the value address is not required.  Am I missing something
here?

[One property for which the value address could be useful is when
evaluating bitpos of virtual base class fields. However, in the Python
API for example, one looks up bitpos via gdb.Field.bitpos where the
field object is typically got via gdb.Type.fields() and not via a
gdb.Value object. Hence, dwarf2loc:dwarf2_evaluate_property is not
really useful with or without the CORE_ADDR argument for this. I am
working on a patch which looks up the bitpos of virtual base classes
dynamically via the vtable of the class.]

gdb/ChangeLog:
2014-04-29  Siva Chandra Reddy  <sivachandra@google.com>

        * dwarf2loc.c (dwarf2_locexpr_baton_eval,
        dwarf2_evaluate_property): Remove unused CORE_ADDR argument.
        Update all callers.
        * dwarf2loc.h (dwarf2_evaluate_property): Update signature.
        * gdbtypes.c (resolve_dynamic_range, resolve_dynamic_array):
        Remove unused CORE_ADDR argument. Update all callers.

[-- Attachment #2: remove_unused_args_v1.txt --]
[-- Type: text/plain, Size: 3991 bytes --]

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index fa17ea6..5517442 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -2436,7 +2436,7 @@ dwarf2_evaluate_loc_desc (struct type *type, struct frame_info *frame,
 
 static int
 dwarf2_locexpr_baton_eval (const struct dwarf2_locexpr_baton *dlbaton,
-			   CORE_ADDR addr, CORE_ADDR *valp)
+			   CORE_ADDR *valp)
 {
   struct dwarf_expr_context *ctx;
   struct dwarf_expr_baton baton;
@@ -2491,8 +2491,7 @@ dwarf2_locexpr_baton_eval (const struct dwarf2_locexpr_baton *dlbaton,
 /* See dwarf2loc.h.  */
 
 int
-dwarf2_evaluate_property (const struct dynamic_prop *prop, CORE_ADDR address,
-			  CORE_ADDR *value)
+dwarf2_evaluate_property (const struct dynamic_prop *prop, CORE_ADDR *value)
 {
   if (prop == NULL)
     return 0;
@@ -2503,7 +2502,7 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop, CORE_ADDR address,
       {
 	const struct dwarf2_property_baton *baton = prop->data.baton;
 
-	if (dwarf2_locexpr_baton_eval (&baton->locexpr, address, value))
+	if (dwarf2_locexpr_baton_eval (&baton->locexpr, value))
 	  {
 	    if (baton->referenced_type)
 	      {
diff --git a/gdb/dwarf2loc.h b/gdb/dwarf2loc.h
index 36173c5..8ad5fa9 100644
--- a/gdb/dwarf2loc.h
+++ b/gdb/dwarf2loc.h
@@ -96,7 +96,7 @@ struct value *dwarf2_evaluate_loc_desc (struct type *type,
    into VALUE, otherwise returns 0.  */
 
 int dwarf2_evaluate_property (const struct dynamic_prop *prop,
-			      CORE_ADDR addr, CORE_ADDR *value);
+			      CORE_ADDR *value);
 
 CORE_ADDR dwarf2_read_addr_index (struct dwarf2_per_cu_data *per_cu,
 				  unsigned int addr_index);
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 1a07420..8e6631a 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1646,7 +1646,7 @@ is_dynamic_type (struct type *type)
 }
 
 static struct type *
-resolve_dynamic_range (struct type *dyn_range_type, CORE_ADDR addr)
+resolve_dynamic_range (struct type *dyn_range_type)
 {
   CORE_ADDR value;
   struct type *static_range_type;
@@ -1657,7 +1657,7 @@ resolve_dynamic_range (struct type *dyn_range_type, CORE_ADDR addr)
   gdb_assert (TYPE_CODE (dyn_range_type) == TYPE_CODE_RANGE);
 
   prop = &TYPE_RANGE_DATA (dyn_range_type)->low;
-  if (dwarf2_evaluate_property (prop, addr, &value))
+  if (dwarf2_evaluate_property (prop, &value))
     {
       low_bound.kind = PROP_CONST;
       low_bound.data.const_val = value;
@@ -1669,7 +1669,7 @@ resolve_dynamic_range (struct type *dyn_range_type, CORE_ADDR addr)
     }
 
   prop = &TYPE_RANGE_DATA (dyn_range_type)->high;
-  if (dwarf2_evaluate_property (prop, addr, &value))
+  if (dwarf2_evaluate_property (prop, &value))
     {
       high_bound.kind = PROP_CONST;
       high_bound.data.const_val = value;
@@ -1696,7 +1696,7 @@ resolve_dynamic_range (struct type *dyn_range_type, CORE_ADDR addr)
    of the associated array.  */
 
 static struct type *
-resolve_dynamic_array (struct type *type, CORE_ADDR addr)
+resolve_dynamic_array (struct type *type)
 {
   CORE_ADDR value;
   struct type *elt_type;
@@ -1707,12 +1707,12 @@ resolve_dynamic_array (struct type *type, CORE_ADDR addr)
 
   elt_type = type;
   range_type = check_typedef (TYPE_INDEX_TYPE (elt_type));
-  range_type = resolve_dynamic_range (range_type, addr);
+  range_type = resolve_dynamic_range (range_type);
 
   ary_dim = check_typedef (TYPE_TARGET_TYPE (elt_type));
 
   if (ary_dim != NULL && TYPE_CODE (ary_dim) == TYPE_CODE_ARRAY)
-    elt_type = resolve_dynamic_array (TYPE_TARGET_TYPE (type), addr);
+    elt_type = resolve_dynamic_array (TYPE_TARGET_TYPE (type));
   else
     elt_type = TYPE_TARGET_TYPE (type);
 
@@ -1751,11 +1751,11 @@ resolve_dynamic_type (struct type *type, CORE_ADDR addr)
 	}
 
       case TYPE_CODE_ARRAY:
-	resolved_type = resolve_dynamic_array (type, addr);
+	resolved_type = resolve_dynamic_array (type);
 	break;
 
       case TYPE_CODE_RANGE:
-	resolved_type = resolve_dynamic_range (type, addr);
+	resolved_type = resolve_dynamic_range (type);
 	break;
     }
 

Re: [PATCH] Remove unused arguments to few functions in dwarf2loc.c and gdbtypes.c.

[Joel Brobecker]

> I did some digging as to why the unused args could be useful at all.
> For example, one function touched by the attached patch is
> dwarf2loc:dwarf2_evaluate_property. Before this patch, this function
> required an (eventually) unused argument of type CORE_ADDR. I think
> the intention there was correct: A dynamic property is probably
> embedded in the value and hence the value address is required to
> evaluate it. However, the patch that added this function added c99 vla
> support. I think the array lengths for VLAs are not embedded in the
> (array) object but somewhere else (the stack frame for example).
> Hence, the value address is not required.  Am I missing something
> here?

I was wondering the very same thing, and even sent a question about it:
https://www.sourceware.org/ml/gdb-patches/2014-04/msg00385.html

> [One property for which the value address could be useful is when
> evaluating bitpos of virtual base class fields. However, in the Python
> API for example, one looks up bitpos via gdb.Field.bitpos where the
> field object is typically got via gdb.Type.fields() and not via a
> gdb.Value object. Hence, dwarf2loc:dwarf2_evaluate_property is not
> really useful with or without the CORE_ADDR argument for this. I am
> working on a patch which looks up the bitpos of virtual base classes
> dynamically via the vtable of the class.]

Interesting!

> gdb/ChangeLog:
> 2014-04-29  Siva Chandra Reddy  <sivachandra@google.com>
> 
>         * dwarf2loc.c (dwarf2_locexpr_baton_eval,
>         dwarf2_evaluate_property): Remove unused CORE_ADDR argument.
>         Update all callers.
>         * dwarf2loc.h (dwarf2_evaluate_property): Update signature.
>         * gdbtypes.c (resolve_dynamic_range, resolve_dynamic_array):
>         Remove unused CORE_ADDR argument. Update all callers.

Given the lack of response so far, I'm going to go ahead and approve.
If new situations prove that we need to add new parameters in order
to provide more info, we will take care of it then. One thing that
I have trouble with, at the moment, is the fact that there wouldn't
always be an address (case of an object stored inside a set of registers,
for instance).

One nit in the ChangeLog: Mising second space on the last line
after the period (before "Update [...]").

Thanks for the patch!

> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index fa17ea6..5517442 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -2436,7 +2436,7 @@ dwarf2_evaluate_loc_desc (struct type *type, struct frame_info *frame,
>  
>  static int
>  dwarf2_locexpr_baton_eval (const struct dwarf2_locexpr_baton *dlbaton,
> -			   CORE_ADDR addr, CORE_ADDR *valp)
> +			   CORE_ADDR *valp)
>  {
>    struct dwarf_expr_context *ctx;
>    struct dwarf_expr_baton baton;
> @@ -2491,8 +2491,7 @@ dwarf2_locexpr_baton_eval (const struct dwarf2_locexpr_baton *dlbaton,
>  /* See dwarf2loc.h.  */
>  
>  int
> -dwarf2_evaluate_property (const struct dynamic_prop *prop, CORE_ADDR address,
> -			  CORE_ADDR *value)
> +dwarf2_evaluate_property (const struct dynamic_prop *prop, CORE_ADDR *value)
>  {
>    if (prop == NULL)
>      return 0;
> @@ -2503,7 +2502,7 @@ dwarf2_evaluate_property (const struct dynamic_prop *prop, CORE_ADDR address,
>        {
>  	const struct dwarf2_property_baton *baton = prop->data.baton;
>  
> -	if (dwarf2_locexpr_baton_eval (&baton->locexpr, address, value))
> +	if (dwarf2_locexpr_baton_eval (&baton->locexpr, value))
>  	  {
>  	    if (baton->referenced_type)
>  	      {
> diff --git a/gdb/dwarf2loc.h b/gdb/dwarf2loc.h
> index 36173c5..8ad5fa9 100644
> --- a/gdb/dwarf2loc.h
> +++ b/gdb/dwarf2loc.h
> @@ -96,7 +96,7 @@ struct value *dwarf2_evaluate_loc_desc (struct type *type,
>     into VALUE, otherwise returns 0.  */
>  
>  int dwarf2_evaluate_property (const struct dynamic_prop *prop,
> -			      CORE_ADDR addr, CORE_ADDR *value);
> +			      CORE_ADDR *value);
>  
>  CORE_ADDR dwarf2_read_addr_index (struct dwarf2_per_cu_data *per_cu,
>  				  unsigned int addr_index);
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index 1a07420..8e6631a 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -1646,7 +1646,7 @@ is_dynamic_type (struct type *type)
>  }
>  
>  static struct type *
> -resolve_dynamic_range (struct type *dyn_range_type, CORE_ADDR addr)
> +resolve_dynamic_range (struct type *dyn_range_type)
>  {
>    CORE_ADDR value;
>    struct type *static_range_type;
> @@ -1657,7 +1657,7 @@ resolve_dynamic_range (struct type *dyn_range_type, CORE_ADDR addr)
>    gdb_assert (TYPE_CODE (dyn_range_type) == TYPE_CODE_RANGE);
>  
>    prop = &TYPE_RANGE_DATA (dyn_range_type)->low;
> -  if (dwarf2_evaluate_property (prop, addr, &value))
> +  if (dwarf2_evaluate_property (prop, &value))
>      {
>        low_bound.kind = PROP_CONST;
>        low_bound.data.const_val = value;
> @@ -1669,7 +1669,7 @@ resolve_dynamic_range (struct type *dyn_range_type, CORE_ADDR addr)
>      }
>  
>    prop = &TYPE_RANGE_DATA (dyn_range_type)->high;
> -  if (dwarf2_evaluate_property (prop, addr, &value))
> +  if (dwarf2_evaluate_property (prop, &value))
>      {
>        high_bound.kind = PROP_CONST;
>        high_bound.data.const_val = value;
> @@ -1696,7 +1696,7 @@ resolve_dynamic_range (struct type *dyn_range_type, CORE_ADDR addr)
>     of the associated array.  */
>  
>  static struct type *
> -resolve_dynamic_array (struct type *type, CORE_ADDR addr)
> +resolve_dynamic_array (struct type *type)
>  {
>    CORE_ADDR value;
>    struct type *elt_type;
> @@ -1707,12 +1707,12 @@ resolve_dynamic_array (struct type *type, CORE_ADDR addr)
>  
>    elt_type = type;
>    range_type = check_typedef (TYPE_INDEX_TYPE (elt_type));
> -  range_type = resolve_dynamic_range (range_type, addr);
> +  range_type = resolve_dynamic_range (range_type);
>  
>    ary_dim = check_typedef (TYPE_TARGET_TYPE (elt_type));
>  
>    if (ary_dim != NULL && TYPE_CODE (ary_dim) == TYPE_CODE_ARRAY)
> -    elt_type = resolve_dynamic_array (TYPE_TARGET_TYPE (type), addr);
> +    elt_type = resolve_dynamic_array (TYPE_TARGET_TYPE (type));
>    else
>      elt_type = TYPE_TARGET_TYPE (type);
>  
> @@ -1751,11 +1751,11 @@ resolve_dynamic_type (struct type *type, CORE_ADDR addr)
>  	}
>  
>        case TYPE_CODE_ARRAY:
> -	resolved_type = resolve_dynamic_array (type, addr);
> +	resolved_type = resolve_dynamic_array (type);
>  	break;
>  
>        case TYPE_CODE_RANGE:
> -	resolved_type = resolve_dynamic_range (type, addr);
> +	resolved_type = resolve_dynamic_range (type);
>  	break;
>      }
>  


-- 
Joel

Re: [PATCH] Remove unused arguments to few functions in dwarf2loc.c and gdbtypes.c.

[Siva Chandra]

On Tue, Apr 29, 2014 at 7:15 AM, Joel Brobecker <brobecker@adacore.com> wrote:
> Given the lack of response so far, I'm going to go ahead and approve.
> [...]
> One nit in the ChangeLog: Mising second space on the last line
> after the period (before "Update [...]").

Pushed after fixing the ChangeLog: 1cfdf5340af6f07bb44b97c278f7036ef8db5c43

RE: [PATCH] Remove unused arguments to few functions in dwarf2loc.c and gdbtypes.c.

[Agovic, Sanimir]

> > I did some digging as to why the unused args could be useful at all.
> > For example, one function touched by the attached patch is
> > dwarf2loc:dwarf2_evaluate_property. Before this patch, this function
> > required an (eventually) unused argument of type CORE_ADDR. I think
> > the intention there was correct: A dynamic property is probably
> > embedded in the value and hence the value address is required to
> > evaluate it. However, the patch that added this function added c99 vla
> > support. I think the array lengths for VLAs are not embedded in the
> > (array) object but somewhere else (the stack frame for example).
> > Hence, the value address is not required.  Am I missing something
> > here?
> 
Indeed. As you described the parameter is used to pass the address of the object 
currently evaluated. The opcode DW_OP_push_object_address consumes the address.
I decided to extract the implementation of the mentioned opcode and 
DW_AT_data_location into two separate patches so ease reviewing, as they are not
specific to C99-vla. Looks like I forgot to remove the bit while refactoring.

Thanks for taking care! The two pending patches will be send to the ML next week.

 -Sanimir

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052