Re: [PATCH 4/4] gnulib: import select

[Pedro Alves <pedro@palves.net>]

On 2021-06-09 7:32 p.m., Mike Frysinger wrote:
> On 09 Jun 2021 19:12, Pedro Alves wrote:
>> On 2021-06-09 5:52 p.m., Mike Frysinger wrote:
>>> On 09 Jun 2021 13:27, Pedro Alves wrote:
>>>> I've always feared that gnulib's select module would conflict with that,
>>>> but I don't know for sure.  In theory, we should be able to replace all
>>>> that code by using gnulib's select instead, though I'm not sure gnulib
>>>> handles some particulars like Ctrl-C interruption.
>>>
>>> why do you think it would conflict ?  the file doesn't seem to use select()
>>> at all, and ser-mingw is always enabled for mingw targets.  seems like the
>>> func would just be ignored.
>>
>> gnulib also sometimes replaces headers with their own incompatible
>> definitions.
> 
> good point, but i'm not sure how that matters to ser-mingw specifically.
> it isn't including any select.h related headers, just Windows-specific ones.

I meant to also point at mingw-hdep.c, which is where gdb_select implementation lives.
That of course, includes sys/select.h via gdb_select.h.
I misremembered that gdb_select itself was in ser-mingw.c too.  mingw's version
of gdb_select is built on top of the ser-mingw.c and struct serial, but lives
in mingw-hdep.c.

Also, the sockets module was pulled in as well as dependency, and I suppose that hides the SOCKET vs
int, close vs closesocket, _get_osfhandle, read/write vs send/recv etc. WinSock2 particularities,
and don't know whether that can cause issues in the code that we have that assumes native Windows
sockets on mingw, which you'll find a lot of if you look for USE_WIN32API, particularly in
gdbserver and gdbreplay.

If it does, it may not be noticeable with a build-only check.

Even if it doesn't, there's now a lot of USE_WIN32API code in gdb, gdbsupport
and gdbserver that is now ripe for cleansing.  In addition to close vs closesocket, read vs send
etc I mentioned above, there should be no need anymore to include the windows/winsock headers to
get access to sockets things, we should call gl_sockets_startup instead of WSAStartup, etc.

> i haven't gone through that list (and indirect includes) explicitly.  but i
> would expect the build to at least warn or fail if that were the case.
> 
> did you have any specific concerns here ?  or just aiming for due diligence
> with the testsuite to assuage general unease ?

The latter, but I wasn't thinking of a testsuite run, which is notoriously difficult
on Windows.  More like a smoke test, run GDB, to make sure you can still debug something.

Also, a quick check to make sure remote debugging against a Windows GDBserver built
from master still works would be good -- gdbserver also uses gnulib, and uses select
(not gdb_select) and sockets.

Interestingly, this module import may have put us closer to removing the Windows
gdbserver limitation that it only works with tcp/ip socket connections, particularly
interesting being pipes, for "target remote | gdbserver" support.

Thanks,
Pedro Alves