[PATCH] ada-lang.c:coerce_unspec_val_to_type: Preserve laziness.

[PATCH] ada-lang.c:coerce_unspec_val_to_type: Preserve laziness.

[Pedro Alves]

ada-lang.c:coerce_unspec_val_to_type does:

      if (value_lazy (val)
	  || TYPE_LENGTH (type) > TYPE_LENGTH (value_type (val)))
	result = allocate_value_lazy (type);
      else
	{
	  result = allocate_value (type);
	  memcpy (value_contents_raw (result), value_contents (val),
		  TYPE_LENGTH (type));
	}
      set_value_component_location (result, val);
      set_value_bitsize (result, value_bitsize (val));
      set_value_bitpos (result, value_bitpos (val));
      set_value_address (result, value_address (val));
      set_value_optimized_out (result, value_optimized_out (val));

Notice that before value_optimized_out was made to auto-fetch lazy
values, VAL would end up still lazy if it was lazy on entry.  It's not
really a problem here if VAL is lazy, and VAL->optimized_out is 0,
because RESULT is also left lazy.  IOW, this just wants to copy the
VAL->optimized_out flag to RESULT->optimized_out, nothing else.

The patch adds the value_optimized_out_const function for that.

(I found this out by grepping for set_value_optimized_out and trying
to convert the uses I found to instead allocate the value with
allocate_optimized_out_value.)

Tested on x86_64 Fedora 17.

WDYT?

gdb/
2013-07-04  Pedro Alves  <palves@redhat.com>

	* ada-lang.c (coerce_unspec_val_to_type): Use
	value_optimized_out_const.
	* value.c (value_optimized_out_const): New function.
	* value.h (value_optimized_out_const): New declaration.
---
 gdb/ada-lang.c |    2 +-
 gdb/value.c    |    6 ++++++
 gdb/value.h    |    8 +++++++-
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 8240fee..dc5f2b6 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -581,7 +581,7 @@ coerce_unspec_val_to_type (struct value *val, struct type *type)
       set_value_bitsize (result, value_bitsize (val));
       set_value_bitpos (result, value_bitpos (val));
       set_value_address (result, value_address (val));
-      set_value_optimized_out (result, value_optimized_out (val));
+      set_value_optimized_out (result, value_optimized_out_const (val));
       return result;
     }
 }
diff --git a/gdb/value.c b/gdb/value.c
index 353f62a..9e58199 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1062,6 +1062,12 @@ value_optimized_out (struct value *value)
   return value->optimized_out;
 }
 
+int
+value_optimized_out_const (const struct value *value)
+{
+  return value->optimized_out;
+}
+
 void
 set_value_optimized_out (struct value *value, int val)
 {
diff --git a/gdb/value.h b/gdb/value.h
index 8a66aa4..8d669ec 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -319,10 +319,16 @@ extern int value_fetch_lazy (struct value *val);
 extern int value_contents_equal (struct value *val1, struct value *val2);
 
 /* If nonzero, this is the value of a variable which does not actually
-   exist in the program.  */
+   exist in the program, at least partially.  If the value is lazy,
+   this may fetch it now.  */
 extern int value_optimized_out (struct value *value);
 extern void set_value_optimized_out (struct value *value, int val);
 
+/* Like value_optimized_out, but don't fetch the value even if it is
+   lazy.  Mainly useful for constructing other values using VALUE as
+   template.  */
+extern int value_optimized_out_const (const struct value *value);
+
 /* Like value_optimized_out, but return false if any bit in the object
    is valid.  */
 extern int value_entirely_optimized_out (const struct value *value);

Re: [PATCH] ada-lang.c:coerce_unspec_val_to_type: Preserve laziness.

[Andrew Burgess]

On 04/07/2013 7:22 PM, Pedro Alves wrote:
> ada-lang.c:coerce_unspec_val_to_type does:
> 
>       if (value_lazy (val)
> 	  || TYPE_LENGTH (type) > TYPE_LENGTH (value_type (val)))
> 	result = allocate_value_lazy (type);
>       else
> 	{
> 	  result = allocate_value (type);
> 	  memcpy (value_contents_raw (result), value_contents (val),
> 		  TYPE_LENGTH (type));
> 	}
>       set_value_component_location (result, val);
>       set_value_bitsize (result, value_bitsize (val));
>       set_value_bitpos (result, value_bitpos (val));
>       set_value_address (result, value_address (val));
>       set_value_optimized_out (result, value_optimized_out (val));
> 
> Notice that before value_optimized_out was made to auto-fetch lazy
> values, VAL would end up still lazy if it was lazy on entry.  It's not
> really a problem here if VAL is lazy, and VAL->optimized_out is 0,
> because RESULT is also left lazy.  IOW, this just wants to copy the
> VAL->optimized_out flag to RESULT->optimized_out, nothing else.
> 
> The patch adds the value_optimized_out_const function for that.

As an alternative, this patch avoids the new _const function,
I move the setting of the optimized_out value into the else case where we know val is no longer lazy.

Cheers,
Andrew


diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 8240fee..ea7b579 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -576,12 +576,14 @@ coerce_unspec_val_to_type (struct value *val, struct type *type)
 	  result = allocate_value (type);
 	  memcpy (value_contents_raw (result), value_contents (val),
 		  TYPE_LENGTH (type));
+	  /* Only copy the optimized out setting if val is not lazy.  If it
+	     is lazy then the optimized_out flag will always be false.  */
+	  set_value_optimized_out (result, value_optimized_out (val));
 	}
       set_value_component_location (result, val);
       set_value_bitsize (result, value_bitsize (val));
       set_value_bitpos (result, value_bitpos (val));
       set_value_address (result, value_address (val));
-      set_value_optimized_out (result, value_optimized_out (val));
       return result;
     }
 }








> 
> (I found this out by grepping for set_value_optimized_out and trying
> to convert the uses I found to instead allocate the value with
> allocate_optimized_out_value.)
> 
> Tested on x86_64 Fedora 17.
> 
> WDYT?
> 
> gdb/
> 2013-07-04  Pedro Alves  <palves@redhat.com>
> 
> 	* ada-lang.c (coerce_unspec_val_to_type): Use
> 	value_optimized_out_const.
> 	* value.c (value_optimized_out_const): New function.
> 	* value.h (value_optimized_out_const): New declaration.
> ---
>  gdb/ada-lang.c |    2 +-
>  gdb/value.c    |    6 ++++++
>  gdb/value.h    |    8 +++++++-
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 8240fee..dc5f2b6 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -581,7 +581,7 @@ coerce_unspec_val_to_type (struct value *val, struct type *type)
>        set_value_bitsize (result, value_bitsize (val));
>        set_value_bitpos (result, value_bitpos (val));
>        set_value_address (result, value_address (val));
> -      set_value_optimized_out (result, value_optimized_out (val));
> +      set_value_optimized_out (result, value_optimized_out_const (val));
>        return result;
>      }
>  }
> diff --git a/gdb/value.c b/gdb/value.c
> index 353f62a..9e58199 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -1062,6 +1062,12 @@ value_optimized_out (struct value *value)
>    return value->optimized_out;
>  }
>  
> +int
> +value_optimized_out_const (const struct value *value)
> +{
> +  return value->optimized_out;
> +}
> +
>  void
>  set_value_optimized_out (struct value *value, int val)
>  {
> diff --git a/gdb/value.h b/gdb/value.h
> index 8a66aa4..8d669ec 100644
> --- a/gdb/value.h
> +++ b/gdb/value.h
> @@ -319,10 +319,16 @@ extern int value_fetch_lazy (struct value *val);
>  extern int value_contents_equal (struct value *val1, struct value *val2);
>  
>  /* If nonzero, this is the value of a variable which does not actually
> -   exist in the program.  */
> +   exist in the program, at least partially.  If the value is lazy,
> +   this may fetch it now.  */
>  extern int value_optimized_out (struct value *value);
>  extern void set_value_optimized_out (struct value *value, int val);
>  
> +/* Like value_optimized_out, but don't fetch the value even if it is
> +   lazy.  Mainly useful for constructing other values using VALUE as
> +   template.  */
> +extern int value_optimized_out_const (const struct value *value);
> +
>  /* Like value_optimized_out, but return false if any bit in the object
>     is valid.  */
>  extern int value_entirely_optimized_out (const struct value *value);
> 
> 
> 
> 

Re: [PATCH] ada-lang.c:coerce_unspec_val_to_type: Preserve laziness.

[Pedro Alves]

On 07/05/2013 11:29 AM, Andrew Burgess wrote:
> On 04/07/2013 7:22 PM, Pedro Alves wrote:
>> ada-lang.c:coerce_unspec_val_to_type does:
>>
>>       if (value_lazy (val)
>> 	  || TYPE_LENGTH (type) > TYPE_LENGTH (value_type (val)))
>> 	result = allocate_value_lazy (type);
>>       else
>> 	{
>> 	  result = allocate_value (type);
>> 	  memcpy (value_contents_raw (result), value_contents (val),
>> 		  TYPE_LENGTH (type));
>> 	}
>>       set_value_component_location (result, val);
>>       set_value_bitsize (result, value_bitsize (val));
>>       set_value_bitpos (result, value_bitpos (val));
>>       set_value_address (result, value_address (val));
>>       set_value_optimized_out (result, value_optimized_out (val));
>>
>> Notice that before value_optimized_out was made to auto-fetch lazy
>> values, VAL would end up still lazy if it was lazy on entry.  It's not
>> really a problem here if VAL is lazy, and VAL->optimized_out is 0,
>> because RESULT is also left lazy.  IOW, this just wants to copy the
>> VAL->optimized_out flag to RESULT->optimized_out, nothing else.
>>
>> The patch adds the value_optimized_out_const function for that.
> 
> As an alternative, this patch avoids the new _const function,
> I move the setting of the optimized_out value into the else case where we know val is no longer lazy.
> 
> Cheers,
> Andrew
> 
> 
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 8240fee..ea7b579 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -576,12 +576,14 @@ coerce_unspec_val_to_type (struct value *val, struct type *type)
>  	  result = allocate_value (type);
>  	  memcpy (value_contents_raw (result), value_contents (val),
>  		  TYPE_LENGTH (type));
> +	  /* Only copy the optimized out setting if val is not lazy.  If it
> +	     is lazy then the optimized_out flag will always be false.  */
> +	  set_value_optimized_out (result, value_optimized_out (val));
>  	}
>        set_value_component_location (result, val);
>        set_value_bitsize (result, value_bitsize (val));
>        set_value_bitpos (result, value_bitpos (val));
>        set_value_address (result, value_address (val));
> -      set_value_optimized_out (result, value_optimized_out (val));
>        return result;

That works, but I had originally discarded such an approach
because it looked brittle to me.  E.g., I was toying with optimizing
value_optimized_out by only fetching lazy values if it really can't
avoid it, and such a change would render this subtly broken.

-- 
Pedro Alves

Re: [PATCH] ada-lang.c:coerce_unspec_val_to_type: Preserve laziness.

[Joel Brobecker]

Hi Pedro,

> ada-lang.c:coerce_unspec_val_to_type does:
> 
>       if (value_lazy (val)
> 	  || TYPE_LENGTH (type) > TYPE_LENGTH (value_type (val)))
> 	result = allocate_value_lazy (type);
>       else
> 	{
> 	  result = allocate_value (type);
> 	  memcpy (value_contents_raw (result), value_contents (val),
> 		  TYPE_LENGTH (type));
> 	}
>       set_value_component_location (result, val);
>       set_value_bitsize (result, value_bitsize (val));
>       set_value_bitpos (result, value_bitpos (val));
>       set_value_address (result, value_address (val));
>       set_value_optimized_out (result, value_optimized_out (val));
> 
> Notice that before value_optimized_out was made to auto-fetch lazy
> values, VAL would end up still lazy if it was lazy on entry.  It's not
> really a problem here if VAL is lazy, and VAL->optimized_out is 0,
> because RESULT is also left lazy.  IOW, this just wants to copy the
> VAL->optimized_out flag to RESULT->optimized_out, nothing else.
> 
> The patch adds the value_optimized_out_const function for that.
> 
> (I found this out by grepping for set_value_optimized_out and trying
> to convert the uses I found to instead allocate the value with
> allocate_optimized_out_value.)
> 
> Tested on x86_64 Fedora 17.

> gdb/
> 2013-07-04  Pedro Alves  <palves@redhat.com>
> 
> 	* ada-lang.c (coerce_unspec_val_to_type): Use
> 	value_optimized_out_const.
> 	* value.c (value_optimized_out_const): New function.
> 	* value.h (value_optimized_out_const): New declaration.

Thanks for the patch!

We can actually demonstrate a regression which your patch fixes.
Consider:

   type Small is range -64 .. 63;
   for Small'Size use 7;
   type Arr is array (1..10) of Small;
   pragma Pack (Arr);

   type Arr_Ptr is access Arr;

   An_Arr_Ptr : Arr_Ptr := new Arr'(10, 20, 30, 40, 50, 60, 62, 63,
                                    -23, 42);

Trying to print one element of An_Arr_Ptr yields:

    (gdb) p an_arr_ptr(3)
    Cannot access memory at address 0x0

I've updated one of the testcases that deals with array "pointers"
(we call them access types in Ada) to add this case:

    http://www.sourceware.org/ml/gdb-patches/2013-07/msg00254.html

Since your patch looks correct to me, you had answers to Andrew's
comments, this is a regression, you are away for a while and no one
else commented on your patch, I've taken the liberty of checking it
in.

> ---
>  gdb/ada-lang.c |    2 +-
>  gdb/value.c    |    6 ++++++
>  gdb/value.h    |    8 +++++++-
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 8240fee..dc5f2b6 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -581,7 +581,7 @@ coerce_unspec_val_to_type (struct value *val, struct type *type)
>        set_value_bitsize (result, value_bitsize (val));
>        set_value_bitpos (result, value_bitpos (val));
>        set_value_address (result, value_address (val));
> -      set_value_optimized_out (result, value_optimized_out (val));
> +      set_value_optimized_out (result, value_optimized_out_const (val));
>        return result;
>      }
>  }
> diff --git a/gdb/value.c b/gdb/value.c
> index 353f62a..9e58199 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -1062,6 +1062,12 @@ value_optimized_out (struct value *value)
>    return value->optimized_out;
>  }
>  
> +int
> +value_optimized_out_const (const struct value *value)
> +{
> +  return value->optimized_out;
> +}
> +
>  void
>  set_value_optimized_out (struct value *value, int val)
>  {
> diff --git a/gdb/value.h b/gdb/value.h
> index 8a66aa4..8d669ec 100644
> --- a/gdb/value.h
> +++ b/gdb/value.h
> @@ -319,10 +319,16 @@ extern int value_fetch_lazy (struct value *val);
>  extern int value_contents_equal (struct value *val1, struct value *val2);
>  
>  /* If nonzero, this is the value of a variable which does not actually
> -   exist in the program.  */
> +   exist in the program, at least partially.  If the value is lazy,
> +   this may fetch it now.  */
>  extern int value_optimized_out (struct value *value);
>  extern void set_value_optimized_out (struct value *value, int val);
>  
> +/* Like value_optimized_out, but don't fetch the value even if it is
> +   lazy.  Mainly useful for constructing other values using VALUE as
> +   template.  */
> +extern int value_optimized_out_const (const struct value *value);
> +
>  /* Like value_optimized_out, but return false if any bit in the object
>     is valid.  */
>  extern int value_entirely_optimized_out (const struct value *value);

-- 
Joel

Re: [PATCH] ada-lang.c:coerce_unspec_val_to_type: Preserve laziness.

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> That works, but I had originally discarded such an approach
Pedro> because it looked brittle to me.  E.g., I was toying with optimizing
Pedro> value_optimized_out by only fetching lazy values if it really can't
Pedro> avoid it, and such a change would render this subtly broken.

This code in ada-lang.c and similar code elsewhere always seems to me to
be a call for a new value copy-constructor that takes a type argument.
Then any necessary wackiness can be isolated in value.c.

Tom

Re: [PATCH] ada-lang.c:coerce_unspec_val_to_type: Preserve laziness.

[Pedro Alves]

On 07/10/2013 06:13 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> That works, but I had originally discarded such an approach
> Pedro> because it looked brittle to me.  E.g., I was toying with optimizing
> Pedro> value_optimized_out by only fetching lazy values if it really can't
> Pedro> avoid it, and such a change would render this subtly broken.
> 
> This code in ada-lang.c and similar code elsewhere always seems to me to
> be a call for a new value copy-constructor that takes a type argument.
> Then any necessary wackiness can be isolated in value.c.

Yeah!  The deprecated_set_value_type calls have tended to morph into things
like that.  I had actually started out with something like that, noticing
that this Ada code is like a reinterpret cast.  I did something based on
refactoring value_copy, but wasn't getting happy with the results (or rather,
the time it was taking me), so I gave up.   But I'd definitely support
it if someone went along those lines.

-- 
Pedro Alves

Re: [PATCH] ada-lang.c:coerce_unspec_val_to_type: Preserve laziness.

[Pedro Alves]

On 07/10/2013 01:44 AM, Joel Brobecker wrote:
> Hi Pedro,
> 
>> ada-lang.c:coerce_unspec_val_to_type does:
>>
>>       if (value_lazy (val)
>> 	  || TYPE_LENGTH (type) > TYPE_LENGTH (value_type (val)))
>> 	result = allocate_value_lazy (type);
>>       else
>> 	{
>> 	  result = allocate_value (type);
>> 	  memcpy (value_contents_raw (result), value_contents (val),
>> 		  TYPE_LENGTH (type));
>> 	}
>>       set_value_component_location (result, val);
>>       set_value_bitsize (result, value_bitsize (val));
>>       set_value_bitpos (result, value_bitpos (val));
>>       set_value_address (result, value_address (val));
>>       set_value_optimized_out (result, value_optimized_out (val));
>>
>> Notice that before value_optimized_out was made to auto-fetch lazy
>> values, VAL would end up still lazy if it was lazy on entry.  It's not
>> really a problem here if VAL is lazy, and VAL->optimized_out is 0,
>> because RESULT is also left lazy.  IOW, this just wants to copy the
>> VAL->optimized_out flag to RESULT->optimized_out, nothing else.
>>
>> The patch adds the value_optimized_out_const function for that.
>>
>> (I found this out by grepping for set_value_optimized_out and trying
>> to convert the uses I found to instead allocate the value with
>> allocate_optimized_out_value.)
>>
>> Tested on x86_64 Fedora 17.
> 
>> gdb/
>> 2013-07-04  Pedro Alves  <palves@redhat.com>
>>
>> 	* ada-lang.c (coerce_unspec_val_to_type): Use
>> 	value_optimized_out_const.
>> 	* value.c (value_optimized_out_const): New function.
>> 	* value.h (value_optimized_out_const): New declaration.
> 
> Thanks for the patch!
> 
> We can actually demonstrate a regression which your patch fixes.
> Consider:
> 
>    type Small is range -64 .. 63;
>    for Small'Size use 7;
>    type Arr is array (1..10) of Small;
>    pragma Pack (Arr);
> 
>    type Arr_Ptr is access Arr;
> 
>    An_Arr_Ptr : Arr_Ptr := new Arr'(10, 20, 30, 40, 50, 60, 62, 63,
>                                     -23, 42);
> 
> Trying to print one element of An_Arr_Ptr yields:
> 
>     (gdb) p an_arr_ptr(3)
>     Cannot access memory at address 0x0
> 
> I've updated one of the testcases that deals with array "pointers"
> (we call them access types in Ada) to add this case:
> 
>     http://www.sourceware.org/ml/gdb-patches/2013-07/msg00254.html
> 
> Since your patch looks correct to me, you had answers to Andrew's
> comments, this is a regression, you are away for a while and no one
> else commented on your patch, I've taken the liberty of checking it
> in.

Thanks!

-- 
Pedro Alves