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

Aktemur, Tankut Baris
2020-09-04T07:59:03Z

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