Re: [PATCH v2 04/29] Step over clone syscall w/ breakpoint, TARGET_WAITKIND_THREAD_CLONED

[Simon Marchi <simark@simark.ca>]

Just nits, from what I can tell the patch looks OK.

On 2022-07-13 18:24, Pedro Alves wrote:
> (A good chunk of the problem statement in the commit log below is
> Andrew's, adjusted for a different solution, and for covering
> displaced stepping too.)
> 
> This commit addresses bugs gdb/19675 and gdb/27830, which are about
> stepping over a breakpoint set at a clone syscall instruction, one is
> about displaced stepping, and the other about in-line stepping.
> 
> Currently, when a new thread is created through a clone syscall, GDB
> sets the new thread running.  With 'continue' this makes sense
> (assuming no schedlock):
> 
>  - all-stop mode, user issues 'continue', all threads are set running,
>    a newly created thread should also be set running.
> 
>  - non-stop mode, user issues 'continue', other pre-existing threads
>    are not effected, but as the new thread is (sort-of) a child of the

effected -> affected ?

>    thread the user asked to run, it makes sense that the new threads
>    should be created in the running state.
> 
> Similarly, if we are stopped at the clone syscall, and there's no
> software breakpoint at this address, then the current behaviour is
> fine:
> 
>  - all-stop mode, user issues 'stepi', stepping will be done in place
>    (as there's no breakpoint to step over).  While stepping the thread
>    of interest all the other threads will be allowed to continue.  A
>    newly created thread will be set running, and then stopped once the
>    thread of interest has completed its step.
> 
>  - non-stop mode, user issues 'stepi', stepping will be done in place
>    (as there's no breakpoint to step over).  Other threads might be
>    running or stopped, but as with the continue case above, the new
>    thread will be created running.  The only possible issue here is
>    that the new thread will be left running after the initial thread
>    has completed its stepi.  The user would need to manually select
>    the thread and interrupt it, this might not be what the user
>    expects.  However, this is not something this commit tries to
>    change.
> 
> The problem then is what happens when we try to step over a clone
> syscall if there is a breakpoint at the syscall address.
> 
> - For both all-stop and non-stop modes, with in-line stepping:
> 
>    + user issues 'stepi',
>    + [non-stop mode only] GDB stops all threads.  In all-stop mode all
>      threads are already stopped.
>    + GDB removes s/w breakpoint at syscall address,
>    + GDB single steps just the thread of interest, all other threads
>      are left stopped,
>    + New thread is created running,
>    + Initial thread completes its step,
>    + [non-stop mode only] GDB resumes all threads that it previously
>      stopped.
> 
> There are two problems in the in-line stepping scenario above:
> 
>   1. The new thread might pass through the same code that the initial
>      thread is in (i.e. the clone syscall code), in which case it will
>      fail to hit the breakpoint in clone as this was removed so the
>      first thread can single step,
> 
>   2. The new thread might trigger some other stop event before the
>      initial thread reports its step completion.  If this happens we
>      end up triggering an assertion as GDB assumes that only the
>      thread being stepped should stop.  The assert looks like this:
> 
>      infrun.c:5899: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed.
> 
> - For both all-stop and non-stop modes, with displaced stepping:
> 
>    + user issues 'stepi',
>    + GDB starts the displaced step, moves thread's PC to the
>      out-of-line scratch pad, maybe adjusts registers,
>    + GDB single steps the thread of interest, [non-stop mode only] all
>      other threads are left as they were, either running or stopped.
>      In all-stop, all other threads are left stopped.
>    + New thread is created running,
>    + Initial thread completes its step, GDB re-adjusts its PC,
>      restores/releases scratchpad,
>    + [non-stop mode only] GDB resumes the thread, now past its
>      breakpoint.
>    + [all-stop mode only] GDB resumes all threads.
> 
> There is one problem with the displaced stepping scenario above:
> 
>   3. When the parent thread completed its step, GDB adjusted its PC,
>      but did not adjust the child's PC, thus that new child thread
>      will continue execution in the scratch pad, invoking undefined
>      behavior.  If you're lucky, you see a crash.  If unlucky, the
>      inferior gets silently corrupted.
> 
> What is needed is for GDB to have more control over whether the new
> thread is created running or not.  Issue #1 above requires that the
> new thread not be allowed to run until the breakpoint has been
> reinserted.  The only way to guarantee this is if the new thread is
> held in a stopped state until the single step has completed.  Issue #3
> above requires that GDB is informed of when a thread clones itself,
> and of what is the child's ptid, so that GDB can fixup both the parent
> and the child.
> 
> When looking for solutions to this problem I considered how GDB
> handles fork/vfork as these have some of the same issues.  The main
> difference between fork/vfork and clone is that the clone events are
> not reported back to core GDB.  Instead, the clone event is handled
> automatically in the target code and the child thread is immediately
> set running.
> 
> Note we have support for requesting thread creation events out of the
> target (TARGET_WAITKIND_THREAD_CREATED).  However, those are reported
> for the new/child thread.  That would be sufficient to address in-line
> stepping (issue #1), but not for displaced-stepping (issue #3).  To
> handle displaced-stepping, we need an event that is reported to the
> _parent_ of the clone, as the information about the displaced step is
> associated with the clone parent.  TARGET_WAITKIND_THREAD_CREATED
> includes no indication of which thread is the parent that spawned the
> new child.  In fact, for some targets, like e.g., Windows, it would be
> impossible to know which thread that was, as thread creation there
> doesn't work by "cloning".
> 
> The solution implemented here is to model clone on fork/vfork, and
> introduce a new TARGET_WAITKIND_THREAD_CLONED event.  This event is
> similar to TARGET_WAITKIND_FORKED and TARGET_WAITKIND_VFORKED, except
> that we end up with a new thread in the same process, instead of a new
> thread of a new process.  Like FORKED and VFORKED, THREAD_CLONED
> waitstatuses have a child_ptid property, and the child is help stopped

help -> held

> until GDB explicitly resumes it.  This addresses the in-line stepping
> case (issues #1 and #2).
> 
> The infrun code that handles displaced stepping fixup for the child
> after a fork/vfork event is thus reused for THREAD_CLONE, with some
> minimal conditions added, addressing the displaced stepping case
> (issue #3).
> 
> The native Linux backend is adjusted to unconditionally report
> TARGET_WAITKIND_THREAD_CLONED events to the core.
> 
> Following the follow_fork model in core GDB, we introduce a
> target_follow_clone target method, which is responsible for making the
> new clone child visible to the rest of GDB.
> 
> Subsequent patches will add clone events support to the remote
> protocol and gdbserver.
> 
> A testcase will be added by a later patch.
> 
> displaced_step_in_progress_thread is removed in this patch, but it is
> added back again in a subsequent patch.  We need to do this because
> the function is static, and with no callers, the compiler would warn,
> (error with -Werror), breaking the build.

If you prefer, you could annotate temporarily the function with the
"used" attribute.

> @@ -1848,8 +1863,39 @@ displaced_step_finish (thread_info *event_thread,
>  
>    /* Do the fixup, and release the resources acquired to do the displaced
>       step. */
> -  return gdbarch_displaced_step_finish (displaced->get_original_gdbarch (),
> -					event_thread, event_status);
> +  displaced_step_finish_status status
> +    = gdbarch_displaced_step_finish (displaced->get_original_gdbarch (),
> +				     event_thread, event_status);
> +
> +  if (event_status.kind () == TARGET_WAITKIND_FORKED
> +      || event_status.kind () == TARGET_WAITKIND_VFORKED
> +      || event_status.kind () == TARGET_WAITKIND_THREAD_CLONED)
> +    {
> +      /* Since the vfork/fork/clone syscall instruction was executed
> +	 in the scratchpad, the child's PC is also within the
> +	 scratchpad.  Set the child's PC to the parent's PC value,
> +	 which has already been fixed up.  Note: we use the parent's
> +	 aspace here, although we're touching the child, because the
> +	 child hasn't been added to the inferior list yet at this
> +	 point.  */
> +
> +      struct regcache *child_regcache
> +	= get_thread_arch_aspace_regcache (parent_inf->process_target (),
> +					   event_status.child_ptid (),
> +					   gdbarch,
> +					   parent_inf->aspace);
> +      /* Read PC value of parent.  */
> +      CORE_ADDR parent_pc = regcache_read_pc (regcache);
> +
> +      displaced_debug_printf ("write child pc from %s to %s",
> +			      paddress (gdbarch,
> +					regcache_read_pc (child_regcache)),
> +			      paddress (gdbarch, parent_pc));
> +
> +      regcache_write_pc (child_regcache, parent_pc);

Since you talked about Windows in the commit message, that made me
wonder: the code just above wouldn't work for Windows, right?  My
understanding is that Windows threads start with the PC directly at the
function the user provided to CreateThread.

Would the Windows target just not report TARGET_WAITKIND_THREAD_CLONED?

> @@ -5032,7 +5076,7 @@ handle_one (const wait_one_event &event)
>  		global_thread_step_over_chain_enqueue (t);
>  	    }
>  
> -	  regcache = get_thread_regcache (t);
> +	  struct regcache *regcache = get_thread_regcache (t);

This change is kind of unrelated to this patch.

> +/* If LP has a pending fork/vfork/clone status, store it in WS and
> +   return true.  Otherwise, return false.  */
>  
> +static bool
> +get_pending_child_status (lwp_info *lp, target_waitstatus *ws)

It would maybe be a bit more C++ to return an
optional<target_waitstatus>?  The callers can then look like:

  if (gdb::optional<target_waitstatus> ws = get_pending_child_status (lp))

> @@ -1819,6 +1853,58 @@ linux_handle_syscall_trap (struct lwp_info *lp, int stopping)
>    return 1;
>  }
>  
> +void
> +linux_nat_target::follow_clone (ptid_t child_ptid)
> +{
> +  linux_nat_debug_printf
> +    ("Got clone event from LWP %ld, new child is LWP %ld",
> +     inferior_ptid.lwp (), child_ptid.lwp ());

Even if we are in the linux-nat target and know the LWPs are unique, I
think it's nicer to print full ptids anyway.  When you are debugging a
problem that involves multiples processes, it becomes easier to identify
which LWPs belong to which process.

> +
> +  lwp_info *new_lp = add_lwp (child_ptid);
> +  new_lp->stopped = 1;
> +
> +  /* If the thread_db layer is active, let it record the user
> +     level thread id and status, and add the thread to GDB's
> +     list.  */
> +  if (!thread_db_notice_clone (inferior_ptid, new_lp->ptid))
> +    {
> +      /* The process is not using thread_db.  Add the LWP to
> +	 GDB's list.  */
> +      target_post_attach (new_lp->ptid.lwp ());

Can you explain why that target_post_attach call?  Oh, I just noticed it
was already there.  Still, can you explain?

> +      add_thread (linux_target, new_lp->ptid);
> +    }
> +
> +  /* We just created NEW_LP so it cannot yet contain STATUS.  */
> +  gdb_assert (new_lp->status == 0);
> +
> +  if (!pull_pid_from_list (&stopped_pids, child_ptid.lwp (), &new_lp->status))
> +    internal_error (__FILE__, __LINE__, _("no saved status for clone lwp"));

gdb_assert_not_reached maybe?

Otherwise, IBWN to have a version of gdb_assert that lets us provide a
message.

Simon