RE: [PATCH 10/10] Add minimal and functional NetBSD/amd64 gdbserver

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

On Friday, September 4, 2020 2:13 AM, Kamil Rytarowski wrote:
> On 03.09.2020 19:42, Aktemur, Tankut Baris wrote:
> > On September 2, 2020 7:59 PM, Kamil Rytarowski wrote:
> >> +/* 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.  */;
> >
> > Any particular reason for the explicit ';'?
> >
> 
> Copied from Linux. I've removed the strat semicolon.
> 
> gdbserver/linux-low.cc:   /* Errors ignored.  */;
> 

It seems originally the code did not have braces, so the semicolon was
in fact needed.  Braces were added later by commit 8c29b58e98b4, but the
semicolon was not removed.

-           /* Errors ignored.  */;
+           {
+             /* Errors ignored.  */;
+           }

> >> +  if ((*target).read_memory (phdr_memaddr, phdr_buf.data (), phdr_buf.size ()))
> >
> > Why not simply `target->read_memory`?
> 
> Fixed.
> 
> > Also, it might be better to explicitly check != 0.
> >
> 
> I've avoided it as the line is too long and would need to be wrapped.

I believe the explicit check is required per coding rules despite forcing a wrap
(please note that I'm not a maintainer with approval/waiver authority).

> >> +extern struct netbsd_regset_info netbsd_target_regsets[];
> >> +
> >> +/* The target-specific operations for NetBSD support.  */
> >> +
> >> +struct netbsd_target_ops
> >> +{
> >> +  /* Architecture-specific setup.  */
> >> +  void (*arch_setup) ();
> >> +
> >> +  /* Hook to support target specific qSupported.  */
> >> +  void (*process_qsupported) (char **, int count);
> >
> > Is the `process_qsupported` function called anywhere?
> >
> 
> Hmm... it was originally copied from Linux (before C++ification). It
> looks like unused now, at least I'm not triggering a call for it.
> 
> Should I drop it?
> 

I see.  Before C++ification, the linux target directly forwarded the request to
the low target.  After C++ification, the linux target simply inherits the default
implementation from its parent, process_stratum_target, where it's just an empty-bodied
method.  The low targets are free to override, and that's what 
x86_target::process_qsupported does in linux-x86-low.cc.  I think you can either
override `process_qsupported` in netbsd_process_target to simply call the low
target's `process_qsupported`, or ...

> >> +};
> >
> > No strong opinion, but the low target could be a derived class of
> > netbsd_process_target, like the linux low targets.
> >
> 
> This is a simplified x86_64 support and for the time being I will leave
> it as it is. During addition of i386, I will switch it to more
> linux-like approach.

... make the low target derive from netbsd_process_target and override the 
method there :).

> >> +/* Update all the target description of all processes; a new GDB
> >> +   connected, and it may or not support xml target descriptions.  */
> >> +
> >> +static void
> >> +x86_64_netbsd_update_xmltarget (void)
> >> +{
> >> +  struct thread_info *saved_thread = current_thread;
> >> +
> >> +  /* Before changing the register cache's internal layout, flush the
> >> +     contents of the current valid caches back to the threads, and
> >> +     release the current regcache objects.  */
> >> +  regcache_release ();
> >> +
> >> +  for_each_process ([] (process_info *proc) {
> >> +		      int pid = proc->pid;
> >> +
> >> +		      /* Look up any thread of this process.  */
> >> +		      current_thread = find_any_thread_of_pid (pid);
> >> +
> >> +		      the_low_target.arch_setup ();
> >
> > I find this confusing because the target object is supposed to be a singleton.
> > Why is its arch_setup called for each process?
> >
> 
> This logic was copied from Linux, should I drop it? However.. after
> removal of process_qsupported this is no longer in use and I have
> removed the x86_64_netbsd_update_xmltarget function entirely.

Hmm, the x86 low target updates the tdesc of the current process.
The lambda above changes the current thread before calling arch_setup.

	/* Initialize the target description for the architecture of the
	   inferior.  */

	void
	x86_target::low_arch_setup ()
	{
	  current_process ()->tdesc = x86_linux_read_description ();
	}

So, it seems fine from this perspective.  Sorry for misleading.

A final note: I'm not fully knowledgeable in the semantics of all the target ops
or in NetBSD.  Please consider my comments as a general check against the code style
and smells.

Thanks.
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928