gdb build problem (gdb/unittests/scoped_mmap-selftests.c)

gdb build problem (gdb/unittests/scoped_mmap-selftests.c)

[Steve Ellcey]

Is anyone else still having a problem building gdb?  I saw the 
mail about remote.c but I currently get this failure when
building gdb with gcc 5.4.0 on an aarch64 linux box.

/home/sellcey/tot/src/binutils-gdb/gdb/unittests/scoped_mmap-selftests.c: In function ‘void selftests::mmap_file::test_normal()’:
/home/sellcey/tot/src/binutils-gdb/gdb/unittests/scoped_mmap-selftests.c:94:26: error: ignoring return value of ‘ssize_t write(int, const void*, size_t)’, declared with attribute warn_unused_result [-Werror=unused-result]
   write (fd, "Hello!", 7);



It looks like this file was changed yesterday.


% git log scoped_mmap-selftests.c | more
commit 5c831bb1eb6b22cd1705b98188b7d1b0633e7c54
Author: Simon Marchi <simon.marchi@ericsson.com>
Date:   Tue Aug 7 18:10:29 2018 -0400

    Introduce mmap_file function



----
Steve Ellcey
sellcey@cavium.com

Re: gdb build problem (gdb/unittests/scoped_mmap-selftests.c)

[Tom Tromey]

>>>>> "Steve" == Steve Ellcey <sellcey@cavium.com> writes:

Steve> Is anyone else still having a problem building gdb?  I saw the 
Steve> mail about remote.c but I currently get this failure when
Steve> building gdb with gcc 5.4.0 on an aarch64 linux box.

I haven't seen this, but IIRC some distros enable _FORTIFY_SOURCE  by
default, which changes which unused-result warnings are emitted.

Does the appended work for you?

I wonder if we should enable _FORTIFY_SOURCE for development builds.

Tom

diff --git a/gdb/unittests/scoped_mmap-selftests.c b/gdb/unittests/scoped_mmap-selftests.c
index d70a56a1862..e9d4afdffc5 100644
--- a/gdb/unittests/scoped_mmap-selftests.c
+++ b/gdb/unittests/scoped_mmap-selftests.c
@@ -91,7 +91,7 @@ test_normal ()
   int fd = mkstemp (filename);
   SELF_CHECK (fd >= 0);
 
-  write (fd, "Hello!", 7);
+  SELF_CHECK (write (fd, "Hello!", 7) == 7);
   close (fd);
 
   gdb::unlinker unlink_test_file (filename);

Re: gdb build problem (gdb/unittests/scoped_mmap-selftests.c)

[Simon Marchi]

On 2018-08-08 13:35, Tom Tromey wrote:
>>>>>> "Steve" == Steve Ellcey <sellcey@cavium.com> writes:
> 
> Steve> Is anyone else still having a problem building gdb?  I saw the 
> Steve> mail about remote.c but I currently get this failure when
> Steve> building gdb with gcc 5.4.0 on an aarch64 linux box.
> 
> I haven't seen this, but IIRC some distros enable _FORTIFY_SOURCE  by
> default, which changes which unused-result warnings are emitted.
> 
> Does the appended work for you?
> 
> I wonder if we should enable _FORTIFY_SOURCE for development builds.
> 
> Tom
> 
> diff --git a/gdb/unittests/scoped_mmap-selftests.c
> b/gdb/unittests/scoped_mmap-selftests.c
> index d70a56a1862..e9d4afdffc5 100644
> --- a/gdb/unittests/scoped_mmap-selftests.c
> +++ b/gdb/unittests/scoped_mmap-selftests.c
> @@ -91,7 +91,7 @@ test_normal ()
>    int fd = mkstemp (filename);
>    SELF_CHECK (fd >= 0);
> 
> -  write (fd, "Hello!", 7);
> +  SELF_CHECK (write (fd, "Hello!", 7) == 7);
>    close (fd);
> 
>    gdb::unlinker unlink_test_file (filename);

Oops, sorry for the breakage.  Tom's fix LGTM.

I think that enabling _FORTIFY_SOURCE can only do some good.

Simon

Re: gdb build problem (gdb/unittests/scoped_mmap-selftests.c)

[Steve Ellcey]

On Wed, 2018-08-08 at 11:35 -0600, Tom Tromey wrote:
> 
> I haven't seen this, but IIRC some distros enable _FORTIFY_SOURCE  by
> default, which changes which unused-result warnings are emitted.
> 
> Does the appended work for you?
> 
> I wonder if we should enable _FORTIFY_SOURCE for development builds.
> 
> Tom
> 
> diff --git a/gdb/unittests/scoped_mmap-selftests.c
> b/gdb/unittests/scoped_mmap-selftests.c
> index d70a56a1862..e9d4afdffc5 100644
> --- a/gdb/unittests/scoped_mmap-selftests.c
> +++ b/gdb/unittests/scoped_mmap-selftests.c
> @@ -91,7 +91,7 @@ test_normal ()
>    int fd = mkstemp (filename);
>    SELF_CHECK (fd >= 0);
> 
> -  write (fd, "Hello!", 7);
> +  SELF_CHECK (write (fd, "Hello!", 7) == 7);
>    close (fd);
> 
>    gdb::unlinker unlink_test_file (filename);

Yes, this patch fixed the build problem I was having.  I was building
on Ubuntu 16.04 so I guess that one of the platforms that enables
_FORTIFY_SOURCE by default.

Steve Ellcey
sellcey@cavium.com

Re: gdb build problem (gdb/unittests/scoped_mmap-selftests.c)

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> I think that enabling _FORTIFY_SOURCE can only do some good.

What do you think of the appended?
The check for __OPTIMIZE__ has to be done since otherwise a glibc header
will complain.

Building with this patch applied (after autoheader etc) let me reproduce
Steve's original problem.

Tom

diff --git a/gdb/configure.ac b/gdb/configure.ac
index 13bc5f9a8f2..76a1ba0364f 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -2279,6 +2279,10 @@ dnl  At the moment, we just assume it's UTF-8.
 AC_DEFINE(GDB_DEFAULT_HOST_CHARSET, "UTF-8",
           [Define to be a string naming the default host character set.])
 
+AH_BOTTOM([#if defined __OPTIMIZE__ && __OPTIMIZE__ > 0
+#define _FORTIFY_SOURCE 2
+#endif])
+
 if $development; then
   AC_DEFINE(GDB_SELF_TEST, 1,
             [Define if self-testing features should be enabled])

Re: gdb build problem (gdb/unittests/scoped_mmap-selftests.c)

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> Oops, sorry for the breakage.  Tom's fix LGTM.

Here's what I'm checking in.

Tom

commit 1ca8d27a27b8f05272764bf9ac1799cd86c74f52
Author: Tom Tromey <tom@tromey.com>
Date:   Wed Aug 8 11:57:39 2018 -0600

    Check result of "write"
    
    Some distros enable _FORTIFY_SOURCE by default, which caught a failure
    to check the result of "write" in scoped_mmap-selftests.c.  This patch
    fixes the problem.
    
    ChangeLog
    2018-08-08  Tom Tromey  <tom@tromey.com>
    
            * unittests/scoped_mmap-selftests.c: Check result of "write".

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 682520fc400..4181da6c01a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2018-08-08  Tom Tromey  <tom@tromey.com>
+
+	* unittests/scoped_mmap-selftests.c: Check result of "write".
+
 2018-08-08  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	PR gdb/18050:
diff --git a/gdb/unittests/scoped_mmap-selftests.c b/gdb/unittests/scoped_mmap-selftests.c
index d70a56a1862..e9d4afdffc5 100644
--- a/gdb/unittests/scoped_mmap-selftests.c
+++ b/gdb/unittests/scoped_mmap-selftests.c
@@ -91,7 +91,7 @@ test_normal ()
   int fd = mkstemp (filename);
   SELF_CHECK (fd >= 0);
 
-  write (fd, "Hello!", 7);
+  SELF_CHECK (write (fd, "Hello!", 7) == 7);
   close (fd);
 
   gdb::unlinker unlink_test_file (filename);

Re: gdb build problem (gdb/unittests/scoped_mmap-selftests.c)

[Simon Marchi]

On 2018-08-08 13:56, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> I think that enabling _FORTIFY_SOURCE can only do some good.
> 
> What do you think of the appended?
> The check for __OPTIMIZE__ has to be done since otherwise a glibc 
> header
> will complain.
> 
> Building with this patch applied (after autoheader etc) let me 
> reproduce
> Steve's original problem.
> 
> Tom
> 
> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index 13bc5f9a8f2..76a1ba0364f 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -2279,6 +2279,10 @@ dnl  At the moment, we just assume it's UTF-8.
>  AC_DEFINE(GDB_DEFAULT_HOST_CHARSET, "UTF-8",
>            [Define to be a string naming the default host character 
> set.])
> 
> +AH_BOTTOM([#if defined __OPTIMIZE__ && __OPTIMIZE__ > 0
> +#define _FORTIFY_SOURCE 2
> +#endif])
> +
>  if $development; then
>    AC_DEFINE(GDB_SELF_TEST, 1,
>              [Define if self-testing features should be enabled])

Oh right, _FORTIFY_SOURCE requires building with optimization.  That 
code will end up at the bottom of config.h, is that it?  If so, it LGTM.

Simon

Re: gdb build problem (gdb/unittests/scoped_mmap-selftests.c)

[Pedro Alves]

On 08/08/2018 06:56 PM, Tom Tromey wrote:

> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index 13bc5f9a8f2..76a1ba0364f 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -2279,6 +2279,10 @@ dnl  At the moment, we just assume it's UTF-8.
>  AC_DEFINE(GDB_DEFAULT_HOST_CHARSET, "UTF-8",
>            [Define to be a string naming the default host character set.])
>  
> +AH_BOTTOM([#if defined __OPTIMIZE__ && __OPTIMIZE__ > 0
> +#define _FORTIFY_SOURCE 2
> +#endif])

Do we really need to put this in config.h?  Wouldn't putting it straight
in common/common-defs.h work out the same?  We already define
__STDC_CONSTANT_MACRO etc. there before any system header.

Thanks,
Pedro Alves

Re: gdb build problem (gdb/unittests/scoped_mmap-selftests.c)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> On 08/08/2018 06:56 PM, Tom Tromey wrote:
>> diff --git a/gdb/configure.ac b/gdb/configure.ac
>> index 13bc5f9a8f2..76a1ba0364f 100644
>> --- a/gdb/configure.ac
>> +++ b/gdb/configure.ac
>> @@ -2279,6 +2279,10 @@ dnl  At the moment, we just assume it's UTF-8.
>> AC_DEFINE(GDB_DEFAULT_HOST_CHARSET, "UTF-8",
>> [Define to be a string naming the default host character set.])
>> 
>> +AH_BOTTOM([#if defined __OPTIMIZE__ && __OPTIMIZE__ > 0
>> +#define _FORTIFY_SOURCE 2
>> +#endif])

Pedro> Do we really need to put this in config.h?  Wouldn't putting it straight
Pedro> in common/common-defs.h work out the same?  We already define
Pedro> __STDC_CONSTANT_MACRO etc. there before any system header.

That does seems simpler, and I think it will work just as well.

Tom