[RFA,02/22] Use RAII to save and restore scalars

  This patch replaces many (but not all) uses of
make_cleanup_restore_integer with a simple RAII-based template class.
It also removes the similar restore_execution_direction cleanup in
favor of this new class.  Subsequent patches will replace other
similar cleanups with this class.

I chose the name "scoped_restore" for this class.  I considered that
perhaps "auto_restore" might be clearer.  Or maybe something else;
it's simple enough to change.

I had hoped that template parameter deduction would work here, but it
did not, and so the patch uses explicit template parameters
everywhere.

2016-09-26  Tom Tromey  <tom@tromey.com>

	* utils.h (class scoped_restore): New class.
	* top.c (execute_command_to_string): Use scoped_restore.
	* python/python.c (python_interactive_command): Use
	scoped_restore.
	(python_command, execute_gdb_command): Likewise.
	* printcmd.c (do_one_display): Use scoped_restore.
	* mi/mi-main.c (exec_continue): Use scoped_restore.
	* mi/mi-cmd-var.c (mi_cmd_var_assign): Use scoped_restore.
	* linux-fork.c (checkpoint_command): Use scoped_restore.
	* infrun.c (restore_execution_direction): Remove.
	(fetch_inferior_event): Use scoped_restore.
	* compile/compile.c (compile_file_command): Use
	scoped_restore.
	(compile_code_command, compile_print_command): Likewise.
	* cli/cli-script.c (execute_user_command): Use
	scoped_restore.
	(while_command, if_command, script_from_file): Likewise.
	* arm-tdep.c (arm_insert_single_step_breakpoint): Use
	scoped_restore.
---
 gdb/ChangeLog         | 22 ++++++++++++++++++++++
 gdb/arm-tdep.c        |  8 ++------
 gdb/cli/cli-script.c  | 18 ++++--------------
 gdb/compile/compile.c | 23 ++++++++---------------
 gdb/infrun.c          | 16 ++--------------
 gdb/linux-fork.c      | 11 ++++++-----
 gdb/mi/mi-cmd-var.c   |  7 +------
 gdb/mi/mi-main.c      |  3 +--
 gdb/printcmd.c        |  5 +----
 gdb/python/python.c   | 12 +++---------
 gdb/top.c             |  3 +--
 gdb/utils.h           | 44 ++++++++++++++++++++++++++++++++++++++++++++
 12 files changed, 95 insertions(+), 77 deletions(-)
  

On Mon, Sep 26, 2016 at 10:08:30PM -0600, Tom Tromey wrote:
> This patch replaces many (but not all) uses of
> make_cleanup_restore_integer with a simple RAII-based template class.
> It also removes the similar restore_execution_direction cleanup in
> favor of this new class.  Subsequent patches will replace other
> similar cleanups with this class.
> 
> I chose the name "scoped_restore" for this class.  I considered that
> perhaps "auto_restore" might be clearer.  Or maybe something else;
> it's simple enough to change.

Its worth noting dmalcolm wants to add something similar to gcc for his
rtl front end work.

> I had hoped that template parameter deduction would work here, but it
> did not, and so the patch uses explicit template parameters
> everywhere.

yeah, you can't deduce template args from a constructor in C++

> +  // Create a new scoped_restore object that saves the current value
> +  // of *VAR, and then sets *VAR to VALUE.  *VAR will be restored when
> +  // this scoped_restore object is destroyed.
> +  scoped_restore (T *var, T value)
> +    : saved_var (var),
> +      saved_value (*var)
> +  {
> +    *var = value;

I wonder if it isn't clearer to create the scoped_restore and then set
the variable yourself.

> +  // The saved variable.
> +  T *saved_var;
> +
> +  // The saved value.
> +  T saved_value;

 I think you could use const T, but I doubt it matters, these objects
 really shouldn't have there address taken so the compiler should see
 everything about them.

Trev
  

On 09/27/2016 09:59 AM, Trevor Saunders wrote:

>> I had hoped that template parameter deduction would work here, but it
>> did not, and so the patch uses explicit template parameters
>> everywhere.
> 
> yeah, you can't deduce template args from a constructor in C++

I don't know whether the trick below is usual, but you can work
around that and end up with no-template-args-in-sight usage
like this:

void
foo ()
{
  scoped_restore scope = make_scoped_restore (&some_global);

  ...
}



You'd do something like this:


struct scoped_restore_base {};

template<typename T>
class scoped_restore_ : public scoped_restore_base
{
 public:
...
  explicit scoped_restore (T *var)
    : saved_var (var),
      saved_value (*var)
  {}

  ~scoped_restore ()
  {
    *saved_var = saved_value;
  }

 private:
  scoped_restore &operator= (const scoped_restore &rhs);

  // The saved variable.
  T *saved_var;

  // The saved value.
  T saved_value;
};

typedef const scoped_restore_base &scoped_restore;

template<class T>
scoped_restore_<T> make_scoped_restore (T *var)
{
  return scoped_restore_<T> (var);
}

The trick is making scoped_restore_ inherit a non-template
class, and make "scoped_restore" a typedef for a const reference
to the base class, since a const reference can bind to the
temporary that is returned by make_scoped_restore, and it
extends the temporary's lifetime.

While copy elision / RVO makes it so that the temporary returned by
value from make_scoped_restore is constructed directly in the caller
instead of constructed and then copied out, you still have to
provide the copy constructor, instead of "deleting" it as in your
original patch.  Copy elision was optional until C++17, but I
believe all compilers worth their salt implement it.  And if a compiler
doesn't implement it, it's not a real issue, anyway, the code
still works.

The downside is that printing a scoped_restore object from
gdb will just print the reference using the base type, so you
won't see stored fields.

(gdb) p scope
$1 = (const scoped_restore) @0x7fffffffd910: {<No data fields>}

To see the object's fields you'd have to downcast it manually, like:

(gdb) p (scoped_restore_<ui_out *>) scope
$1 = {<scoped_restore_base> = {<No data fields>}, saved_var = 0x601050 <current_uiout>, saved_value = 0x0}

Not sure whether that's an issue.

Thanks,
Pedro Alves
  

On 09/30/2016 02:17 AM, Pedro Alves wrote:
> On 09/27/2016 09:59 AM, Trevor Saunders wrote:
> 
>>> I had hoped that template parameter deduction would work here, but it
>>> did not, and so the patch uses explicit template parameters
>>> everywhere.
>>
>> yeah, you can't deduce template args from a constructor in C++
> 
> I don't know whether the trick below is usual, 

Looks like it is -- Andrei Alexandrescu's ScopeGuard uses the
same trick:

  http://www.drdobbs.com/cpp/generic-change-the-way-you-write-excepti/184403758

Thanks,
Pedro Alves

> but you can work
> around that and end up with no-template-args-in-sight usage
> like this:
> 
> void
> foo ()
> {
>   scoped_restore scope = make_scoped_restore (&some_global);
> 
>   ...
> }
> 
> You'd do something like this:
> 
> 
> struct scoped_restore_base {};
> 
> template<typename T>
> class scoped_restore_ : public scoped_restore_base
> {
>  public:
> ...
>   explicit scoped_restore (T *var)
>     : saved_var (var),
>       saved_value (*var)
>   {}
> 
>   ~scoped_restore ()
>   {
>     *saved_var = saved_value;
>   }
> 
>  private:
>   scoped_restore &operator= (const scoped_restore &rhs);
> 
>   // The saved variable.
>   T *saved_var;
> 
>   // The saved value.
>   T saved_value;
> };
> 
> typedef const scoped_restore_base &scoped_restore;
> 
> template<class T>
> scoped_restore_<T> make_scoped_restore (T *var)
> {
>   return scoped_restore_<T> (var);
> }
> 
> The trick is making scoped_restore_ inherit a non-template
> class, and make "scoped_restore" a typedef for a const reference
> to the base class, since a const reference can bind to the
> temporary that is returned by make_scoped_restore, and it
> extends the temporary's lifetime.
> 
> While copy elision / RVO makes it so that the temporary returned by
> value from make_scoped_restore is constructed directly in the caller
> instead of constructed and then copied out, you still have to
> provide the copy constructor, instead of "deleting" it as in your
> original patch.  Copy elision was optional until C++17, but I
> believe all compilers worth their salt implement it.  And if a compiler
> doesn't implement it, it's not a real issue, anyway, the code
> still works.
> 
> The downside is that printing a scoped_restore object from
> gdb will just print the reference using the base type, so you
> won't see stored fields.
> 
> (gdb) p scope
> $1 = (const scoped_restore) @0x7fffffffd910: {<No data fields>}
> 
> To see the object's fields you'd have to downcast it manually, like:
> 
> (gdb) p (scoped_restore_<ui_out *>) scope
> $1 = {<scoped_restore_base> = {<No data fields>}, saved_var = 0x601050 <current_uiout>, saved_value = 0x0}
> 
> Not sure whether that's an issue.
> 
> Thanks,
> Pedro Alves
>
  

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

Pedro> template<class T>
Pedro> scoped_restore_<T> make_scoped_restore (T *var)
Pedro> {
Pedro>   return scoped_restore_<T> (var);
Pedro> }

Yeah, I'll do this.  I don't know why I didn't remember this when
writing the patches.

I'd also appreciate it if you could say whether you want "m_" member
naming.

Tom
  

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

Pedro>   scoped_restore scope = make_scoped_restore (&some_global);

I made this change.  One minor oddity is that the 2-argument form of
make_scoped_restore required a copy constructor in the
scoped_restore_tmpl subclass.  I think that's harmless though.

Tom
  

On 09/30/2016 03:02 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> template<class T>
> Pedro> scoped_restore_<T> make_scoped_restore (T *var)
> Pedro> {
> Pedro>   return scoped_restore_<T> (var);
> Pedro> }
> 
> Yeah, I'll do this.  I don't know why I didn't remember this when
> writing the patches.

Great, thanks.

> 
> I'd also appreciate it if you could say whether you want "m_" member
> naming.
> 

Unless there's a good reason to deviate, I think it's a good idea
to follow gcc's coding conventions.  For that reason, my preference
is to use "m_" for private data members.

Thanks,
Pedro Alves
  

On 09/30/2016 04:17 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro>   scoped_restore scope = make_scoped_restore (&some_global);
> 
> I made this change.  One minor oddity is that the 2-argument form of
> make_scoped_restore required a copy constructor in the
> scoped_restore_tmpl subclass.  I think that's harmless though.

Yeah.  We just need to make sure that if a copy constructor does run,
the code behaves as if it hadn't.  My earlier comment on that
not mattering may have been incorrect - if the copied-from scoped_restore
is destructed before the make_scoped_restore function returns, then the
global will revert back to the original value too soon.  Guess we could
have the copy constructor actually move instead of copy, setting the
copied-from's pointer member to NULL, and then have the
destructor do nothing if the pointer is NULL.

Thanks,
Pedro Alves
  

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

Pedro> Guess we could have the copy constructor actually move instead of
Pedro> copy, setting the copied-from's pointer member to NULL, and then
Pedro> have the destructor do nothing if the pointer is NULL.

Yeah. I took this approach.  I'm not super thrilled with it, but at the
same time it's not *too* bad.

Tom
  

Pedro> Guess we could have the copy constructor actually move instead of
Pedro> copy, setting the copied-from's pointer member to NULL, and then
Pedro> have the destructor do nothing if the pointer is NULL.

Tom> Yeah. I took this approach.  I'm not super thrilled with it, but at the
Tom> same time it's not *too* bad.

Scratch that, I spoke too soon.  I couldn't get this to work.

I don't understand why the 2-argument form of make_scoped_restore
requires a copy constructor while the 1-argument form does not.

However, once you need a copy constructor, it doesn't seem possible
without a move constructor.  The issue is that a copy constructor take a
const reference, so you can't modify the fields of the original.

Removing the 2-argument form seems to work.  Though like I said, I don't
know why.


Also, as an aside, I found I was using nullptr in my patches, but this
isn't C++03.  I think -std=c++03 is going to be needed or else it will
be too easy to slip in C++11-isms.  Now, I do think C++11 is really much
better, but my understanding is that C++03 is what gdb decided on.

Tom
  

On 10/01/2016 05:23 AM, Tom Tromey wrote:
> Pedro> Guess we could have the copy constructor actually move instead of
> Pedro> copy, setting the copied-from's pointer member to NULL, and then
> Pedro> have the destructor do nothing if the pointer is NULL.
> 
> Tom> Yeah. I took this approach.  I'm not super thrilled with it, but at the
> Tom> same time it's not *too* bad.
> 
> Scratch that, I spoke too soon.  I couldn't get this to work.
> 
> I don't understand why the 2-argument form of make_scoped_restore
> requires a copy constructor while the 1-argument form does not.
> 
> However, once you need a copy constructor, it doesn't seem possible
> without a move constructor.  The issue is that a copy constructor take a
> const reference, so you can't modify the fields of the original.

You can -- make the fields mutable.  That's what ScopeGuard does too.

> 
> Removing the 2-argument form seems to work.  Though like I said, I don't
> know why.

I can't tell off hand.  Maybe seeing the code would suggest something.

> 
> 
> Also, as an aside, I found I was using nullptr in my patches, but this
> isn't C++03.  I think -std=c++03 is going to be needed or else it will
> be too easy to slip in C++11-isms.  Now, I do think C++11 is really much
> better, but my understanding is that C++03 is what gdb decided on.

I'm very much against forcing -std=c++03.

My plan is to allow compiling with C++11 too, and use
some of its features, for extra safety and extra efficiency,
as long as we have C++03 fallbacks in place.  If we force -std=c++03,
that becomes impossible.  

I recently elaborated on this here:
 https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01624.html

The gist is that I think that the features supported by compilers that
people are using in practice to build gdb with is what matters
in the end, not really C++03 vs C++11.

The only reason we're not requiring C++11 is that some people
still wanted to build gdb with older compilers that don't support it.
That was last revisited a year ago when we discussed whether to
go C++ or not in the first place.  It may be that the baseline might have 
advanced meanwhile.

Thanks,
Pedro Alves
  

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

>> However, once you need a copy constructor, it doesn't seem possible
>> without a move constructor.  The issue is that a copy constructor take a
>> const reference, so you can't modify the fields of the original.

Pedro> You can -- make the fields mutable.

Ok, yeah, I had thought of this, but it seemed pretty ugly to me.

Instead I removed all the 2 argument forms in favor of a second explicit
assignment.  This solves the problem as well.

I can go back and do the mutable thing if you really prefer it though.


Pedro> The gist is that I think that the features supported by compilers that
Pedro> people are using in practice to build gdb with is what matters
Pedro> in the end, not really C++03 vs C++11.

The issue I see is that it's just too easy otherwise for C++11isms to
slip in, especially considering that everybody doing development is
likely to be using a compiler with all the C++11 features.  Maybe
buildbot can do C++03 builds to check.

Tom
  

On 10/02/2016 06:11 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
>>> However, once you need a copy constructor, it doesn't seem possible
>>> without a move constructor.  The issue is that a copy constructor take a
>>> const reference, so you can't modify the fields of the original.
> 
> Pedro> You can -- make the fields mutable.
> 
> Ok, yeah, I had thought of this, but it seemed pretty ugly to me.

It's hidden in the "library", so it'd be super fine with me.
I think of it as just part of the idiom.

> 
> Instead I removed all the 2 argument forms in favor of a second explicit
> assignment.  This solves the problem as well.

OK, I thought a little bit more about my worry with the implicit
copy constructor, and I think I was originally right -- it doesn't
make a difference; things still work if the compiler doesn't elide,
because the single-argument form doesn't change the global.  Guess
you could confirm things still build and run properly
with -fno-elide-constructors.

> I can go back and do the mutable thing if you really prefer it though.

I don't have a strong preference.  Maybe client code using the 1-argument form
ends up looking cleaner, in any case, not sure.

> 
> 
> Pedro> The gist is that I think that the features supported by compilers that
> Pedro> people are using in practice to build gdb with is what matters
> Pedro> in the end, not really C++03 vs C++11.
> 
> The issue I see is that it's just too easy otherwise for C++11isms to
> slip in, especially considering that everybody doing development is
> likely to be using a compiler with all the C++11 features.  Maybe
> buildbot can do C++03 builds to check.

I still disagree to an extent.  Even if we were claiming to
require "C++11", we probably wouldn't be able to use _all_ of C++11.
E.g. if we wanted to require full C++11 support, then we're need to
require gcc 4.8, which was released in 2013.  I'd be super fine with
requiring it personally, and I look forward to the day, tbc!
I think it's present in all current versions of major distros, I
believe,  I wouldn't be surprised to find opposition, though.

As compromise, I think we could be much better at documenting the older gcc version
that we actually support compiling with.  That oldest-supported version, I think
should be decided based on what's the oldest version people really care about,
and are willing to routinely test, vs how bad we'd want the C++11 features
not supported by that old compiler.  Last this was discussed, in context of
deciding whether to move forward with the C++ conversion, we settled on gcc 4.2
at least, since that's what some BSDs were still using.  But we have nothing in
place to enforce that, and I suspect that we still build correctly with
older gcc's even.  Or not, I don't know, since no one is testing it, for all I
know (i.e., no one is sending test reports to gdb-testers@).  I think all BSDs have
llvm/clang in their base systems now, so those are probably no longer really a concern.

Because, if we could require say, gcc 4.3 at least, then we
could make use of rvalue references.  If we required gcc 4.4, then we
could use "auto" and other goodies.  Etc.  IOW, if we took an incremental
approach, we'd be bound by the level of C++11 support in the oldest
compiler we supported.  

  https://gcc.gnu.org/projects/cxx-status.html#cxx11

And _that_ version is what I think should be tested by the buildbot,
irrespective of C++98/C++03/C++0x/C++11/... instead of being artificially
blocked by -std=c++03 or some such.

Thanks,
Pedro Alves
  

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

Pedro> You can -- make the fields mutable.

>> Ok, yeah, I had thought of this, but it seemed pretty ugly to me.

Pedro> It's hidden in the "library", so it'd be super fine with me.
Pedro> I think of it as just part of the idiom.

I did this and restored the 2-argument form.

Tom