[RFA,6/8] Use value_freer in dwarf2_evaluate_loc_desc_full

  This changes dwarf2_evaluate_loc_desc_full to use value_freer.

Note that this function previously called do_cleanup using the same
cleanup multiple times.  I had thought this was buggy, but re-reading
make_my_cleanup2 indicates that it is not.  Nevertheless it is
surprising, and at least one of the calls (the one that is completely
removed in this patch) seems to have been done under the assumption
that it would still have some effect.

2016-11-28  Tom Tromey  <tom@tromey.com>

	* value.h (value_freer::~value_freer): Call release.
	(value_freer::release): New method.
	* dwarf2loc.c (dwarf2_evaluate_loc_desc_full): Use value_freer.
---
 gdb/ChangeLog   |  6 ++++++
 gdb/dwarf2loc.c | 27 +++++++++++++--------------
 gdb/value.h     | 12 +++++++++++-
 3 files changed, 30 insertions(+), 15 deletions(-)
  

On 11/29/2016 05:05 AM, Tom Tromey wrote:
> This changes dwarf2_evaluate_loc_desc_full to use value_freer.
> 
> Note that this function previously called do_cleanup using the same
> cleanup multiple times.  I had thought this was buggy, but re-reading
> make_my_cleanup2 indicates that it is not.  Nevertheless it is
> surprising, and at least one of the calls (the one that is completely
> removed in this patch) seems to have been done under the assumption
> that it would still have some effect.
> 
> 2016-11-28  Tom Tromey  <tom@tromey.com>
> 
> 	* value.h (value_freer::~value_freer): Call release.
> 	(value_freer::release): New method.
> 	* dwarf2loc.c (dwarf2_evaluate_loc_desc_full): Use value_freer.
> ---
>  gdb/ChangeLog   |  6 ++++++
>  gdb/dwarf2loc.c | 27 +++++++++++++--------------
>  gdb/value.h     | 12 +++++++++++-
>  3 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 6197bfb..cf61306 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,11 @@
>  2016-11-28  Tom Tromey  <tom@tromey.com>
>  
> +	* value.h (value_freer::~value_freer): Call release.
> +	(value_freer::release): New method.
> +	* dwarf2loc.c (dwarf2_evaluate_loc_desc_full): Use value_freer.
> +
> +2016-11-28  Tom Tromey  <tom@tromey.com>
> +
>  	* python/py-value.c (valpy_dereference, valpy_referenced_value)
>  	(valpy_reference_value, valpy_const_value, valpy_get_address)
>  	(valpy_get_dynamic_type, valpy_lazy_string, valpy_do_cast)
> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index 0d8a47c..43c95b8 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -2321,7 +2321,6 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
>  			       LONGEST byte_offset)
>  {
>    struct value *retval;
> -  struct cleanup *value_chain;
>    struct objfile *objfile = dwarf2_per_cu_objfile (per_cu);
>  
>    if (byte_offset < 0)
> @@ -2335,7 +2334,7 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
>    ctx.per_cu = per_cu;
>    ctx.obj_address = 0;
>  
> -  value_chain = make_cleanup_value_free_to_mark (value_mark ());
> +  value_freer free_values;
>  
>    ctx.gdbarch = get_objfile_arch (objfile);
>    ctx.addr_size = dwarf2_per_cu_addr_size (per_cu);
> @@ -2350,7 +2349,7 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
>      {
>        if (ex.error == NOT_AVAILABLE_ERROR)
>  	{
> -	  do_cleanups (value_chain);
> +	  free_values.release ();

"release ()" looks potentially confusing to me, if you're in an
"unique_ptr" mindset, where release means the opposite -- to stop
managing.  Can we call that something else?

Maybe "free", since the class is called value_freer?

Or "reset ()", following the naming used in the standard smart pointers?

The latter could be naturally extended to support

  free_values.reset (value_mark ());

later too, if we need it.

Thanks,
Pedro Alves
  

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

Pedro> Or "reset ()", following the naming used in the standard smart pointers?

I went with reset since I think "free" can be a macro sometimes.
Or at least it could in C... not actually sure if C++ removed this
possibility.

Tom
  

On 12/13/2016 01:28 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Or "reset ()", following the naming used in the standard smart pointers?
> 
> I went with reset since I think "free" can be a macro sometimes.
> Or at least it could in C... not actually sure if C++ removed this
> possibility.

I always assumed it has, but had never went looking for the specific
wording.  Looking at the C++14 draft N4140, I see:

 17.6.1.2 - Headers:

 5 - Names which are defined as macros in C shall be defined as macros in
 the C ++ standard library, even if C grants license for implementation 
 as functions. [ Note: The names defined as macros in C include the
 following: assert, offsetof, setjmp, va_arg, va_end, and va_start. - end note ]

 6 - Names that are defined as functions in C shall be defined as functions in
 the C ++ standard library." (175)

And then footnote 175 clarifies:

 "175) This disallows the practice, allowed in C, of providing a masking
 macro in addition to the function prototype. The only way to achieve equivalent
 inline behavior in C ++ is to provide a definition as an extern inline function."


So I think the answer is yes, C++ removes that possibility.


BTW, I meanwhile realized that "release" would also be a
naming/concept conflict with release_value / value_release_to_mark
too.  Really best to avoid it here.

In pondering a bit more over this, I wonder whether
adding a "scoped_" to go with scoped_restore etc., would make
it a bit clearer to readers that this is a RAII type.  Then
also considering value_release_to_mark, I wonder would an
API/naming like this:

struct scoped_value_mark
{
   scoped_value_mark () : m_value (value_mark ()) {}
   ~scoped_value_mark () { free_to_mark ()}

   void free_to_mark () { if (m_value) value_free_to_mark (m_value); }
   void release_to_mark () { if (m_value) value_release_to_mark (m_value); }

   /* Get the mark value.  */
   struct value *get () { return m_value; }
};

Uses would look like:

 scoped_value_mark value_mark;
 ...
    value_mark.free_to_mark (); // some path than wants an explicit "free_to_mark".


In eval.c:fetch_subexp_value we'd use it like:

  /* Evaluate the expression.  */
  scoped_value_mark mark;

  [...]
      result = evaluate_subexp (NULL_TYPE, exp, pc, EVAL_NORMAL);
  [...]

  new_mark = value_mark ();
  if (mark.get () == new_mark)
    return;

  /* Make sure it's not lazy, so that after the target stops again we
     have a non-lazy previous value to compare with.  */
  [...]

  if (val_chain)
    {
      /* Return the chain of intermediate values.  We use this to
	 decide which addresses to watch.  */
      *val_chain = new_mark;
      mark.release_to_mark ();
    }
}

Would this result in clearer client code?  IMHO, yes, but WDYT?

Thanks,
Pedro Alves
  

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

Pedro> In pondering a bit more over this, I wonder whether
Pedro> adding a "scoped_" to go with scoped_restore etc., would make
Pedro> it a bit clearer to readers that this is a RAII type.  Then
Pedro> also considering value_release_to_mark, I wonder would an
Pedro> API/naming like this:
[...]
Pedro> Would this result in clearer client code?  IMHO, yes, but WDYT?

It looks good to me.  I've made this change and I'll send the new
patches soon.

FYI I've only implemented the subset of the proposed API that was needed
by the patches I have.

Tom