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

Pedro Alves
2019-09-26T17:47:33Z

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