Re: [PATCH 2/2] gdb/remote: return early from remote_check_symbols if waiting for stop reply

[Andrew Burgess <aburgess@redhat.com>]

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

> On 2022-04-26 06:27, Andrew Burgess wrote:
>> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>> 
>>> Commit 152a17495663 ("gdb: prune inferiors at end of
>>> fetch_inferior_event, fix intermittent failure of
>>> gdb.threads/fork-plus-threads.exp") broke
>>> gdb.base/vfork-follow-parent.exp with the native-gdbserver board:
>>>
>>>     (gdb) PASS: gdb.base/vfork-follow-parent.exp: resolution_method=schedule-multiple: inferior 1
>>>     print unblock_parent = 1^M
>>>     $1 = 1^M
>>>     (gdb) PASS: gdb.base/vfork-follow-parent.exp: resolution_method=schedule-multiple: print unblock_parent = 1
>>>     continue^M
>>>     Continuing.^M
>>>     terminate called after throwing an instance of 'gdb_exception_error'^M
>>>
>>> I can manually reproduce the issue by running (with gdbserver running on
>>> port 1234) with the following command (just the commands that the test
>>> does as a one liner):
>>>
>>>     $ ./gdb -q --data-directory=data-directory \
>>>           testsuite/outputs/gdb.base/step-over-syscall/step-over-vfork \
>> 
>> It was a little confusing that the example you give above is for one
>> test, but this one liner is for another.  Given that both tests fail, it
>> might be nicer to mention that both tests fail, but quote from the test
>> you are about to use as an example...
>
> Hmm right, that wasn't intended.  Fixed.
>
>>> 	  -ex "tar rem :1234" \
>> 
>> You can simplify this to just:
>> 
>>   -ex "target remote | gdbserver - path/to/executable"
>
> Fixed.
>
>> 
>> then it really is a one liners.
>> 
>> 
>>> 	  -ex "b main" \
>>> 	  -ex c \
>>> 	  -ex "d 1" \
>> 
>> And this can be just:
>> 
>>   -ex "start"
>> 
>> right?
>
> No, since we don't run, we continue.  It could be tbreak instead of
> break + delete though.
>
>> 
>> 
>>> 	  -ex "set displaced-stepping off" \
>>> 	  -ex "b *0x7ffff7d7ac5a if main == 0"
>>> 	  -ex "set detach-on-fork off"
>>> 	  -ex "set follow-fork-mode child"
>>> 	  -ex c
>>> 	  -ex "inferior 1"
>>> 	  -ex "b marker"
>>> 	  -ex c
>> 
>> You're missing the backslashes from the last 7 lines here.
>
> Oops, right.  I did the formatting in the email so it wouldn't all be
> one a single line, and forgot some.
>
>>> I realized there are simpler ways to trigger this problem.  You only
>>> need to load a symbol file (which triggers remote_check_symbols) while
>>> sync execution is ongoing:
>>>
>>>     $ ./gdb -q --data-directory=data-directory -nx a.out \
>>>           -ex "tar ext :1234" \
>>> 	  -ex "tb main" \
>>> 	  -ex c \
>> 
>> Use 'start' ?
>
> No, since we continue and don't run.
>
>> 
>>> 	  -ex "c&" \
>>> 	  -ex "add-inferior" \
>>> 	  -ex "inferior 2" \
>>> 	  -ex "file /bin/ls"
>>>     * aborts because of the same exception *
>>>
>>> My initial fix for it was to return early from new_objfile if objfile is
>>> nullptr, since that means that we removed some or all symbols.  I don't
>>> think that's a good idea, because it might be important to send qSymbol
>>> after removing symbols.  This could help a remote target realize that
>>> the symbols it used previously are now gone.  I don't think that
>>> GDBserver uses that, but I think it could be used like that in theory.
>>> It also wouldn't help with that last example, since objfile wouldn't be
>>> nullptr.
>>>
>>> The proposed fix is instead to return early from remote_check_symbols if
>>> rs->waiting_for_stop_reply is set.
>> 
>> What I didn't understand when reading this patch is that in the previous
>> paragraph you seem to argue that sending qSymbol, though not currently
>> "required" is a good thing, and we should preserve that behaviour.
>> 
>> Then in the second paragraph you suggest that the fix is to just not
>> send qSymbol when the target can't respond.  But there's no proposal to
>> send qSymbol later... so isn't your solution at odds with what you just
>> said?
>
> In this case, the target is resumed, so we for sure can't send it.  But
> if the target is stopped and the user uses the "file" command to discard
> symbols, remote_check_symbols is called with nullptr.  Today, we send a
> qSymbol in this case, but with the suggestion above implemented, we
> wouldn't.  And that feels like a regression.
>
> You're right though that with the fix I propose in this patch, we never
> end up sending a qSymbol.  But given that the current behavior is that
> we crash... that doesn't feel like a regression.

Sorry, I guess I misunderstood, I thought that the crash had been
introduced with commit 152a17495663 ("gdb: prune inferiors at end of
fetch_inferior_event, fix intermittent failure of
gdb.threads/fork-plus-threads.exp") - at least that was the impression I
got from the first paragraph of your commit message.

I confess I didn't go back and check the behaviour before that commit
was merged, but I'd kind of assumed we didn't crash then, but instead
ended up sending qSymbol later.  That was where my confusion was coming
from, comparing pre-152a17495663 behaviour to the behaviour after this
commit.


>                                                   Sending a deferred
> qSymbol the next time we stop would be an option, if someone ever needs
> it.  But at the moment, this is just an imaginary use case.  I'm not
> sure what using the "file" command by itself on a resumed target really
> means, given it doesn't really change what's loaded in memory.

I agree that we could/should wait until an actual use case presents
itself before we worry too much about some of these details.

>
> On a side-note, it seems like we don't send a qSymbol when a library is
> unloaded (i.e. dlclose).  It seems to me like it would be useful to do
> so.  If some module on the remote target gets enabled when a particular
> library is loaded by the inferior, it would make sense that it gets
> disabled when that library is unloaded.
>
> On a second side-note, we don't send qSymbol when doing a "run".  In
> practice, most processes load some library early in their startup, so we
> do end up sending a qSymbol then.  If the remote side needs any symbol
> defined in the main objfile, they'll pick it up then.  But that could be
> a problem for static executables that don't load any library.  I gave
> it a try, and found that GDB still sends a qSymbol in reaction to adding
> the dummy vsyscall objfile in add_vsyscall_page.  So if it wasn't for
> that, we would never send a qSymbol on run.  That's kind of a hidden bug
> right now.
>
>> I'm not suggesting that we should necessarily rush to send qSymbol at
>> some deferred time, I guess I'm just not understanding the two previous
>> paragraphs, and why the solution is OK, given what you just said.
>
> The gist of it is: the fix prevents trying to send qSymbol when we
> really can't, but we keep sending it when we can.
>
>>>                                      If I understand correctly, it's not
>>> really necessary to use the more complete check that putpkt_binary uses:
>>>
>>>   if (!target_is_non_stop_p ()
>>>       && target_is_async_p ()
>>>       && rs->waiting_for_stop_reply)
>>>
>>> ... because if rs->waiting_for_stop_reply is set, it means we're using
>>> the all-stop / sync mode.
>>>
>>> With this patch, I see the testsuite going back to the same state as
>>> before 152a17495663.
>>>
>>> Note that when I try my "load symbol file while background execution"
>>> example with this patch applied, GDB still crashes with the same
>>> uncaught exception, but it's another method that causes it
>>> (remote_hostio_send_command), meaning we are getting further in the
>>> execution.  But it would be out of the scope of this patch to fix
>>> that.
>> 
>> Should there be a bugzilla entry created for the remaining issue?
>
> I can file one when this patch is merged (it wouldn't make sense to file
> a bugzilla entry for a behavior that isn't in master).
>
>>> Change-Id: Ica643145bcc03115248290fd310cadab8ec8371c
>>> ---
>>>  gdb/remote.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/gdb/remote.c b/gdb/remote.c
>>> index 562cc586f2bf..a104f4a57a80 100644
>>> --- a/gdb/remote.c
>>> +++ b/gdb/remote.c
>>> @@ -5124,6 +5124,10 @@ remote_target::remote_check_symbols ()
>>>    if (!target_has_execution ())
>>>      return;
>>>  
>>> +  remote_state *rs = get_remote_state ();
>>> +  if (rs->waiting_for_stop_reply)
>>> +    return;
>>> +
>>>    if (packet_support (PACKET_qSymbol) == PACKET_DISABLE)
>>>      return;
>>>  
>> 
>> I'm OK with this change.  My assumption would be that symbol values held
>> on the remote end will be associated with a specific inferior (or maybe
>> with an objfile in use by one or more inferiors).  As the remote knows
>> when the inferior has exited I assume the remote knows to throw away
>> symbols that are no longer valid/used.  As a result I don't really see
>> any urgency to send qSymbol unless some other use case comes up.
>
> The remote side has no notion of inferiors, only of actually running
> processes.

You're right to be exact with the terminology, but I was using inferior
for process.  And though the "inferior" class might only exist in GDB,
the inferior terminology has definitely leaked over into gdbserver - the
code base is littered with references to inferiors.

>             Speaking of GDBserver (I can't tell how other remote
> implementations use qSymbol), the thread_db symbols are per-process.  So
> when a process exits, it all gets destroyed, and if a new process
> appears (which could be the same inferior in GDB), GDBserver will ask
> for new symbols when it gets the chance.

This is how I'd expect things to operate.  I mean, if a server
implementation was smart it could share symbol information between
processes that used the same object files, loaded at the same address,
but that's implementation detail.  Really, symbols are per-process.

> GDBserver also uses qSymbol for libinproctrace symbols, for fast
> tracepoints.  Those are stored in globals, so I can only think that it's
> broken when using multiple processes.

I agree with this assesment.

Thanks,
Andrew