[RFA,7/8] Use unique_xmalloc_ptr in execute_gdb_command

  On 12/15/2016 03:48 AM, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> 
> Tom> To my surprise, this patch is broken.
> Tom> I must not have re-run the python tests locally after writing it :(
> 
> Tom> The problem is that prevent_dont_repeat returns a cleanup, which is then
> Tom> left dangling after this patch.
> 
> Here's the updated patch.
> I ran this one through the buildbot.
> 

In some other local patch that I hadn't posted, I handled a similar
situation of save/restoring some global that we don't want to expose
by making the "make_cleanup_..." function return a scoped_restore, which
avoids having to create a new class.  It seems a bit simpler that
creating a class to me, and maybe a tiny bit more efficient code-space
wise (rtti?)  Did you consider this approach?  In this case, it'd look
like this:

From 3138a3fb72f76d0d1bb5a2e1db57e450c37dd43a Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 20 Dec 2016 17:30:56 +0000
Subject: [PATCH] Return scoped_restore instead of cleanup

---
 gdb/breakpoint.c |  2 +-
 gdb/command.h    |  2 +-
 gdb/top.c        | 16 ++++++----------
 3 files changed, 8 insertions(+), 12 deletions(-)
  

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

Pedro> In some other local patch that I hadn't posted, I handled a similar
Pedro> situation of save/restoring some global that we don't want to expose
Pedro> by making the "make_cleanup_..." function return a scoped_restore, which
Pedro> avoids having to create a new class.  It seems a bit simpler that
Pedro> creating a class to me, and maybe a tiny bit more efficient code-space
Pedro> wise (rtti?)  Did you consider this approach?

I didn't, but I like it, so I'll make this change.

Tom
  

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

Pedro> -extern struct cleanup *prevent_dont_repeat (void);
Pedro> +extern scoped_restore_tmpl<bool> prevent_dont_repeat ();

Probably this should use __attribute__((warn_unused_result)), but that
isn't wrapped in ansidecl.h (yet) and I don't know offhand what version
test to use for it.  Do you?

Tom
  

On 12/20/2016 11:27 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> -extern struct cleanup *prevent_dont_repeat (void);
> Pedro> +extern scoped_restore_tmpl<bool> prevent_dont_repeat ();
> 
> Probably this should use __attribute__((warn_unused_result)), but that
> isn't wrapped in ansidecl.h (yet) and I don't know offhand what version
> test to use for it.  Do you?

Good idea.  Looks like it's GCC 3.4.  glibc has:

/* If fortification mode, we warn about unused results of certain
   function calls which can lead to problems.  */
#if __GNUC_PREREQ (3,4)
# define __attribute_warn_unused_result__ \
   __attribute__ ((__warn_unused_result__))
# if __USE_FORTIFY_LEVEL > 0
#  define __wur __attribute_warn_unused_result__
# endif
#else
# define __attribute_warn_unused_result__ /* empty */
#endif
#ifndef __wur
# define __wur /* Ignore */
#endif

Grepping my F23's system headers I see other hits for GCC 3.4:

/usr/include/lzma.h:237:        /* warn_unused_result was added in GCC 3.4. */
/usr/include/llvm/Support/Compiler.h:125:#if __has_attribute(warn_unused_result) || LLVM_GNUC_PREREQ(3, 4, 0)
/usr/include/llvm/Support/Compiler.h:126:#define LLVM_ATTRIBUTE_UNUSED_RESULT __attribute__((__warn_unused_result__))

Thanks,
Pedro Alves
  

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

Tom> Probably this should use __attribute__((warn_unused_result)), but that
Tom> isn't wrapped in ansidecl.h (yet) and I don't know offhand what version
Tom> test to use for it.  Do you?

Pedro> Good idea.  Looks like it's GCC 3.4.

This turns out not to work :(

FWIW in addition to the ATTRIBUTE_WARN_UNUSED_RESULT patch, I also had
to patch gdb's configure script to add -Wunused-result, as -Wunused
turns this off.

The g++ bug is known:

    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38172

It looks like the accepted fix upstream to use the C++17 [[nodiscard]]
attribute; not sure if we can use that somehow.

Meanwhile I'm going to drop this part of my patch.

Tom
  

On 12/22/2016 02:49 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Tom> Probably this should use __attribute__((warn_unused_result)), but that
> Tom> isn't wrapped in ansidecl.h (yet) and I don't know offhand what version
> Tom> test to use for it.  Do you?
> 
> Pedro> Good idea.  Looks like it's GCC 3.4.
> 
> This turns out not to work :(
> 
> FWIW in addition to the ATTRIBUTE_WARN_UNUSED_RESULT patch, I also had
> to patch gdb's configure script to add -Wunused-result, as -Wunused
> turns this off.
> 

Bummer.  :-(

> The g++ bug is known:
> 
>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38172
> 

> It looks like the accepted fix upstream to use the C++17 [[nodiscard]]
> attribute; not sure if we can use that somehow.

Can't see why not, if we put it behind some ATTRIBUTE_NODISCARD or
some such define.  Sounds like with that we could put the attribute in
the scoped_restore_impl template itself ('struct [[nodiscard]] foo'),
and get the warning for all similar make_foo_scoped_restore-like
functions for free.

> 
> Meanwhile I'm going to drop this part of my patch.

That's fine, we can always add it later.  Thanks much for experimenting.

Thanks,
Pedro Alves
  

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

>> It looks like the accepted fix upstream to use the C++17 [[nodiscard]]
>> attribute; not sure if we can use that somehow.

Pedro> Can't see why not, if we put it behind some ATTRIBUTE_NODISCARD or
Pedro> some such define.

It wasn't obvious to me (and as you can tell I'm not doing much research
into this kind of thing lately...) what condition to use.  C++17 isn't
official yet, and presumably even if we had some value for __cplusplus
to check, we'd also have to check for when exactly it was added to g++?

Tom
  

On 12/22/2016 03:29 PM, Tom Tromey wrote:

> C++17 isn't
> official yet, and presumably even if we had some value for __cplusplus
> to check, we'd also have to check for when exactly it was added to g++?

According to:

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

that could be checked with __has_cpp_attribute(nodiscard).

From that page, seems like G++ 4.8 had an [[gnu::warn_unused_result]]
alternative that perhaps works on C++11 too.  I have no experience
with it though.

But as I said, we can always add this later.

Thanks,
Pedro Alves