Re: [PATCH 1/3] [gdbsupport] Add gdb::{waitpid,read,write,close}

[Tom de Vries <tdevries@suse.de>]

On 12/2/24 10:38, Kévin Le Gouguec wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>>> When building for mingw-w64, I see this, which seems related:
>>>         CXX    cli/cli-cmds.o
>>>       In file included from /home/simark/src/binutils-gdb/gdb/cli/cli-cmds.c:58:
>>>       /home/simark/src/binutils-gdb/gdb/../gdbsupport/eintr.h: In function ‘pid_t gdb::waitpid(pid_t, int*, int)’:
>>>       /home/simark/src/binutils-gdb/gdb/../gdbsupport/eintr.h:77:35: error: ‘::waitpid’ has not been declared; did you mean ‘gdb::waitpid’?
>>>          77 |   return gdb::handle_eintr (-1, ::waitpid, pid, wstatus, options);
>>>             |                                   ^~~~~~~
>>>             |                                   gdb::waitpid
>>>       /home/simark/src/binutils-gdb/gdb/../gdbsupport/eintr.h:75:1: note: ‘gdb::waitpid’ declared here
>>>          75 | waitpid (pid_t pid, int *wstatus, int options)
>>>             | ^~~~~~~
>>> Note that to get to it, you have to step past this other build error in
>>> gdbserver (by doing `make all-gdb` for instance):
>>>     https://inbox.sourceware.org/gdb-patches/75926446-4c8a-40ee-8ed8-ab55f38e1520@simark.ca/T/#m194744b18419e37dfb4ec82c8e0ce73fb017ba17
>>
>> Hi Simon,
>>
>> thanks for reporting this.
>>
>> I don't have a windows setup unfortunately, so I have no way of reproducing this, and at this point I have no idea why waitpid is not declared.
> 
> FWIW (not much: not very Windows-savvy), 

Hi Kevin,

same here.

> I fixed my test build with the
> following patch.  I was also getting a "'::wait' has not been declared"
> error, hence the extra #ifdef (found no HAVE_ macro for 'wait', so went
> with HAVE_SYS_WAIT_H after glancing the 3posix manpage).
> 
> (Figured I'd share since that let me build & test native Windows, though
> emphatically not suggesting this as a "proper fix": not sure "wait ⇔
> HAVE_SYS_WAIT_H" is a correct implication, haven't checked if gnulib
> might help, …)
> 

Thanks for sharing this.

I've been looking into this, and am currently testing a patch that 
simply uses "#ifndef USE_WIN32API".

The HAVE_WAITPID is available in gdb but not gdbserver, so it could be 
used but would need moving to gdbsupport or duplicating to gdbserver.
I suppose it would make sense to add a HAVE_WAIT alongside while doing that.

I've left the '#include <sys/wait.h>' unmodified, since it causes no 
compilation problems, because there's a gnulib fallback.

The situation around sys/wait.h and HAVE_SYS_WAIT_H is a bit confusing, 
for me at least.  The platform doesn't have a sys/wait.h, but gnulib 
provides a fallback.  While configuring, we ignore the gnulib header, 
and so HAVE_SYS_WAIT_H is undefined.  But when compiling the gnulib 
header is not ignored, so we can use '#include <sys/wait.h>'.  AFAIU in 
the current situation, the only thing that guarding '#include 
<sys/wait.h>' with HAVE_SYS_WAIT_H achieves is preventing the gnulib 
fallback
from being included.  I suspect that this is a configuration bug, but 
I'm not sure.

BTW, the canonical way (according to ARI) of including <sys/wait.h> 
seems to be including gdb_wait.h (which does use HAVE_SYS_WAIT_H).

Thanks,
- Tom

> diff --git a/gdbsupport/eintr.h b/gdbsupport/eintr.h
> index 3980e3f5ac1..2bd17108f6e 100644
> --- a/gdbsupport/eintr.h
> +++ b/gdbsupport/eintr.h
> @@ -22,7 +22,9 @@
>   
>   #include <cerrno>
>   #include <sys/types.h>
> +#ifdef HAVE_SYS_WAIT_H
>   #include <sys/wait.h>
> +#endif
>   #include <sys/stat.h>
>   #include <fcntl.h>
>   #include <unistd.h>
> @@ -71,11 +73,13 @@ handle_eintr (ErrorValType errval, const Fun &f, const Args &... args)
>     return ret;
>   }
>   
> +#ifdef HAVE_WAITPID
>   inline pid_t
>   waitpid (pid_t pid, int *wstatus, int options)
>   {
>     return gdb::handle_eintr (-1, ::waitpid, pid, wstatus, options);
>   }
> +#endif
>   
>   inline int
>   open (const char *pathname, int flags)
> @@ -83,11 +87,13 @@ open (const char *pathname, int flags)
>     return gdb::handle_eintr (-1, ::open, pathname, flags);
>   }
>   
> +#ifdef HAVE_SYS_WAIT_H
>   inline pid_t
>   wait (int *wstatus)
>   {
>     return gdb::handle_eintr (-1, ::wait, wstatus);
>   }
> +#endif
>   
>   inline int
>   close (int fd)
>