Re: GDB breakpoints are broken in new threads -- here's why

[Samuel Thibault]

Sergey Bugaev, le mar. 11 avril 2023 11:48:45 +0300, a ecrit:
> On Tue, Apr 11, 2023 at 1:50 AM Samuel Thibault <samuel.thibault@gnu.org> 
> wrote:
> > Sergey Bugaev, le dim. 02 avril 2023 15:22:33 +0300, a ecrit:
> > > I propose the following: before resetting the exception port, glibc
> > > would fetch the previous one, and if it's non-null, it will perform a
> > > special synchronous RPC on it, both passing the new exception port
> > > that it would set to the tracer (so there's no need to actually set
> > > it), and telling the tracer what its signal thread is (currently GDB
> > > just tries to guess that the second thread is the one, except this
> > > again doesn't work for the very same reason, there's not yet a second
> > > thread when the task is at its very _start).
> > >
> > > routine name_to_be_bikeshedded (
> > >         tracer_exc_port: mach_port_move_send_t;
> > >         my_exc_port: mach_port_make_send_t;
> > >         signal_thread: thread_t);
> >
> > Yes, that seems much more cooperative.
> >
> > I would say call it exception_set_exception_port?

Ah, actually I have naming it after the task_set_exception_port name,
that is actually a macro for a task_set_special_port call.

I was using the exception_ prefix to express that it'd supposed to be
used on an exception port.

> Maybe. I was thinking more like tracee_check_in,

That would assume that it's about tracing, that could not be the case,
we don't know what it could be useful in the future. So better say what
it's doing.

> since it sends more than just the exception port (the signal thread
> port,

One doesn't need to think about it as a signal thread, it's just the
thread which exceptions are sent to.

> and potentially we may want to pack something else in here?).

If there are more things to make signals work that are not related to
exception, I'd say to introduce other calls.

> There's also a question of which subsystem we should put it in. I was
> tentatively thinking into msg.defs, even though it's not a real
> msgport RPC. But maybe it deserves its own subsystem?

I was rather thinking to make it part of the exc subsystem, since that's
what is sent to exception ports.

> > > Any ideas how to fix the first issue (thread and subcode info getting
> > > lost when forwarding signals to the tracer)?
> >
> > I'd say just add an extended proc_wait RPC indeed. proc can easily
> > implement both, and new callers can revert to the old version.
> 
> Yes, so:
> 
> routine proc_wait_ext (
>     process: process_t;
>     sreplyport reply_port: sreply_port_t;
>     waitee: task_t; /* XXX */
>     options: int;
>     out status: int; /* encodes signo */
>     out thread: thread_t;
>     out sigdetail: hurd_signal_detail);
> 
> This needs to accept a task port (not just a PID or even negative PID
> or some such), since it returns the thread port, so we need to make
> sure the caller already has full access to the task (which GDB of
> course does). So this won't fully replace proc_wait / proc_waitid.

So it would be only useful for the trace case. That should be fine
indeed.

> We also need these, which do supersede the existing ones:
> 
> routine proc_mark_stop2 (
>     process: process_t;
>     thread: thread_t; /* null if this was sent to the process as a whole */
>     signo: int;
>     sigdetail: hurd_signal_detail);
> 
> And a new routine to post/forward the signal to the task while
> preserving thread/detail info:
> 
> routine msg_sig_post2 (
>     process: mach_port_t;
>     sreplyport reply_port: sreply_port_t;
>     thread: thread_t; /* null to deliver to any thread */
>     signo: int;
>     sigdetail: hurd_signal_detail;
>     untraced: bool;
>     refport: mach_port_t);

Indeed.

> And ideally, I'd take the chance to add a better public API for
> posting signals in glibc, i.e. introduce _hurd_kill, or own analog of
> pidfd_send_signal:
> 
> /* Send signal SIGNO to a process or process group.  If PROC is non-null,
>    the signal is reliably sent to that process, even in the face of PID
>    reuse, and even if the current process otherwise is not allowed to gain
>    full access to the target task.  In this case PID can still be provided
>    as a hint, to save on the number of RPCs.  If PROC is null, PID can
>    designate either a single process or a process group, like in kill(2).
>    If SIGDETAIL is passed, it will be sent to the target as-is, otherwise
>    one will be synthesized.  If REFPORT is non-null, it will be used to
>    authenticate ourselves to the target; otherwise this function will try
>    to find a suitable refport itself.  */
> error_t
> _hurd_kill (process_t proc, pid_t pid, int signo,
>             const hurd_signal_detail *__restrict sigdetail,
>             mach_port_t refport);

That could be interesting indeed.

> Super ideally, we'd also find a way to implement si_pid/si_uid while
> doing this all, but it's complicated. But in the simple & common case
> when the signal sender has full access to the task (i.e. refport is
> task or auth port), we can just trust whatever the caller says. So
> maybe we just need to add pid/uid to hurd_signal_detail.

That can be useful to add pid/uid alongside indeed.

Samuel