Re: [RFC PATCH 1/7] gdbsupport: Add an event-pipe class.

[John Baldwin <jhb@FreeBSD.org>]

On 6/27/21 9:12 AM, Pedro Alves wrote:
> On 2021-06-07 6:09 p.m., John Baldwin wrote:
>> This pulls out the implementation of an event pipe used to implement
>> target async support in both linux-low.cc (gdbserver) and linux-nat.c
>> (gdb).
>>
> 
> Seems like a good idea, thanks.
> 
>> +  /* Create a new pipe.  */
>> +  bool open ();
>> +
>> +  /* Close the pipe.  */
>> +  void close ();
>> +
>> +  /* True if the event pipe has been initialized.  */
>> +  bool active () const { return fds[0] != -1; }
>> +
> 
> Did you consider "is_open" instead of "active"?  I think reading "active()" at the call
> sites can make one wonder whether "active" is a different state from being
> open, like e.g., the pipe is open but not registered in the event loop.

Hmmm, I hadn't considered is_open.  I think was just going off the names of the existing
macros in the Linux targets, but I prefer is_open and will switch to that.

>> diff --git a/gdbsupport/event-pipe.h b/gdbsupport/event-pipe.h
>> new file mode 100644
>> index 0000000000..1717ea6790
>> --- /dev/null
>> +++ b/gdbsupport/event-pipe.h
>> @@ -0,0 +1,55 @@
>> +/* Event pipe for GDB, the GNU debugger.
>> +
>> +   Copyright (C) 2021 Free Software Foundation, Inc.
>> +
>> +   This file is part of GDB.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +#ifndef COMMON_EVENT_PIPE_H
>> +#define COMMON_EVENT_PIPE_H
>> +
>> +/* An event pipe used as a waitable file in the event loop in place of
>> +   some other event not associated with a file descriptor.  */
> 
> Note this comment can be a little bit unclear given we also have
> create_async_event_handler / mark_async_event_handler, for events not associated
> with a file descriptor.
> 
> We need to use a pipe iff we need to wake up the select/poll in the event loop
> from a signal handler, otherwise we can use the cheaper mark_async_event_handler
> from mainline code.
> 
> Maybe mention in the comment that this is used for implementing the well-known
> self-pipe trick?

Yeah, I wanted to find a way to mention that anyway, and these are good points.
Here is what I came up with:

/* An event pipe used as a waitable file in the event loop in place of
    some other event associated with a signal.  The handler for the
    signal marks the event pipe to force a wakeup in the event loop.
    This uses the well-known self-pipe trick.  */

-- 
John Baldwin