[01/10] Merge netbsd_ptrace_fun into the netbsd_process_target class

Message ID 20201002021804.2814-2-n54@gmx.com
State New
Headers show
Series
  • Refactor the NetBSD gdbserver support
Related show

Commit Message

Kamil Rytarowski Oct. 2, 2020, 2:17 a.m.
Change netbsd_ptrace_fun into a local lambda function, as otherwise
if it is a class member it is harder to pass it as a callbakc function
to the fork_inferior call.

No functional change.

gdb/ChangeLog:

	* netbsd-low.cc (netbsd_ptrace_fun): Turn into...
	(netbsd_process_target::create_inferior): ...lamda here.
---
 gdbserver/ChangeLog     |  5 ++++
 gdbserver/netbsd-low.cc | 65 ++++++++++++++++++++---------------------
 2 files changed, 37 insertions(+), 33 deletions(-)

--
2.28.0

Comments

Simon Marchi Oct. 7, 2020, 2:37 a.m. | #1
On 2020-10-01 10:17 p.m., Kamil Rytarowski wrote:
> Change netbsd_ptrace_fun into a local lambda function, as otherwise

> if it is a class member it is harder to pass it as a callbakc function


callbakc -> callback

I'm not sure I see the advantage of having it as a lambda over a free
function as it is now (not that I see a disadvantage either, and maybe
that will become evident in the following patches), but if it makes you
happy, this is ok.

> @@ -96,8 +64,39 @@ netbsd_process_target::create_inferior (const char *program,

>  {

>    std::string str_program_args = construct_inferior_arguments (program_args);

>

> +  /* Callback used by fork_inferior to start tracing the inferior.  */

> +  auto fn


I'd suggest choosing a more meaningful name than "fn", maybe
"traceme_fn", to match the fork_inferior parameter name.

Simon
Kamil Rytarowski Oct. 7, 2020, 4:36 a.m. | #2
On 07.10.2020 04:37, Simon Marchi wrote:
> On 2020-10-01 10:17 p.m., Kamil Rytarowski wrote:

>> Change netbsd_ptrace_fun into a local lambda function, as otherwise

>> if it is a class member it is harder to pass it as a callbakc function

> 

> callbakc -> callback

> 

> I'm not sure I see the advantage of having it as a lambda over a free

> function as it is now (not that I see a disadvantage either, and maybe

> that will become evident in the following patches), but if it makes you

> happy, this is ok.

> 


I prefer to keep all the local static functions in gdbserver citizens of
the NetBSD gdbserver. Long term, shared functionality with the native
target will go to gdb/nat and perhaps some functions like
elf_64_file_p() could be shared by ELF targets.

This refers to all the follow up patches.

>> @@ -96,8 +64,39 @@ netbsd_process_target::create_inferior (const char *program,

>>  {

>>    std::string str_program_args = construct_inferior_arguments (program_args);

>>

>> +  /* Callback used by fork_inferior to start tracing the inferior.  */

>> +  auto fn

> 

> I'd suggest choosing a more meaningful name than "fn", maybe

> "traceme_fn", to match the fork_inferior parameter name.

> 


I will do it.

> Simon

>
Simon Marchi Oct. 7, 2020, 11:19 a.m. | #3
On 2020-10-07 12:36 a.m., Kamil Rytarowski wrote:
> I prefer to keep all the local static functions in gdbserver citizens of

> the NetBSD gdbserver.


I don't understand what this means.

> Long term, shared functionality with the native

> target will go to gdb/nat and perhaps some functions like

> elf_64_file_p() could be shared by ELF targets.


If the goal is sharing them in gdb/nat, it seems to me like it's easier
to keep it as a free function, that will be easier to share.

Simon
Kamil Rytarowski Oct. 7, 2020, 12:27 p.m. | #4
On 07.10.2020 13:19, Simon Marchi wrote:
> On 2020-10-07 12:36 a.m., Kamil Rytarowski wrote:

>> I prefer to keep all the local static functions in gdbserver citizens of

>> the NetBSD gdbserver.

> 

> I don't understand what this means.

> 


I mean I prefer having functions as members of the class.

>> Long term, shared functionality with the native

>> target will go to gdb/nat and perhaps some functions like

>> elf_64_file_p() could be shared by ELF targets.

> 

> If the goal is sharing them in gdb/nat, it seems to me like it's easier

> to keep it as a free function, that will be easier to share.

> 



OK. I will abandon the idea of adding the freestanding functions to the
target class, unless they interact the class members.

> Simon

>

Patch

diff --git a/gdbserver/ChangeLog b/gdbserver/ChangeLog
index d667a203e67..94fb3409f23 100644
--- a/gdbserver/ChangeLog
+++ b/gdbserver/ChangeLog
@@ -1,3 +1,8 @@ 
+2020-10-01  Kamil Rytarowski  <n54@gmx.com>
+
+	* netbsd-low.cc (netbsd_ptrace_fun): Turn into...
+	(netbsd_process_target::create_inferior): ...lamda here.
+
 2020-10-01  Kamil Rytarowski  <n54@gmx.com>

 	* netbsd-i386-low.cc: Add.
diff --git a/gdbserver/netbsd-low.cc b/gdbserver/netbsd-low.cc
index 7bec55a56ac..8a7ca5294f0 100644
--- a/gdbserver/netbsd-low.cc
+++ b/gdbserver/netbsd-low.cc
@@ -56,38 +56,6 @@  netbsd_add_process (int pid, int attached)
   proc->priv = nullptr;
 }

-/* Callback used by fork_inferior to start tracing the inferior.  */
-
-static void
-netbsd_ptrace_fun ()
-{
-  /* Switch child to its own process group so that signals won't
-     directly affect GDBserver. */
-  if (setpgid (0, 0) < 0)
-    trace_start_error_with_name (("setpgid"));
-
-  if (ptrace (PT_TRACE_ME, 0, nullptr, 0) < 0)
-    trace_start_error_with_name (("ptrace"));
-
-  /* If GDBserver is connected to gdb via stdio, redirect the inferior's
-     stdout to stderr so that inferior i/o doesn't corrupt the connection.
-     Also, redirect stdin to /dev/null.  */
-  if (remote_connection_is_stdio ())
-    {
-      if (close (0) < 0)
-	trace_start_error_with_name (("close"));
-      if (open ("/dev/null", O_RDONLY) < 0)
-	trace_start_error_with_name (("open"));
-      if (dup2 (2, 1) < 0)
-	trace_start_error_with_name (("dup2"));
-      if (write (2, "stdin/stdout redirected\n",
-		 sizeof ("stdin/stdout redirected\n") - 1) < 0)
-	{
-	  /* Errors ignored.  */
-	}
-    }
-}
-
 /* Implement the create_inferior method of the target_ops vector.  */

 int
@@ -96,8 +64,39 @@  netbsd_process_target::create_inferior (const char *program,
 {
   std::string str_program_args = construct_inferior_arguments (program_args);

+  /* Callback used by fork_inferior to start tracing the inferior.  */
+  auto fn
+    = [] ()
+      {
+	/* Switch child to its own process group so that signals won't
+	   directly affect GDBserver. */
+	if (setpgid (0, 0) < 0)
+	  trace_start_error_with_name (("setpgid"));
+
+	if (ptrace (PT_TRACE_ME, 0, nullptr, 0) < 0)
+	  trace_start_error_with_name (("ptrace"));
+
+	/* If GDBserver is connected to gdb via stdio, redirect the inferior's
+	   stdout to stderr so that inferior i/o doesn't corrupt the connection.
+	   Also, redirect stdin to /dev/null.  */
+	if (remote_connection_is_stdio ())
+	  {
+	    if (close (0) < 0)
+	      trace_start_error_with_name (("close"));
+	    if (open ("/dev/null", O_RDONLY) < 0)
+	      trace_start_error_with_name (("open"));
+	    if (dup2 (2, 1) < 0)
+	      trace_start_error_with_name (("dup2"));
+	    if (write (2, "stdin/stdout redirected\n",
+		       sizeof ("stdin/stdout redirected\n") - 1) < 0)
+	      {
+		/* Errors ignored.  */
+	      }
+	  }
+      };
+
   pid_t pid = fork_inferior (program, str_program_args.c_str (),
-			     get_environ ()->envp (), netbsd_ptrace_fun,
+			     get_environ ()->envp (), fn,
 			     nullptr, nullptr, nullptr, nullptr);

   netbsd_add_process (pid, 0);