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

[Pedro Alves <palves at redhat dot com>]

On 05/08/2018 09:58 PM, Stan Cox wrote:
> (Addressed formatting and ChangeLog issues)

Thanks.  This is looking great now.  Some comments below,
but no need for another round of review.

> 
>> 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)

You mean, without the change, we trigger a warning?  What
does it look like?  If we do, then we'll need to address it
somehow, of course, given -Werror.  But I'd like to see
it first.

> Tested on linux with native-gdbserver.
> 
> Add client_state struct.
> 
> Collect per client specific global data items into struct client_state,
> which is similar in purpose to remote.c::remote_state.
> @@ -1835,6 +1820,7 @@ handle_qxfer_btrace (const char *annex,
>               gdb_byte *readbuf, const gdb_byte *writebuf,
>               ULONGEST offset, LONGEST len)
>  {
> +  client_state &cs = get_client_state ();
>    static struct buffer cache;

This "static" here made me notice that there is more global
state stored as function local static variables that should
be moved to client_state.  E.g.,:

$ nm -A server.o | c++filt  | grep " b " | grep "::"
server.o:0000000000000700 b guard variable for handle_query(char*, int, int*)::thread_iter
server.o:00000000000006f8 b handle_query(char*, int, int*)::thread_iter
server.o:00000000000006c0 b handle_qxfer_btrace(char const*, unsigned char*, unsigned char const*, unsigned long long, long long)::cache
server.o:00000000000006a0 b handle_qxfer_threads(char const*, unsigned char*, unsigned char const*, unsigned long long, long long)::result_length
server.o:0000000000000698 b handle_qxfer_threads(char const*, unsigned char*, unsigned char const*, unsigned long long, long long)::result
server.o:00000000000006e0 b handle_qxfer_btrace_conf(char const*, unsigned char*, unsigned char const*, unsigned long long, long long)::cache
server.o:00000000000006b0 b handle_qxfer_traceframe_info(char const*, unsigned char*, unsigned char const*, unsigned long long, long long)::result_length
server.o:00000000000006a8 b handle_qxfer_traceframe_info(char const*, unsigned char*, unsigned char const*, unsigned long long, long long)::result

That can be done as follow up.

> @@ -3643,7 +3642,7 @@ captured_main (int argc, char *argv[])
>          }
>      }
>        else if (strcmp (*next_arg, "--remote-debug") == 0)
> -    remote_debug = 1;
> +    cs.remote_debug = 1;

remote_debug controls whether to print debug output to gdbserver'
own terminal, so I'm thinking that it should probably remain
a global.

Otherwise it looks good to me.  If we don't need to handle
a warning, then feel free to push this in with the
remote_debug issue above addressed.

Thanks,
Pedro Alves