[RFC 0/2, gdbserver] Set linux target in async mode in default

[RFC 0/2, gdbserver] Set linux target in async mode in default

[Yao Qi]

Hi,
When writing the V2 of 'A general notification in GDB RSP' patch
series, I find async mode isn't turned on unless non-stop mode is
turned on.  After I generalize 'async notification' and support both
non-stop and all-stop, it is required to turn async on even in all-stop
mode.  Async can be regarded as an infrastructure in linux target to
serve for other functionalities, such as 'notification' and
'non-stop'.  This is what patch 1/2 tries to do.  Regression tested on
x86_64-linux with boardfile ntaive-gdbserver and
native-extended-gdbserver.

After patch 1/2 is applied, target_async is not used in common code,
so patch 2/2 is to remove macro target_async and field 'async' in
struct 'target_ops' as a cleanup.

[PATCH 2/2] gdbserver:Remove async from target_ops

[Yao Qi]

Hi,
After patch 1/2 is applied, 'target_async' is not used in common code any
more, this patch is to clean up.

gdb/gdbserver:

2012-09-18  Yao Qi  <yao@codesourcery.com>

	* target.h (struct target_ops) <asnyc>: Remove.
	(target_async): Remove macro.
	* linux-low.c (linux_target_ops): Update.
	* lynx-low.c (lynx_target_ops): Update.
	* nto-low.c (nto_target_ops): Update.
	* win32-low.c (win32_target_ops): Update.
---
 gdb/gdbserver/linux-low.c |    1 -
 gdb/gdbserver/lynx-low.c  |    1 -
 gdb/gdbserver/nto-low.c   |    1 -
 gdb/gdbserver/target.h    |    7 -------
 gdb/gdbserver/win32-low.c |    1 -
 5 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index faed578..f4d9a7d 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -5823,7 +5823,6 @@ static struct target_ops linux_target_ops = {
   linux_qxfer_osdata,
   linux_xfer_siginfo,
   linux_supports_non_stop,
-  linux_async,
   linux_start_non_stop,
   linux_supports_multi_process,
 #ifdef USE_THREAD_DB
diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
index 2c7ab6e..38029b8 100644
--- a/gdb/gdbserver/lynx-low.c
+++ b/gdb/gdbserver/lynx-low.c
@@ -755,7 +755,6 @@ static struct target_ops lynx_target_ops = {
   NULL,  /* qxfer_osdata */
   NULL,  /* qxfer_siginfo */
   NULL,  /* supports_non_stop */
-  NULL,  /* async */
   NULL,  /* start_non_stop */
   NULL,  /* supports_multi_process */
   NULL,  /* handle_monitor_command */
diff --git a/gdb/gdbserver/nto-low.c b/gdb/gdbserver/nto-low.c
index 73618cd..1a0f5c4 100644
--- a/gdb/gdbserver/nto-low.c
+++ b/gdb/gdbserver/nto-low.c
@@ -933,7 +933,6 @@ static struct target_ops nto_target_ops = {
   NULL, /* nto_qxfer_osdata */
   NULL, /* xfer_siginfo */
   nto_supports_non_stop,
-  NULL, /* async */
   NULL  /* start_non_stop */
 };
 
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index 9f96e04..6b60e13 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -292,10 +292,6 @@ struct target_ops
 
   int (*supports_non_stop) (void);
 
-  /* Enables async target events.  Returns the previous enable
-     state.  */
-  int (*async) (int enable);
-
   /* Switch to non-stop (1) or all-stop (0) mode.  Return 0 on
      success, -1 otherwise.  */
   int (*start_non_stop) (int);
@@ -432,9 +428,6 @@ int kill_inferior (int);
 #define target_supports_non_stop() \
   (the_target->supports_non_stop ? (*the_target->supports_non_stop ) () : 0)
 
-#define target_async(enable) \
-  (the_target->async ? (*the_target->async) (enable) : 0)
-
 #define target_supports_multi_process() \
   (the_target->supports_multi_process ? \
    (*the_target->supports_multi_process) () : 0)
diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index 4d5fe67..cbdf656 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -1806,7 +1806,6 @@ static struct target_ops win32_target_ops = {
   NULL, /* qxfer_osdata */
   NULL, /* qxfer_siginfo */
   NULL, /* supports_non_stop */
-  NULL, /* async */
   NULL, /* start_non_stop */
   NULL, /* supports_multi_process */
   NULL, /* handle_monitor_command */
-- 
1.7.7.6

[PATCH 1/2] gdbserver:Set linux target in async mode in default

[Yao Qi]

Set gdbserver linux in async mode in default.

gdb/gdbserver:

2012-09-18  Yao Qi  <yao@codesourcery.com>

	* linux-low.c (target_is_async_p): Remove macro.
	(linux_wait) Don't check target_is_async_p.  Check 'non_stop'.
	(sigchld_handler): Don't check target_is_async_p, check
	non_stop instead.
	(linux_async): Don't call linux_async.
	(initialize_low): Call linux_async.
	* remote-utils.c (handle_accept_event): Don't call
	target_async, setting 'non_stop' to zero instead.
---
 gdb/gdbserver/linux-low.c    |   15 +++++----------
 gdb/gdbserver/remote-utils.c |    2 +-
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index a476031..faed578 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -264,9 +264,6 @@ static int num_regsets;
    event loop.  */
 static int linux_event_pipe[2] = { -1, -1 };
 
-/* True if we're currently in async mode.  */
-#define target_is_async_p() (linux_event_pipe[0] != -1)
-
 static void send_sigstop (struct lwp_info *lwp);
 static void wait_for_sigstop (struct inferior_list_entry *entry);
 
@@ -2797,15 +2794,13 @@ linux_wait (ptid_t ptid,
     fprintf (stderr, "linux_wait: [%s]\n", target_pid_to_str (ptid));
 
   /* Flush the async file first.  */
-  if (target_is_async_p ())
-    async_file_flush ();
+  async_file_flush ();
 
   event_ptid = linux_wait_1 (ptid, ourstatus, target_options);
 
   /* If at least one stop was reported, there may be more.  A single
      SIGCHLD can signal more than one child stop.  */
-  if (target_is_async_p ()
-      && (target_options & TARGET_WNOHANG) != 0
+  if (non_stop && (target_options & TARGET_WNOHANG) != 0
       && !ptid_equal (event_ptid, null_ptid))
     async_file_mark ();
 
@@ -4962,7 +4957,7 @@ sigchld_handler (int signo)
 	} while (0);
     }
 
-  if (target_is_async_p ())
+  if (non_stop)
     async_file_mark (); /* trigger a linux_wait */
 
   errno = old_errno;
@@ -5025,8 +5020,6 @@ linux_async (int enable)
 static int
 linux_start_non_stop (int nonstop)
 {
-  /* Register or unregister from event-loop accordingly.  */
-  linux_async (nonstop);
   return 0;
 }
 
@@ -5889,4 +5882,6 @@ initialize_low (void)
   sigemptyset (&sigchld_action.sa_mask);
   sigchld_action.sa_flags = SA_RESTART;
   sigaction (SIGCHLD, &sigchld_action, NULL);
+
+  linux_async (1);
 }
diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index 0b3adac..8a01da7 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -211,7 +211,7 @@ handle_accept_event (int err, gdb_client_data client_data)
      try to send vStopped notifications to GDB.  But, don't do that
      until GDB as selected all-stop/non-stop, and has queried the
      threads' status ('?').  */
-  target_async (0);
+  non_stop = 0;
 
   return 0;
 }
-- 
1.7.7.6

RE: [RFC 0/2, gdbserver] Set linux target in async mode in default

[Marc Khouzam]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org 
> [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Yao Qi
> Sent: Tuesday, September 18, 2012 5:48 AM
> To: gdb-patches@sourceware.org
> Subject: [RFC 0/2, gdbserver] Set linux target in async mode 
> in default
> 
> Hi,
> When writing the V2 of 'A general notification in GDB RSP' patch
> series, I find async mode isn't turned on unless non-stop mode is
> turned on.  After I generalize 'async notification' and support both
> non-stop and all-stop, it is required to turn async on even 
> in all-stop
> mode.  Async can be regarded as an infrastructure in linux target to
> serve for other functionalities, such as 'notification' and
> 'non-stop'.  This is what patch 1/2 tries to do.  Regression tested on
> x86_64-linux with boardfile ntaive-gdbserver and
> native-extended-gdbserver.

To clarify, after this patch we can still use "-gdb-set target-async off"
to turn off async mode right?

Eclipse currently uses non-async mode for all-stop and some code
is based on that assumption (e.g., interrupting the target)
so we need to keep non-async available.

Thanks

Marc

> 
> After patch 1/2 is applied, target_async is not used in common code,
> so patch 2/2 is to remove macro target_async and field 'async' in
> struct 'target_ops' as a cleanup.
> 
> 

Re: [RFC 0/2, gdbserver] Set linux target in async mode in default

[Yao Qi]

On 09/18/2012 10:12 PM, Marc Khouzam wrote:
> To clarify, after this patch we can still use "-gdb-set target-async off"
> to turn off async mode right?
>

This patch doesn't change the behaviour from the GDB's and Eclipse's 
perspective.  We can still set target-async on or off in GDB side as needed.

IIUC, the term "async mode" in GDBserver is different from its 
counterpart in GDB.  In GDBserver, "async mode" means an "async event 
loop" which is used for non-stop and notification.  If we look at RSP,
there is no command to turn "async mode" on or off directly in GDBserver 
(QNonStop can start async mode on linux target), the state of "async 
mode" of GDBserver is unknown to GDB, and vice versa.

In current GDBserver/linux, we have only two modes, 'non-stop with 
async-event-loop' and 'all-stop without async-event-loop'.  This patch 
only changes the latter to 'all-stop with async-event-loop', so that 
async notification can work on top it.  At the same time, GDB still 
works in both "target-async on" and "target-async off" with this patched 
GDbserver.

> Eclipse currently uses non-async mode for all-stop and some code
> is based on that assumption (e.g., interrupting the target)
> so we need to keep non-async available.

Forgot to mention in my mail that I run testsuite with 
{native-gdbserver, native-extended-gdbserver} x {async, sync} (in GDB 
side), and no regression.

-- 
Yao

RE: [RFC 0/2, gdbserver] Set linux target in async mode in default

[Marc Khouzam]

> -----Original Message-----
> From: Yao Qi [mailto:yao@codesourcery.com] 
> Sent: Tuesday, September 18, 2012 10:35 AM
> To: Marc Khouzam
> Cc: 'gdb-patches@sourceware.org'
> Subject: Re: [RFC 0/2, gdbserver] Set linux target in async 
> mode in default
> 
> On 09/18/2012 10:12 PM, Marc Khouzam wrote:
> > To clarify, after this patch we can still use "-gdb-set 
> target-async off"
> > to turn off async mode right?
> >
> 
> This patch doesn't change the behaviour from the GDB's and Eclipse's 
> perspective.  We can still set target-async on or off in GDB 
> side as needed.
> 
> IIUC, the term "async mode" in GDBserver is different from its 
> counterpart in GDB.  In GDBserver, "async mode" means an "async event 
> loop" which is used for non-stop and notification.  If we look at RSP,
> there is no command to turn "async mode" on or off directly 
> in GDBserver 
> (QNonStop can start async mode on linux target), the state of "async 
> mode" of GDBserver is unknown to GDB, and vice versa.

Sorry, I missed that this was for gdbserver only.


> 
> In current GDBserver/linux, we have only two modes, 'non-stop with 
> async-event-loop' and 'all-stop without async-event-loop'.  
> This patch 
> only changes the latter to 'all-stop with async-event-loop', so that 
> async notification can work on top it.  At the same time, GDB still 
> works in both "target-async on" and "target-async off" with 
> this patched 
> GDbserver.
> 
> > Eclipse currently uses non-async mode for all-stop and some code
> > is based on that assumption (e.g., interrupting the target)
> > so we need to keep non-async available.
> 
> Forgot to mention in my mail that I run testsuite with 
> {native-gdbserver, native-extended-gdbserver} x {async, sync} (in GDB 
> side), and no regression.
> 
> -- 
> Yao
> 

Re: [RFC 0/2, gdbserver] Set linux target in async mode in default

[Pedro Alves]

On 09/18/2012 10:47 AM, Yao Qi wrote:
> Hi,
> When writing the V2 of 'A general notification in GDB RSP' patch
> series, I find async mode isn't turned on unless non-stop mode is
> turned on.  After I generalize 'async notification' and support both
> non-stop and all-stop, it is required to turn async on even in all-stop
> mode.  Async can be regarded as an infrastructure in linux target to
> serve for other functionalities, such as 'notification' and
> 'non-stop'.  This is what patch 1/2 tries to do.  Regression tested on
> x86_64-linux with boardfile ntaive-gdbserver and
> native-extended-gdbserver.
> 
> After patch 1/2 is applied, target_async is not used in common code,
> so patch 2/2 is to remove macro target_async and field 'async' in
> struct 'target_ops' as a cleanup.

Please explain better why this is necessary (probably with an example).
What exactly doesn't work if you don't do this change?  It sounds as though
this could make gdbserver push notifications down to GDB in all-stop
mode even when it isn't waiting for server replies?

-- 
Pedro Alves

Re: [RFC 0/2, gdbserver] Set linux target in async mode in default

[Yao Qi]

On 09/26/2012 12:23 AM, Pedro Alves wrote:
> Please explain better why this is necessary (probably with an example).
> What exactly doesn't work if you don't do this change?  It sounds as though

Hi, Pedro,
When we have a notification, we'll queue it and call 'async_file_mark'. 
  Then, notification can be sent in async handler 'handle_target_event' 
later, which is done by my later patch posted here, mainly in 
server.c:handle_target_event,

    [PATCH 3/6] de-couple %Stop from notification: gdbserver
    http://sourceware.org/ml/gdb-patches/2012-09/msg00480.html

The whole process is identical to all notifications, including %Stop. 
Actually, this process is generalized from %Stop.

For example, we need a notification on 'trace status', and GDBserver 
will send it once trace is stopped due to some reasons, such as 
tracebuffer is full.  We can add some code below to trigger event-loop, 
so that such notification can be sent later (it doesn't compile, just to 
show why do we need async for sending notification).

diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 201a25b..1135f53 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -3472,6 +3472,10 @@ stop_tracing (void)
      }

    unpause_all (1);
+
+#ifndef IN_PROCESS_AGENT
+  QUEUE_enque (notif_p, notif_queue, &notif_trace);
+  async_file_mark ();
+#endif
  }

  static int

Otherwise, when/how should we send notification in GDBserver?

> this could make gdbserver push notifications down to GDB in all-stop
> mode even when it isn't waiting for server replies?

I am assuming that 'it' means GDB', so answer is 'yes'.

If I understand 'non-stop for remote target' stuff correctly, %Stop 
notification is sent to GDB in non-stop mode even when GDB isn't waiting 
for server replies.  Then, what I am trying to achieve is 'notifications 
are sent to GDB in all-stop or non-stop mode even when GDB isn't waiting 
for server replies'.


-- 
Yao

Re: [RFC 0/2, gdbserver] Set linux target in async mode in default

[Yao Qi]

On 09/26/2012 12:23 AM, Pedro Alves wrote:
> Please explain better why this is necessary (probably with an example).
> What exactly doesn't work if you don't do this change?  It sounds as though
> this could make gdbserver push notifications down to GDB in all-stop
> mode even when it isn't waiting for server replies?

Pedro,
I think about your question and 'async notification' again, looks we 
have two options on the timing of sending '%' notifications,

   1.  in server.c:handle_target_event, in which %Stop is sent,
   2.  in the place where something interesting happens.  For example, 
if we have a notification trace stop, '%Trace' notification is sent at 
the end of tracepoint.c:stop_tracing.

My original design is to defer *all* notification sent in #1 
(handle_target_event), so async mode should be on.  Today, I find that 
notifications except %Stop can be sent in #2, and don't have to put them 
into #1.  If this sounds reasonable to you, this patch set is useless, 
and I'll revise my 'async notification' patch set in this way.

-- 
Yao