Re: [RFC][PATCH v3] Consolidate gdbserver global variables

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Tuesday, May 08 2018, Stan Cox wrote:

> (Addressed formatting and ChangeLog issues)
>
>> As mentioned, I'm skeptical of all client_state references
>> in the backends.
>>
>> How will this work with multiple clients?   What if one client
>> wants exec events, and the other one doesn't?  This seems to
>> suggest that the backends need to enable exec events if _any_
>> client wants them (or unconditionally), and then filter out exec
>> events at a higher level, before reporting the event to
>> each client?
>
> Perhaps filtering that is roughly analogous to the way
> gdb_catch_this_syscall_p filters for particular syscalls.
>
>> This "= 0" is a spurious change.  Remove it.
>
> Removed (it eliminated a compiler warning with gcc 7.3.1)
>
>> Give the global different name to avoid conflict with
>> the type name.  Maybe "g_client_state" or "the_client_state".
>
> Used g_client_state and also made it static instead of allocating it
> with new.
>
>> What's the plan for last_status/last_ptid?
>> Shouldn't those be per-client too?
>
> Added those.
>
>> Should these two be per client?  I'd think so off hand,
>> since they correspond to the Hc/Hg threads?
>
> Added cont_thread and general_thread
>
>> I suspect that it'd be better to make the server_waiting
>> global be per-process instead
>
> Returned to server_waiting being a global for now.
>
>
> Thanks Pedro!
>
>
> Tested on linux with native-gdbserver.

Hi Stan,

This patch introduced a regression on GDB when testing with
--target_board=native-gdbserver.  The failure is described here:

  https://sourceware.org/bugzilla/show_bug.cgi?id=23378

I've been trying to narrow down the cause, but so far have not been very
successful.  It seems that everything was covered by your patch, so I'm
guessing there must be some hidden spot where we're forgetting to update
some internal state and the failure ends up happening.

For the sake of comparison, here's the output that should have been
printed:

  Expecting: ^(-var-update L[
  ]+)?(\^done,changelist=\[{name="L",in_scope="true",type_changed="false",has_more="0"}\][
  ]+[(]gdb[)] 
  [ ]*)
  -var-update L
  ^done,changelist=[{name="L",in_scope="true",type_changed="false",has_more="0"}]
  (gdb) 
  PASS: gdb.mi/mi-var-cmd.exp: in-and-out-of-scope: in scope now

And this is what the failure looks like:

  Expecting: ^(-var-update L[
  ]+)?(\^done,changelist=\[{name="L",in_scope="true",type_changed="false",has_more="0"}\][
  ]+[(]gdb[)] 
  [ ]*)
  -var-update L
  ^done,changelist=[]
  (gdb) 
  FAIL: gdb.mi/mi-var-cmd.exp: in-and-out-of-scope: in scope now (unexpected output)

Something to take into consideration is that gdbserver is restarted
many times during the testcase.  I think this may have something to do
with it, because the test doesn't fail on native-extended-gdbserver.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/