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

[Pedro Alves <pedro@palves.net>]

On 2023-03-21 16:06, Andrew Burgess wrote:
> Pedro Alves <pedro@palves.net> writes:
> 
>> On 2023-02-04 3:38 p.m., Andrew Burgess wrote:

>> 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 held stopped
>> 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.
> 
> What's the motivation for splitting the implementation from the
> associated tests?

It's just that while developing this, I kept reordering patches, while
fixing different aspects, different modes (displaced step, in-line step), different
targets (native, remote).  It was so much easier to keep the testcase in a
separate patch, and fix it / adjust it as I went along.  Keeping the testcase separate
also allowed for keeping you listed as the main git commit author for that patch.
I've now merged the testcase into this patch, and added a note giving you credit
(along with git tag, of course).  The test fails for remote at this point, so I added
a "require" call to skip it on anything other than native at this point.

> 
> Though your implementation may be different (better) than mine, the
> original test I wrote[1] still passes just fine with this commit.
> 
> When digging back through old patches it's much nicer to easier to find
> the GDB changes and the tests in a single commit IMHO.  Do feel free to
> take the tests from that commit, if that helps at all (though I notice I
> left some debug logging in which would need cleaning up, and the test
> fails when using a remote target, but that's easily fixed by skipping
> the test in that case).
> 
> [1] https://sourceware.org/pipermail/gdb-patches/2021-June/180520.html
> 

I guess you noticed later while reading the rest of the series, but I did
already take the tests from that commit, and preserved them as patch #13
in this series:

 https://inbox.sourceware.org/gdb-patches/8735331856.fsf@redhat.com/

I had kept the "New in vX" notes in there so you could more easily tell what
had changed from yours.  (I've now dropped them in the new version.)

> I've looked through the code and it all looks good to me.
> 
> Reviewed-By: Andrew Burgess <aburgess@redhat.com>
> 

Thanks!