Re: [PATCHv3] gdb: make thread_info executing and resumed state more consistent

[Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> On 2022-01-13 1:34 p.m., Andrew Burgess wrote:
>> From: Andrew Burgess <andrew.burgess@embecosm.com>
>>
>> I finally got back around to working on this patch, and have a new
>> version that I'd like to propose.
>>
>> The biggest change in this version is that all of the thread creation
>> functions now have a new parameter which controls if the thread is
>> started resumed or not.
>>
>> There was also a great discussion last time about whether the
>> executing and resumed flags (within thread_info) should be combined
>> into a single enum.  Simon argued in favour of this change, while I
>> remain unconvinced.
>>
>> For now, I'd like to propose that the enum change be deferred.  The
>> patch as it stands is already pretty large, and changing how we manage
>> the two flags would only make the patch larger.  The change as I have
>> it right now maintains the flags as they are, but makes their use
>> consistent.  If we later want to change the flags to an enum then it
>> feels like this would be better done as a separate step.
>>
>> There are still things that I would like to improve in the area of
>> this code, I'm still not completely happy with how the thread state is
>> managed around the target_ops::create_inferior call, but I though the
>> code as it stands isn't great, at least things are consistent after
>> this patch.
>>
>> I'm as sure as I feel I can be that I've not broken anything for
>> Linux, but it's almost certain that something will be broken for other
>> targets.  I've details the additional testing I've done at the end of
>> my commit message.
>>
>> My commit message also includes a ChangeLog like log, in this I've
>> tried to mention those areas of the change that I know are untested,
>> or lightly tested.
>>
>> I welcome all review feedback, as well as any additional testing that
>> people might like to do.
>>
>> Thanks,
>> Andrew
>>
>> ---
>>
>> This commit was inspired by this comment from Simon:
>>
>>   https://sourceware.org/pipermail/gdb-patches/2021-July/180694.html
>>
>> The comment is about two thread_info flags m_executing and m_resumed.
>> The m_resumed flag indicates that at the infrun level GDB has set the
>> thread running, while the m_executing flag indicates that the thread
>> is actually running at the target level.
>>
>> It is very common that a thread can be m_resumed, but not m_executing,
>> that is, core GDB thinks the thread is, or should be, running, but at
>> the target level the thread isn't currently running.
>>
>> The comment Simon made was in reply to a patch I posted where I found
>> a case where a thread was marked m_executing, but not m_resumed, that
>> is, at the infrun level GDB thought the thread was stopped, but at the
>> target level the thread was actually running.  Simon suggests that
>> this feels like an invalid state, and after thinking about it, I
>> agree.
>>
>> So, the goal of this commit is to add some asserts to GDB.  The core
>> idea here is that the resumed and executing flags should only change
>> in the following pattern, to begin with everything is set false:
>>
>>   Resumed: false
>>   Executing: false
>>
>> Then infrun marks the thread resumed:
>>
>>   Resumed: true
>>   Executing: false
>>
>> Then a target starts the thread executing:
>>
>>   Resumed: true
>>   Executing: true
>>
>> The thread stops, the target spots this and marks the thread no longer
>> executing:
>>
>>   Resumed: true
>>   Executing: false
>>
>> And finally, infrun acknowledges that the thread is now no longer
>> resumed:
>>
>>   Resumed: false
>>   Executing: false
>>
>> Notice that at no point is resumed false, while executing is true.
>>
>> And so we can add these asserts:
>>
>>  1. The resumed flag should only change state when the executing flag
>>  is false, and
>>
>>  2. The executing flag should only change state when the resumed flag
>>  is true.
>>
>> I added these asserts and ....
>>
>> .... it turns out these rules are broken all over the place in GDB, we
>> have problems like:
>>
>>  (a) When new threads appear they are marked executing, but not
>>  resumed, and
>>
>>  (b) In some places we mark a single thread as resumed, but then
>>  actually set multiple threads executing.
>>
>> For (a) it could be argued that this is a legitimate state - this is
>> actually the problem I addressed in the original patch that Simon was
>> replying too, however, we don't need to support this as a separate
>> state, so if we can make this case follow the expected set of state
>> transitions then it allows us to reduce the number of states that GDB
>> can be in, which I think is a good thing.
>>
>> Case (b) seems to just be a bug to me.
>>
>> My initial approach was to retain the idea that threads are always
>> created in the non-executing, non-resumed state, and then find all the
>> places where new threads are created (which should be in the resumed
>> state), and mark the thread resumed in that case.
>>
>> However, after looking at the changes for a while, it felt like it
>> would be simpler to create threads as resumed by default and instead
>> change the threads back to non-resumed in the few cases where this was
>> appropriate.  The appropriate case being (as far as I can tell), just
>> the initial threads created as part of starting up a new inferior.
>>
>> So, that's what this patch does.  The thread creation routines now
>> take a flag to indicate if the new thread should be created resumed or
>> not.  Almost all threads are started in the resumed state, except for
>> a few cases associated with initial target creation.
>>
>> In an ideal world, I would have liked that all threads created as part
>> of the ::create_inferior process would also be created in the
>> non-resumed state, but that doesn't seem easily achievable right now.
>> Though I could easily see changing the Linux target, other targets that
>> I can't test will be harder to get right.  So, for now, after calling
>> ::create_process I assert that the threads are currently resumed, and
>> then mark the threads as non-resumed.
>>
>> I do have a plan that I might be able to make further improvements in
>> the ::create_inferior area, however, I want to see whether this patch
>> is accepted first before investing further time on this.  I think that
>> where this patch gets to is good enough for now.
>>
>> On Testing:
>>
>> I've tested this primarily on x86-64 GNU/Linux with the unix,
>> native-gdbserver, and native-extended-gdbserver boards, and see no
>> regressions.
>>
>> I have also tried to test some other targets, though the testing has
>> been pretty minimal:
>>
>> I installed FreeBSD in an x86-64 VM.  I can build GDB fine, but when I
>> try to run dejagnu the VM falls over with an out of swap space error,
>> no matter how much swap I give the machine.  However, before falling
>> over a few hundred tests do run, and I see no regressions in those
>> tests.  I've also manually checked that the gdb.base/attach.exp test
>> works (to do some basic attach testing).  So I'm reasonably confident
>> that this target should be mostly OK.
>>
>> As I was working on GNU/Hurd for another bug I had an i386 GNU/Hurd VM
>> available, so I built GDB in here too.  Due to other issues with this
>> target the dejagnu testing is pretty useless here, but I checked that
>> I can start multi-threaded targets, step/continue past thread
>> creation, and also that I can attach to a multi-threaded target.
>>
>> I have also built this branch on Windows, using mingw.  I was unable
>> to get dejagnu testing working on this target, but again I did some
>> basic testing that GDB could start up with multi-threaded inferiors,
>> and correctly see new threads as they appear.
>>
>> I also built a target with a simulator, and checked that I could
>> startup and run basic tests their too.
>>
>> I did try making use of the GCC machine farm to test on AIX and
>> Solaris, but I was unable to get GDB building on any of the machines I
>> tried in the farm.
>
> Hi Andrew,
>
> I see nothing in the patch that stands out as obviously wrong.  But like
> you, I have a hard time convincing myself what is the right resumed
> state to pass to add_thread at each place.  And I'm not sure that just
> staring at the code longer will help much.  It's hard to know without
> actually testing.
>
> For example, I am not sure that passing "true" in all update_thread_list
> methods is correct.  update_thread_list is sometimes called when
> everything is stopped, on all-stop targets.  So in that context, I
> suppose that new threads discovered this way should be created
> non-resumed.
>
> However, I think you did a pretty good job of testing what you could,
> given the context.

Simon,

Thanks for the feedback, and sorry for not following up sooner.  This
got pushed down my list of priorities.

Given how close we are to the GDB 12 branch point, my current plan is to
wait until shortly after we branch, and then update, retest, and push
this patch.

That will give us (me) as much time as possible to help resolve any
problems that this introduces.  I'm expecting there will be some issues
on the targets I've not tested as much, but hopefully they should be
easy enough to resolve.

Obviously the above plan can change if people object.

Thanks,
Andrew