Re: [PATCH v3 06/17] Replace delete_longjmp_breakpoint_cleanup with a forward_scope_exit type

Re: [PATCH v3 06/17] Replace delete_longjmp_breakpoint_cleanup with a forward_scope_exit type

[Pavel I. Kryukov]

The patch causes build errors on XCode:

$ clang --version
Apple LLVM version 10.0.0 (clang-1000.11.45.5)
Target: x86_64-apple-darwin17.7.0
Thread model: posix
InstalledDir: /Applications/Xcode10.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin


In file included from breakpoint.c:34:
In file included from ./inferior.h:54:
./common/forward-scope-exit.h:98:7: error: no matching constructor for
initialization of 'decltype(std::bind(&delete_longjmp_breakpoint,
std::declval<int>()))' (aka '__bind<void (*)(int), int>')
    : m_bind_function (std::bind (function, args...))
      ^                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./common/gdb_optional.h:155:19: note: in instantiation of member
function 'detail::forward_scope_exit<void (int),
&delete_longjmp_breakpoint, void (int)>::forward_scope_exit' requested
here
    new (&m_item) T (std::forward<Args>(args)...);
                  ^
breakpoint.c:11127:18: note: in instantiation of function template
specialization 'gdb::optional<detail::forward_scope_exit<void (int),
&delete_longjmp_breakpoint, void (int)> >::emplace<int &>' requested
here
      lj_deleter.emplace (thread);
                 ^
/Applications/Xcode-10.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/functional:2220:7:
note: candidate constructor (the implicit copy constructor) not
viable: no known conversion from '__bind<[...], int &>' to 'const
__bind<[...], int>' for 1st argument
class __bind
      ^
/Applications/Xcode-10.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/functional:2220:7:
note: candidate constructor (the implicit move constructor) not
viable: no known conversion from '__bind<[...], int &>' to
'__bind<[...], int>' for 1st argument
/Applications/Xcode-10.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/functional:2240:16:
note: candidate template ignored: requirement 'is_constructible<_Fd,
__bind<void (*)(int), int &> >::value' was not satisfied [with _Gp =
std::__1::__bind<void (*)(int), int &>, _BA = <>]
      explicit __bind(_Gp&& __f, _BA&& ...__bound_args)
               ^
1 error generated.
make: *** [breakpoint.o] Error 1

Thanks,
--
Pavel

Re: [PATCH v3 06/17] Replace delete_longjmp_breakpoint_cleanup with a forward_scope_exit type

[Pedro Alves]

On 01/24/2019 11:23 AM, Pavel I. Kryukov wrote:
> The patch causes build errors on XCode:
> 
> $ clang --version
> Apple LLVM version 10.0.0 (clang-1000.11.45.5)
> Target: x86_64-apple-darwin17.7.0
> Thread model: posix
> InstalledDir: /Applications/Xcode10.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

Thanks.

Hmm, I only have clang 5.0.2 handy, and I think it links with
libstdc++ instead of clang's own libc++.  Here it builds fine.
I had also made sure the patch builds fine with g++ 4.8.

> 
> 
> In file included from breakpoint.c:34:
> In file included from ./inferior.h:54:
> ./common/forward-scope-exit.h:98:7: error: no matching constructor for
> initialization of 'decltype(std::bind(&delete_longjmp_breakpoint,
> std::declval<int>()))' (aka '__bind<void (*)(int), int>')
>     : m_bind_function (std::bind (function, args...))
>       ^                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ./common/gdb_optional.h:155:19: note: in instantiation of member
> function 'detail::forward_scope_exit<void (int),
> &delete_longjmp_breakpoint, void (int)>::forward_scope_exit' requested
> here
>     new (&m_item) T (std::forward<Args>(args)...);
>                   ^
> breakpoint.c:11127:18: note: in instantiation of function template
> specialization 'gdb::optional<detail::forward_scope_exit<void (int),
> &delete_longjmp_breakpoint, void (int)> >::emplace<int &>' requested
> here
>       lj_deleter.emplace (thread);
>                  ^
> /Applications/Xcode-10.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/functional:2220:7:
> note: candidate constructor (the implicit copy constructor) not
> viable: no known conversion from '__bind<[...], int &>' to 'const
> __bind<[...], int>' for 1st argument
> class __bind
>       ^

Hmm, references to the copy ctor.  I don't really know why I ended up
with a copy here.  We can just pass the arguments directly to the
being-constructed bind.  Does this work for you?

From b1caeb9c5334a59680af7b7717ab620e5a912bcf Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 24 Jan 2019 16:25:44 +0000
Subject: [PATCH] No copy ctor

---
 gdb/common/forward-scope-exit.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/common/forward-scope-exit.h b/gdb/common/forward-scope-exit.h
index 8d639151a4..bffc6e683b 100644
--- a/gdb/common/forward-scope-exit.h
+++ b/gdb/common/forward-scope-exit.h
@@ -95,7 +95,7 @@ class forward_scope_exit<Function, function, Res (Args...)>
 
 public:
   explicit forward_scope_exit (Args ...args)
-    : m_bind_function (std::bind (function, args...))
+    : m_bind_function (function, args...)
   {
     /* Nothing.  */
   }
-- 
2.14.4

Re: [PATCH v3 06/17] Replace delete_longjmp_breakpoint_cleanup with a forward_scope_exit type

[John Baldwin]

On 1/24/19 9:09 AM, Pedro Alves wrote:
> On 01/24/2019 11:23 AM, Pavel I. Kryukov wrote:
>> The patch causes build errors on XCode:
>>
>> $ clang --version
>> Apple LLVM version 10.0.0 (clang-1000.11.45.5)
>> Target: x86_64-apple-darwin17.7.0
>> Thread model: posix
>> InstalledDir: /Applications/Xcode10.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
> 
> Thanks.
> 
> Hmm, I only have clang 5.0.2 handy, and I think it links with
> libstdc++ instead of clang's own libc++.  Here it builds fine.
> I had also made sure the patch builds fine with g++ 4.8.
> 
>>
>>
>> In file included from breakpoint.c:34:
>> In file included from ./inferior.h:54:
>> ./common/forward-scope-exit.h:98:7: error: no matching constructor for
>> initialization of 'decltype(std::bind(&delete_longjmp_breakpoint,
>> std::declval<int>()))' (aka '__bind<void (*)(int), int>')
>>     : m_bind_function (std::bind (function, args...))
>>       ^                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ./common/gdb_optional.h:155:19: note: in instantiation of member
>> function 'detail::forward_scope_exit<void (int),
>> &delete_longjmp_breakpoint, void (int)>::forward_scope_exit' requested
>> here
>>     new (&m_item) T (std::forward<Args>(args)...);
>>                   ^
>> breakpoint.c:11127:18: note: in instantiation of function template
>> specialization 'gdb::optional<detail::forward_scope_exit<void (int),
>> &delete_longjmp_breakpoint, void (int)> >::emplace<int &>' requested
>> here
>>       lj_deleter.emplace (thread);
>>                  ^
>> /Applications/Xcode-10.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/functional:2220:7:
>> note: candidate constructor (the implicit copy constructor) not
>> viable: no known conversion from '__bind<[...], int &>' to 'const
>> __bind<[...], int>' for 1st argument
>> class __bind
>>       ^
> 
> Hmm, references to the copy ctor.  I don't really know why I ended up
> with a copy here.  We can just pass the arguments directly to the
> being-constructed bind.  Does this work for you?
> 
> From b1caeb9c5334a59680af7b7717ab620e5a912bcf Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 24 Jan 2019 16:25:44 +0000
> Subject: [PATCH] No copy ctor
> 
> ---
>  gdb/common/forward-scope-exit.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/common/forward-scope-exit.h b/gdb/common/forward-scope-exit.h
> index 8d639151a4..bffc6e683b 100644
> --- a/gdb/common/forward-scope-exit.h
> +++ b/gdb/common/forward-scope-exit.h
> @@ -95,7 +95,7 @@ class forward_scope_exit<Function, function, Res (Args...)>
>  
>  public:
>    explicit forward_scope_exit (Args ...args)
> -    : m_bind_function (std::bind (function, args...))
> +    : m_bind_function (function, args...)
>    {
>      /* Nothing.  */
>    }
> 

I got the same failure on FreeBSD (also uses libc++) and this patch fixed
the build for me.

-- 
John Baldwin

                                                                            

[pushed] Fix clang/libc++ build (Re: [PATCH v3 06/17] Replace delete_longjmp_breakpoint_cleanup with a forward_scope_exit type)

[Pedro Alves]

On 01/24/2019 05:33 PM, John Baldwin wrote:

> I got the same failure on FreeBSD (also uses libc++) and this patch fixed
> the build for me.

In that case, I've merged the patch in, as below.

Thanks,
Pedro Alves

From 3046d67a0e29686ec18abd719660969c97973063 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 24 Jan 2019 18:01:49 +0000
Subject: [PATCH] Fix clang/libc++ build

This fixes the following build error with clang/libc++, reported at
<https://sourceware.org/ml/gdb-patches/2019-01/msg00537.html>:

  (...)
  In file included from breakpoint.c:34:
  In file included from ./inferior.h:54:
  ./common/forward-scope-exit.h:98:7: error: no matching constructor for
  initialization of 'decltype(std::bind(&delete_longjmp_breakpoint,
  std::declval<int>()))' (aka '__bind<void (*)(int), int>')
      : m_bind_function (std::bind (function, args...))
	^                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ./common/gdb_optional.h:155:19: note: in instantiation of member
  function 'detail::forward_scope_exit<void (int),
  &delete_longjmp_breakpoint, void (int)>::forward_scope_exit' requested
  here
      new (&m_item) T (std::forward<Args>(args)...);
		    ^
  breakpoint.c:11127:18: note: in instantiation of function template
  specialization 'gdb::optional<detail::forward_scope_exit<void (int),
  &delete_longjmp_breakpoint, void (int)> >::emplace<int &>' requested
  here
	lj_deleter.emplace (thread);
		   ^
  /Applications/Xcode-10.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/functional:2220:7:
  note: candidate constructor (the implicit copy constructor) not
  viable: no known conversion from '__bind<[...], int &>' to 'const
  __bind<[...], int>' for 1st argument
  class __bind
	^
  (...)

I don't really know why I ended up with a copy here.  We can just pass
the arguments directly to the being-constructed bind.

gdb/ChangeLog:
2019-01-24  Pedro Alves  <palves@redhat.com>

	* common/forward-scope-exit.h
	(forward_scope_exit::forward_scope_exit): Pass arguments to
	m_bind_function directly, instead of creating a std::bind and
	copying that.
---
 gdb/ChangeLog                   | 7 +++++++
 gdb/common/forward-scope-exit.h | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 453677e599..e8e01288d9 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2019-01-24  Pedro Alves  <palves@redhat.com>
+
+	* common/forward-scope-exit.h
+	(forward_scope_exit::forward_scope_exit): Pass arguments to
+	m_bind_function directly, instead of creating a std::bind and
+	copying that.
+
 2019-01-24  Alan Hayward  <alan.hayward@arm.com>
 
 	* aarch64-tdep.c (aapcs_is_vfp_call_or_return_candidate_1): Check
diff --git a/gdb/common/forward-scope-exit.h b/gdb/common/forward-scope-exit.h
index 8d639151a4..bffc6e683b 100644
--- a/gdb/common/forward-scope-exit.h
+++ b/gdb/common/forward-scope-exit.h
@@ -95,7 +95,7 @@ class forward_scope_exit<Function, function, Res (Args...)>
 
 public:
   explicit forward_scope_exit (Args ...args)
-    : m_bind_function (std::bind (function, args...))
+    : m_bind_function (function, args...)
   {
     /* Nothing.  */
   }
-- 
2.14.4

[PATCH v3 00/17] Remove some cleanups using scope_exit

[Pedro Alves]

Here's the new version of Tromey's "cleanup function" series, updated
to use the new scope_exit instead.

v1 was here:
  https://sourceware.org/ml/gdb-patches/2019-01/msg00159.html

I'm calling Andrew's patches the "v2":
  https://sourceware.org/ml/gdb-patches/2019-01/msg00273.html

The discussion around "v2" led to this version.

New in v3:

   - cleanup_function is replaced with a new scope_exit template
     class.  scope_exit is a general-purpose scope guard that calls
     its exit function at the end of the current scope.  Unlike the
     original cleanup_function, scope_exit is a template, and saves
     the callable internaly (function pointer, function object,
     lambda) instead of using gdb::function_view.  This leads to a bit
     more natural syntax, since there's no need to worry about
     lifetime of the callable.  Also, being a template, there's no
     heap involved, unlike if we used std::function.

   - forward_scope_exit is new.  The idea is based on Andrew's here:
      https://sourceware.org/ml/gdb-patches/2019-01/msg00273.html
     though the implementation is different, and this version ends up
     with a simpler user interface.

   - Patches #1 and #5 are new.

   - Patches #8 and #9 and swapped compared to the original series,
     and a cleanup conversion was moved between the patches.

   - Patch #7 removes a bit more comment than the previous version.

   - Series updated throughout to use
     scope_exit/SCOPE_EXIT/make_scope_exit/forward_scope_exit as
     appropriate.

Pedro Alves (6):
  Rename ESC -> ESC_PARENS
  Introduce scope_exit
  Introduce forward_scope_exit
  Use forward_scope_exit for scoped_finish_thread_state
  Use SCOPE_EXIT in gdbarch-selftest.c
  Remove clear_symtab_users_cleanup

Tom Tromey (11):
  Replace delete_longjmp_breakpoint_cleanup with a forward_scope_exit
    type
  Remove remaining cleanup from gdb/breakpoint.c
  Remove delete_just_stopped_threads_infrun_breakpoints_cleanup
  Remove make_bpstat_clear_actions_cleanup
  Remove cleanup_delete_std_terminate_breakpoint
  Remove cleanup from linux-nat.c
  Remove cleanup from stop_all_threads
  Remove remaining cleanup from fetch_inferior_event
  Update an obsolete cleanup comment
  Update cleanup comment in ui-out.h
  Use scope_exit in regcache.c

 gdb/breakpoint.c                |  27 ++---
 gdb/common/forward-scope-exit.h | 123 +++++++++++++++++++++
 gdb/common/preprocessor.h       |   2 +-
 gdb/common/scope-exit.h         | 186 ++++++++++++++++++++++++++++++++
 gdb/common/valid-expr.h         |  18 ++--
 gdb/gdbarch-selftests.c         |   8 +-
 gdb/gdbthread.h                 |  28 +----
 gdb/infcall.c                   |  13 +--
 gdb/infcmd.c                    |  12 +--
 gdb/inferior.h                  |   4 +-
 gdb/infrun.c                    | 230 ++++++++++++++++++----------------------
 gdb/linux-nat.c                 |  18 +---
 gdb/regcache.c                  |  34 +-----
 gdb/symfile.c                   |  24 ++---
 gdb/top.c                       |   8 +-
 gdb/ui-out.h                    |   8 +-
 gdb/utils.c                     |  17 ---
 gdb/utils.h                     |   1 -
 18 files changed, 465 insertions(+), 296 deletions(-)
 create mode 100644 gdb/common/forward-scope-exit.h
 create mode 100644 gdb/common/scope-exit.h

-- 
2.14.4

[PATCH v3 06/17] Replace delete_longjmp_breakpoint_cleanup with a forward_scope_exit type

[Pedro Alves]

From: Tom Tromey <tom@tromey.com>

This removes delete_longjmp_breakpoint_cleanup in favor of forward_scope_exit.

gdb/ChangeLog:
yyyy-mm-dd  Tom Tromey  <tom@tromey.com>
	    Andrew Burgess  <andrew.burgess@embecosm.com>
	    Pedro Alves  <palves@redhat.com>

	* breakpoint.c (until_break_command): Use
	delete_longjmp_breakpoint_cleanup class.
	* infcmd.c (delete_longjmp_breakpoint_cleanup): Remove function.
	(until_next_command): Use delete_longjmp_breakpoint_cleanup class.
	* inferior.h: Include forward-scope-exit.h.
	(delete_longjmp_breakpoint_cleanup): Replace function declaration
	with FORWARD_SCOPE_EXIT type.
---
 gdb/breakpoint.c | 11 ++++++-----
 gdb/infcmd.c     | 12 ++----------
 gdb/inferior.h   |  4 +++-
 3 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3166fa0129..999809c312 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -11073,7 +11073,6 @@ until_break_command (const char *arg, int from_tty, int anywhere)
   struct gdbarch *frame_gdbarch;
   struct frame_id stack_frame_id;
   struct frame_id caller_frame_id;
-  struct cleanup *old_chain;
   int thread;
   struct thread_info *tp;
   struct until_break_fsm *sm;
@@ -11106,8 +11105,6 @@ until_break_command (const char *arg, int from_tty, int anywhere)
   tp = inferior_thread ();
   thread = tp->global_num;
 
-  old_chain = make_cleanup (null_cleanup, NULL);
-
   /* Note linespec handling above invalidates the frame chain.
      Installing a breakpoint also invalidates the frame chain (as it
      may need to switch threads), so do any frame handling before
@@ -11122,6 +11119,9 @@ until_break_command (const char *arg, int from_tty, int anywhere)
      one.  */
 
   breakpoint_up caller_breakpoint;
+
+  gdb::optional<delete_longjmp_breakpoint_cleanup> lj_deleter;
+
   if (frame_id_p (caller_frame_id))
     {
       struct symtab_and_line sal2;
@@ -11136,7 +11136,7 @@ until_break_command (const char *arg, int from_tty, int anywhere)
 						    bp_until);
 
       set_longjmp_breakpoint (tp, caller_frame_id);
-      make_cleanup (delete_longjmp_breakpoint_cleanup, &thread);
+      lj_deleter.emplace (thread);
     }
 
   /* set_momentary_breakpoint could invalidate FRAME.  */
@@ -11159,7 +11159,8 @@ until_break_command (const char *arg, int from_tty, int anywhere)
 			    std::move (caller_breakpoint));
   tp->thread_fsm = &sm->thread_fsm;
 
-  discard_cleanups (old_chain);
+  if (lj_deleter)
+    lj_deleter->release ();
 
   proceed (-1, GDB_SIGNAL_DEFAULT);
 }
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 3c3add89ab..fafb7e2ec4 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -947,13 +947,6 @@ nexti_command (const char *count_string, int from_tty)
   step_1 (1, 1, count_string);
 }
 
-void
-delete_longjmp_breakpoint_cleanup (void *arg)
-{
-  int thread = * (int *) arg;
-  delete_longjmp_breakpoint (thread);
-}
-
 /* Data for the FSM that manages the step/next/stepi/nexti
    commands.  */
 
@@ -1517,7 +1510,6 @@ until_next_command (int from_tty)
   struct symtab_and_line sal;
   struct thread_info *tp = inferior_thread ();
   int thread = tp->global_num;
-  struct cleanup *old_chain;
   struct until_next_fsm *sm;
 
   clear_proceed_status (0);
@@ -1556,11 +1548,11 @@ until_next_command (int from_tty)
   tp->control.step_over_calls = STEP_OVER_ALL;
 
   set_longjmp_breakpoint (tp, get_frame_id (frame));
-  old_chain = make_cleanup (delete_longjmp_breakpoint_cleanup, &thread);
+  delete_longjmp_breakpoint_cleanup lj_deleter (thread);
 
   sm = new_until_next_fsm (command_interp (), tp->global_num);
   tp->thread_fsm = &sm->thread_fsm;
-  discard_cleanups (old_chain);
+  lj_deleter.release ();
 
   proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
 }
diff --git a/gdb/inferior.h b/gdb/inferior.h
index a82df1a52a..cadaaedf22 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -51,6 +51,7 @@ struct thread_info;
 
 #include "symfile-add-flags.h"
 #include "common/refcounted-object.h"
+#include "common/forward-scope-exit.h"
 
 #include "common-inferior.h"
 #include "gdbthread.h"
@@ -198,7 +199,8 @@ extern void continue_1 (int all_threads);
 
 extern void interrupt_target_1 (int all_threads);
 
-extern void delete_longjmp_breakpoint_cleanup (void *arg);
+using delete_longjmp_breakpoint_cleanup
+  = FORWARD_SCOPE_EXIT (delete_longjmp_breakpoint);
 
 extern void detach_command (const char *, int);
 
-- 
2.14.4