Re: [RFC 00/17] Merge event loop implementations

[Pedro Alves <palves@redhat.com>]

On 2/24/19 4:51 PM, Tom Tromey wrote:
> This series merges the gdb and gdbserver forks of event-loop.[ch].

Thanks for doing this.

> This is an RFC because there are a few possibly unresolved issues.
> 
> Most of the patches are straightforward.  The series begins by tidying
> up the gdb event-loop code, removing things that are specific to gdb.
> Then, the code is moved.  After this, gdbserver is switched to use the
> common code; and finally, a few cleanups are applied.
> 
> I initially attempted something more ambitious here: unifying the
> async event and async signal code in event-loop (as I do not
> understand the reason for the difference); and then further handling
> the async signal code using the same code path as ordinary file
> descriptors.

See:

 https://sourceware.org/ml/gdb-patches/2008-10/msg00595.html

> 
> However, this didn't work, and since it was not directly important to
> my goal of merging event loops, I dropped it.  I think it may be worth
> reviving.  For example I think th async event code suffers from the
> same race that led Pedro to change the async signal code to use a the
> self-pipe trick.



> 
> Another related possible to-do item is changing the ser-event code to
> maintain just a single self-pipe.  It seems to me that there's never a
> reason to need more than one.

Can you clarify/expand?  Are you suggesting to use one single pipe,
and then if select/poll wakes up, go through a list of registered
ser-events to know which one triggered?  Or something else?

> 
> create_file_handler may have a latent bug where the global select
> masks are not updated if it is called a second time for the same file
> descriptor.  Both versions of the event loop have this issue; I didn't
> try to verify it, so perhaps I'm just misunderstanding the code here.

I think you're right.

> 
> The final patch simplifies the rather convoluted handling of "serial"
> (meaning remote protocol) input in gdbserver.  It passes testing, but
> I wonder whether there's some subtle reason that the code is written
> the way it is.  This is one of the unresolved issues I mentioned.

I replied to this one directly.

> 
> The second unresolved issue involves the USE_WIN32API code.  Before
> this series, gdbserver used gdb_fildes_t, defined like:
> 
>     #if USE_WIN32API
>     #include <winsock2.h>
>     typedef SOCKET gdb_fildes_t;
>     #else
>     typedef int gdb_fildes_t;
>     #endif
> 
> gdb did not use this approach, but does have a separate gdb_select
> implementation in mingw-hdep.c, which gdbserver does not.
> 
> I don't know much about Windows, so I don't know why these things are
> needed.  I did a build using " --host=i686-w64-mingw32
> --target=i686-w64-mingw32", and everything built just fine using a
> POSIX-style API.
> 
> Given that, I removed gdb_fildes_t in this series.  However, perhaps
> it is still needed and this series needs some more work.  I could use
> some advice here -- when is this code actually needed and is there a
> way I can reproduce any problems?  I don't have a Windows host, so I'm
> hoping for some sort of compile-time error using a mingw cross.

This was already discussed.  Do I understand correctly that you're
going to try to replace gdb_select with gnulib's select?

Thanks,
Pedro Alves