diff mbox

nat/fork-inferior: include linux-ptrace.h

Message ID 20180625080547.7629-1-thomas.petazzoni@bootlin.com
State New, archived
Headers

Commit Message

Thomas Petazzoni June 25, 2018, 8:05 a.m. UTC 
  To decide whether fork() or vfork() should be used, fork-inferior.c
uses the following test:

  #if !(defined(__UCLIBC__) && defined(HAS_NOMMU))

However, HAS_NOMMU is never defined, because it gets defined in
linux-ptrace.h, which is not included by fork-inferior.c. Due to this,
gdbserver fails to build on noMMU architectures. This commit fixes
that by simply including linux-ptrace.h.

This bug was introduced by commit
2090129c36c7e582943b7d300968d19b46160d84 ("Share fork_inferior et al
with gdbserver"). Indeed, the same fork()/vfork() selection was done,
but in another file where linux-ptrace.h was included.

Fixes the following build issue:

../nat/fork-inferior.c: In function 'pid_t fork_inferior(const char*, const string&, char**, void (*)(), void (*)(int), void (*)(), const char*, void (*)(const char*, char* const*, char* const*))':
../nat/fork-inferior.c:376:11: error: 'fork' was not declared in this scope
     pid = fork ();
           ^~~~
../nat/fork-inferior.c:376:11: note: suggested alternative: 'vfork'
     pid = fork ();
           ^~~~
           vfork

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 gdb/nat/fork-inferior.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Simon Marchi June 27, 2018, 2:24 p.m. UTC | #1 
On 2018-06-25 04:05, Thomas Petazzoni wrote:
> To decide whether fork() or vfork() should be used, fork-inferior.c
> uses the following test:
> 
>   #if !(defined(__UCLIBC__) && defined(HAS_NOMMU))
> 
> However, HAS_NOMMU is never defined, because it gets defined in
> linux-ptrace.h, which is not included by fork-inferior.c. Due to this,
> gdbserver fails to build on noMMU architectures. This commit fixes
> that by simply including linux-ptrace.h.
> 
> This bug was introduced by commit
> 2090129c36c7e582943b7d300968d19b46160d84 ("Share fork_inferior et al
> with gdbserver"). Indeed, the same fork()/vfork() selection was done,
> but in another file where linux-ptrace.h was included.
> 
> Fixes the following build issue:
> 
> ../nat/fork-inferior.c: In function 'pid_t fork_inferior(const char*,
> const string&, char**, void (*)(), void (*)(int), void (*)(), const
> char*, void (*)(const char*, char* const*, char* const*))':
> ../nat/fork-inferior.c:376:11: error: 'fork' was not declared in this 
> scope
>      pid = fork ();
>            ^~~~
> ../nat/fork-inferior.c:376:11: note: suggested alternative: 'vfork'
>      pid = fork ();
>            ^~~~
>            vfork
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
>  gdb/nat/fork-inferior.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
> index 8b59387fa5..05167628a6 100644
> --- a/gdb/nat/fork-inferior.c
> +++ b/gdb/nat/fork-inferior.c
> @@ -26,6 +26,7 @@
>  #include "common-gdbthread.h"
>  #include "signals-state-save-restore.h"
>  #include "gdb_tilde_expand.h"
> +#include "linux-ptrace.h"
>  #include <vector>
> 
>  extern char **environ;

Hi Thomas,

fork-inferior.c is also included in native builds for BSDs, AIX, Solaris 
and Darwin (see gdb/configure.nat).  I am a bit concerned that 
linux-ptrace.h could use some Linux-specific things, and thus would 
break the other builds.  However, I built-tested on FreeBSD and it seems 
fine.  Worst case, we can probably wrap this include in "#ifdef 
__linux__" if that becomes a problem.

Do you have push access, or do you prefer if I push the patch for you?

I suppose that error was caught by a Buildroot autobuilder?  Would it be 
possible to have the config, so I can add a similar configuration to my 
collection of cross-compiled GDB builds I use for build-testing?

Thanks,

Simon
Pedro Alves June 27, 2018, 2:30 p.m. UTC | #2 
On 06/27/2018 03:24 PM, Simon Marchi wrote:
> On 2018-06-25 04:05, Thomas Petazzoni wrote:
>> To decide whether fork() or vfork() should be used, fork-inferior.c
>> uses the following test:
>>
>>   #if !(defined(__UCLIBC__) && defined(HAS_NOMMU))
>>
>> However, HAS_NOMMU is never defined, because it gets defined in
>> linux-ptrace.h, which is not included by fork-inferior.c. Due to this,
>> gdbserver fails to build on noMMU architectures. This commit fixes
>> that by simply including linux-ptrace.h.
>>
>> This bug was introduced by commit
>> 2090129c36c7e582943b7d300968d19b46160d84 ("Share fork_inferior et al
>> with gdbserver"). Indeed, the same fork()/vfork() selection was done,
>> but in another file where linux-ptrace.h was included.
>>
>> Fixes the following build issue:
>>
>> ../nat/fork-inferior.c: In function 'pid_t fork_inferior(const char*,
>> const string&, char**, void (*)(), void (*)(int), void (*)(), const
>> char*, void (*)(const char*, char* const*, char* const*))':
>> ../nat/fork-inferior.c:376:11: error: 'fork' was not declared in this scope
>>      pid = fork ();
>>            ^~~~
>> ../nat/fork-inferior.c:376:11: note: suggested alternative: 'vfork'
>>      pid = fork ();
>>            ^~~~
>>            vfork
>>
>> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
>> ---
>>  gdb/nat/fork-inferior.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
>> index 8b59387fa5..05167628a6 100644
>> --- a/gdb/nat/fork-inferior.c
>> +++ b/gdb/nat/fork-inferior.c
>> @@ -26,6 +26,7 @@
>>  #include "common-gdbthread.h"
>>  #include "signals-state-save-restore.h"
>>  #include "gdb_tilde_expand.h"
>> +#include "linux-ptrace.h"
>>  #include <vector>
>>
>>  extern char **environ;
> 
> Hi Thomas,
> 
> fork-inferior.c is also included in native builds for BSDs, AIX, Solaris and Darwin (see gdb/configure.nat).  I am a bit concerned that linux-ptrace.h could use some Linux-specific things, and thus would break the other builds.  However, I built-tested on FreeBSD and it seems fine.  Worst case, we can probably wrap this include in "#ifdef __linux__" if that becomes a problem.

Please don't.  It seems very wrong to me to include this header on other hosts.
It is full of Linux-only details.

Thanks,
Pedro Alves
Thomas Petazzoni June 27, 2018, 2:31 p.m. UTC | #3 
Hello Simon,

On Wed, 27 Jun 2018 10:24:07 -0400, Simon Marchi wrote:

> > diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
> > index 8b59387fa5..05167628a6 100644
> > --- a/gdb/nat/fork-inferior.c
> > +++ b/gdb/nat/fork-inferior.c
> > @@ -26,6 +26,7 @@
> >  #include "common-gdbthread.h"
> >  #include "signals-state-save-restore.h"
> >  #include "gdb_tilde_expand.h"
> > +#include "linux-ptrace.h"
> >  #include <vector>
> > 
> >  extern char **environ;  
> 
> Hi Thomas,
> 
> fork-inferior.c is also included in native builds for BSDs, AIX, Solaris 
> and Darwin (see gdb/configure.nat).  I am a bit concerned that 
> linux-ptrace.h could use some Linux-specific things, and thus would 
> break the other builds.  However, I built-tested on FreeBSD and it seems 
> fine.  Worst case, we can probably wrap this include in "#ifdef 
> __linux__" if that becomes a problem.

Or better, this horrible mess of __UCLIBC__ and HAS_NOMMU macros should
be replaced by a proper autoconf check testing for the availability of
fork().

> Do you have push access, or do you prefer if I push the patch for you?

I don't have push access.

> I suppose that error was caught by a Buildroot autobuilder?  Would it be 
> possible to have the config, so I can add a similar configuration to my 
> collection of cross-compiled GDB builds I use for build-testing?

The error wasn't caught by the autobuilders, but by the automated build
logic we use for http://toolchains.bootlin.com to build toolchains for
a wide range of CPU architectures.

The failure was
https://gitlab.com/free-electrons/toolchains-builder/-/jobs/77021057,
and the Buildroot configuration being built was:

BR2_m68k=y
BR2_m68k_cf5208=y
BR2_WGET="wget --passive-ftp -nd -t 3 --no-check-certificate"
BR2_HOST_DIR="/opt/m68k-coldfire--uclibc--bleeding-edge-2018.06-1"
BR2_GNU_MIRROR="http://ftp.gnu.org/pub/gnu"
BR2_KERNEL_HEADERS_4_14=y
BR2_TOOLCHAIN_BUILDROOT_LOCALE=y
BR2_PTHREAD_DEBUG=y
BR2_BINUTILS_VERSION_2_30_X=y
BR2_GCC_VERSION_8_X=y
BR2_TOOLCHAIN_BUILDROOT_CXX=y
BR2_PACKAGE_HOST_GDB=y
BR2_GDB_VERSION_8_1=y
BR2_INIT_NONE=y
# BR2_PACKAGE_BUSYBOX is not set
BR2_PACKAGE_GDB=y
# BR2_TARGET_ROOTFS_TAR is not set

Note that with Buildroot's latest master, this configuration will build
fine, because we already merged the fix for the fork/vfork issue.

Best regards,

Thomas Petazzoni
Simon Marchi June 27, 2018, 2:34 p.m. UTC | #4 
On 2018-06-27 10:31, Thomas Petazzoni wrote:
>> fork-inferior.c is also included in native builds for BSDs, AIX, 
>> Solaris
>> and Darwin (see gdb/configure.nat).  I am a bit concerned that
>> linux-ptrace.h could use some Linux-specific things, and thus would
>> break the other builds.  However, I built-tested on FreeBSD and it 
>> seems
>> fine.  Worst case, we can probably wrap this include in "#ifdef
>> __linux__" if that becomes a problem.
> 
> Or better, this horrible mess of __UCLIBC__ and HAS_NOMMU macros should
> be replaced by a proper autoconf check testing for the availability of
> fork().

Agreed, and I think Pedro will agree too (unless you can think of a 
simpler solution that doesn't involve autoconf?).

Simon
Joel Brobecker June 27, 2018, 4:29 p.m. UTC | #5 
> > > fork-inferior.c is also included in native builds for BSDs, AIX,
> > > Solaris
> > > and Darwin (see gdb/configure.nat).  I am a bit concerned that
> > > linux-ptrace.h could use some Linux-specific things, and thus would
> > > break the other builds.  However, I built-tested on FreeBSD and it
> > > seems
> > > fine.  Worst case, we can probably wrap this include in "#ifdef
> > > __linux__" if that becomes a problem.
> > 
> > Or better, this horrible mess of __UCLIBC__ and HAS_NOMMU macros should
> > be replaced by a proper autoconf check testing for the availability of
> > fork().
> 
> Agreed, and I think Pedro will agree too (unless you can think of a simpler
> solution that doesn't involve autoconf?).

We already autoconf for "fork" (see HAVE_FORK in config.in).
Does that help?
Pedro Alves June 27, 2018, 4:32 p.m. UTC | #6 
On 06/27/2018 03:34 PM, Simon Marchi wrote:
> On 2018-06-27 10:31, Thomas Petazzoni wrote:
>>> fork-inferior.c is also included in native builds for BSDs, AIX, Solaris
>>> and Darwin (see gdb/configure.nat).  I am a bit concerned that
>>> linux-ptrace.h could use some Linux-specific things, and thus would
>>> break the other builds.  However, I built-tested on FreeBSD and it seems
>>> fine.  Worst case, we can probably wrap this include in "#ifdef
>>> __linux__" if that becomes a problem.
>>
>> Or better, this horrible mess of __UCLIBC__ and HAS_NOMMU macros should
>> be replaced by a proper autoconf check testing for the availability of
>> fork().
> 
> Agreed, and I think Pedro will agree too (unless you can think of a simpler solution that doesn't involve autoconf?).

That does sound better.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
index 8b59387fa5..05167628a6 100644
--- a/gdb/nat/fork-inferior.c
+++ b/gdb/nat/fork-inferior.c
@@ -26,6 +26,7 @@ 
 #include "common-gdbthread.h"
 #include "signals-state-save-restore.h"
 #include "gdb_tilde_expand.h"
+#include "linux-ptrace.h"
 #include <vector>
 
 extern char **environ;