Re: [PATCH] Add basic event handling in the NetBSD target

["Kamil Rytarowski" <n54@gmx.com>]

> Sent: Sunday, April 26, 2020 at 11:14 PM
> From: "Simon Marchi" <simark@simark.ca>
> To: "Kamil Rytarowski" <n54@gmx.com>, gdb-patches@sourceware.org
> Subject: Re: [PATCH] Add basic event handling in the NetBSD target
>
> On 2020-04-24 6:09 a.m., Kamil Rytarowski wrote:
> >> I know that you'll say it's because FreeBSD does it this way, but still it surprised
> >> me.  If the core of GDB asks to resume one specific thread, it doesn't mean you should
> >> suspend the other threads, they should just be left in whatever state they are.  Is there
> >> a reason to stop the other threads here?
> >>
> >
> > In Linux there can be non-stop mode and threads are managed (stopped,
> > started, etc) individually. On NetBSD we can start and stop the whole
> > process, regardless of the number of threads in it.
> >
> > We need to set the state of all threads before PT_CONTINUE (or assume
> > unchanged state of them).
> >
> > If a thread is not expected to be suspended, we shall set its flag to
> > resume. The same for single-step (PT_STEP). This approach simplifies the
> > implementation as I don't need to track internally which thread has what
> > status and merely save a few syscalls.
>
> Ok.
>
> >>> +    }
> >>> +   else
> >>> +    {
> >>> +      /* If ptid is a wildcard, resume all matching threads (they won't run
> >>> +         until the process is continued however).  */
> >>> +      for (thread_info *tp : all_non_exited_threads (this, ptid))
> >>> +        if (ptrace (PT_RESUME, tp->ptid.pid (), NULL, tp->ptid.lwp ()) == -1)
> >>> +          perror_with_name (("ptrace"));
> >>> +      ptid = inferior_ptid;
> >>
> >> Can you explain this?  Ideally the resume method should not rely on inferior_ptid.  We
> >> are working on reducing the dependencies on this global, in favor of passing the context
> >> by parameters.  Here, the `ptid` parameter should be enough.
> >>
> >
> > The caller can pass minus_one_ptid, which makes no sense for NetBSD and
> > certainly in the multi-target support.
> >
> > Scenarios, as I can see them on NetBSD:
> >
> >  - passing ptid_t (pid, 0, 0) -> resume the whole process with all the
> > threads
> >  - passing ptid_t (pid, lwp, 0) -> resume pid::lwp with all the other
> > threads suspended in pid (`set scheduler-lock')
> >  - passing ptid_t (-1, ?, ?) -> confusion to me so I try to find any
> > fallback
> >
> > If we can abandon the -1 case, I can drop the inferior_ptid usage. If
> > the core no longer passes -1 (it could be), we shall cleanup existing
> > code that handles this as it could happen. If a refactoring can happen,
> > this shall be followed by addition of gdb_assert(ptid != minus_one_ptid).
>
> Or a 4th one: resume all threads of all processes that this target manages.
>

OK. I will try to propose this solution.

> I don't know off-hand if the core of GDB today can pass minus_one_ptid.  You could
> add such an assert, run all your tests, and if everything works, conclude it's not
> needed.  If the assert is hit, you'll know why.
>

I've checked that the GDB core tries to resume minus_one_ptid.

#2  0x0000000000693e08 in dump_core () at utils.c:203
#3  0x00000000006984ba in internal_vproblem(internal_problem *, const char *, int, const char *, typedef __va_list_tag __va_list_tag *) (problem=problem@entry=0xd24840 <internal_error_problem>,
    file=<optimized out>, line=553, fmt=<optimized out>, ap=ap@entry=0x7f7fff3266a8) at utils.c:413
#4  0x0000000000698622 in internal_verror (file=<optimized out>, line=<optimized out>, fmt=<optimized out>, ap=ap@entry=0x7f7fff3266a8) at utils.c:438
#5  0x0000000000780497 in internal_error (file=file@entry=0x8cdc9c "nbsd-nat.c", line=line@entry=553, fmt=<optimized out>) at errors.cc:55
#6  0x00000000005b18f4 in nbsd_nat_target::resume (this=0xd1f9a0 <the_amd64_nbsd_nat_target>, ptid=..., step=0, signal=GDB_SIGNAL_0) at nbsd-nat.c:553
#7  0x000000000064ae0d in target_resume (ptid=..., step=step@entry=0, signal=signal@entry=GDB_SIGNAL_0) at target.c:2121
#8  0x0000000000657d29 in target_continue_no_signal (ptid=...) at target.c:3416
#9  0x00000000005aea79 in startup_inferior (proc_target=proc_target@entry=0xd1f9a0 <the_amd64_nbsd_nat_target>, pid=pid@entry=718, ntraps=ntraps@entry=1,
    last_waitstatus=last_waitstatus@entry=0x0, last_ptid=last_ptid@entry=0x0) at nat/fork-inferior.c:569
#10 0x0000000000520023 in gdb_startup_inferior (pid=pid@entry=718, num_traps=num_traps@entry=1) at fork-child.c:134
#11 0x0000000000555403 in inf_ptrace_target::create_inferior (During symbol reading: Duplicate PC 0x559029 for DW_TAG_call_site DIE 0x1999451 [in module /public/binutils-gdb/gdb/gdb]
this=0xd1f9a0 <the_amd64_nbsd_nat_target>, exec_file=<optimized out>, allargs=..., env=0x7626ff79ae00, from_tty=<optimized out>)
    at inf-ptrace.c:119
#12 0x000000000055b112 in run_command_1 (args=<optimized out>, from_tty=1, run_how=RUN_NORMAL) at infcmd.c:640
#13 0x0000000000481c96 in cmd_func (cmd=<optimized out>, args=<optimized out>, from_tty=<optimized out>) at cli/cli-decode.c:2004
#14 0x00000000006617b2 in execute_command (p=<optimized out>, p@entry=0x7626ffb1c020 "", from_tty=1) at top.c:655
#15 0x0000000000512d5d in command_handler (command=0x7626ffb1c020 "") at event-top.c:588
#16 0x000000000051303a in command_line_handler (rl=...) at event-top.c:773
#17 0x0000000000513576 in gdb_rl_callback_handler (rl=0x7626ffb1c1e0 "r") at event-top.c:219
#18 0x00000000006cae86 in rl_callback_read_char () at callback.c:281
#19 0x0000000000512317 in gdb_rl_callback_read_char_wrapper_noexcept () at event-top.c:177
#20 0x00000000005133a7 in gdb_rl_callback_read_char_wrapper (client_data=<optimized out>) at event-top.c:194
#21 0x00000000005121d0 in stdin_event_handler (error=<optimized out>, client_data=0x7626ffafe0c0) at event-top.c:516
#22 0x0000000000780b4b in gdb_wait_for_event (block=block@entry=1) at event-loop.cc:673
#23 0x0000000000780d3d in gdb_do_one_event () at event-loop.cc:215
#24 0x0000000000589ed0 in start_event_loop () at main.c:356
#25 captured_command_loop () at main.c:416
#26 0x000000000058bc5f in captured_main (data=0x7f7fff326eb0) at main.c:1254
#27 gdb_main (args=args@entry=0x7f7fff326ed0) at main.c:1269
#28 0x00000000007bcf1d in main (argc=<optimized out>, argv=<optimized out>) at gdb.c:32

> >>> +      /* The common code passes WNOHANG that leads to crashes, overwrite it.  */
> >>> +      pid = waitpid (ptid.pid (), &status, 0);
> >>
> >> What is crashing exactly?
> >>
> >
> > The core code asks to waitpid() with WNOHANG, receives pid=0 (nobody)
> > and crashes.
> >
> > It's a generic bug that shall be fixed... but it looks like nobody
> > really obeys the core and the 3rd argument is overwritten (compare:
> > rs6000-nat.c, obsd-nat.c, inf-ptrace.c or Linux...).
> >
> > I've landed into more similar generic nuances during my effort on GDB,
> > that handling events promptly can crash the core and I need to change
> > the behavior to emulate other kernels.
> >
> > It would be better to fix the core, but it is not NetBSD's fault and not
> > a blocker for NetBSD support.
> >
> > If there is interest in fixing it, I can file a bug report
> > independently. I don't pledge to work on it myself (at least as long as
> > I can merely focus on one kernel support).
>
> Ok, I'm fine with doing this and documenting it somewhere.
>
> Simon
>
>