[2/3] gdb: add remote_state::{is_async_p,can_async_p}

  From: Simon Marchi <simon.marchi@efficios.com>

A subsequent patch will want to know if the remote is async within a
remote_state method.  Add a helper method for that, and for "can async"
as well, for symmetry.

Change-Id: Id0f648ee4896736479fa942f5453eeeb0e5d4352
---
 gdb/remote.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)
  

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

> From: Simon Marchi <simon.marchi@efficios.com>
>
> A subsequent patch will want to know if the remote is async within a
> remote_state method.  Add a helper method for that, and for "can async"
> as well, for symmetry.
>
> Change-Id: Id0f648ee4896736479fa942f5453eeeb0e5d4352
> ---
>  gdb/remote.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 2deba06d7d47..38d0027dbf9e 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -438,6 +438,20 @@ class remote_state
>        ::delete_async_event_handler (&m_async_event_handler_token);
>    }
>  
> +  bool is_async_p () const
> +  {
> +    /* We're async whenever the serial device is.  */
> +    return (this->remote_desc
> +	    != nullptr && serial_is_async_p (this->remote_desc));

This seems like an off choice for line wrapping.  I'd expect the newline
to start with the '&&'.

But... do we ever expect to ask is_async_p() before the remote_desc has
been initialised?  Does the answer even make sense in such a context?

I wonder if we'd be better doing:

  bool is_async_p () const
  {
    gdb_assert (this->remote_desc != nullptr);
    return serial_is_async_p (this->remote_desc);
  }

I'm thinking, that if we did ask this question before remote_desc was
initialised, then when remote_desc was initialised the answer would
change, and so we're probably asking to question too early...

> +  }
> +
> +  bool can_async_p () const
> +  {
> +    /* We can async whenever the serial device can.  */
> +    return (this->remote_desc
> +	    != nullptr && serial_can_async_p (this->remote_desc));

Same here.

Thanks,
Andrew

> +  }
> +  
>  public: /* data */
>  
>    /* A buffer to use for incoming packets, and its current size.  The
> @@ -14853,16 +14867,14 @@ remote_target::can_async_p ()
>    gdb_assert (target_async_permitted);
>  
>    /* We're async whenever the serial device can.  */
> -  struct remote_state *rs = get_remote_state ();
> -  return serial_can_async_p (rs->remote_desc);
> +  return get_remote_state ()->can_async_p ();
>  }
>  
>  bool
>  remote_target::is_async_p ()
>  {
>    /* We're async whenever the serial device is.  */
> -  struct remote_state *rs = get_remote_state ();
> -  return serial_is_async_p (rs->remote_desc);
> +  return get_remote_state ()->is_async_p ();
>  }
>  
>  /* Pass the SERIAL event on and up to the client.  One day this code
> -- 
> 2.42.0
  

On 10/9/23 05:20, Andrew Burgess wrote:
> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> A subsequent patch will want to know if the remote is async within a
>> remote_state method.  Add a helper method for that, and for "can async"
>> as well, for symmetry.
>>
>> Change-Id: Id0f648ee4896736479fa942f5453eeeb0e5d4352
>> ---
>>  gdb/remote.c | 20 ++++++++++++++++----
>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 2deba06d7d47..38d0027dbf9e 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -438,6 +438,20 @@ class remote_state
>>        ::delete_async_event_handler (&m_async_event_handler_token);
>>    }
>>  
>> +  bool is_async_p () const
>> +  {
>> +    /* We're async whenever the serial device is.  */
>> +    return (this->remote_desc
>> +	    != nullptr && serial_is_async_p (this->remote_desc));
> 
> This seems like an off choice for line wrapping.  I'd expect the newline
> to start with the '&&'.

Oops, fixed.

> But... do we ever expect to ask is_async_p() before the remote_desc has
> been initialised?  Does the answer even make sense in such a context?
> 
> I wonder if we'd be better doing:
> 
>   bool is_async_p () const
>   {
>     gdb_assert (this->remote_desc != nullptr);
>     return serial_is_async_p (this->remote_desc);
>   }

That's probably right, I'll try to make that change and re-run the
tests.

> I'm thinking, that if we did ask this question before remote_desc was
> initialised, then when remote_desc was initialised the answer would
> change, and so we're probably asking to question too early...

Yeah... I think for "is_async_p" it would still be a correct answer, the
target isn't in async mode at that moment (before remote_desc is set).
But "can_async_p" could change, once we initialize remote_desc.  Anyhow,
I've put asserts as you suggested.

> 
>> +  }
>> +
>> +  bool can_async_p () const
>> +  {
>> +    /* We can async whenever the serial device can.  */
>> +    return (this->remote_desc
>> +	    != nullptr && serial_can_async_p (this->remote_desc));
> 
> Same here.

Fixed.

Simon