[Darwin,testsuite,committed] Fix Wnonnull on Darwin.
diff mbox series

Message ID 5DDF751A-C750-4CC7-8272-966E663534CF@sandoe.co.uk
State New
Headers show
Series
  • [Darwin,testsuite,committed] Fix Wnonnull on Darwin.
Related show

Commit Message

Iain Sandoe Oct. 19, 2019, 7:55 a.m. UTC
This test has failed always on Darwin, because Darwin does not mark
entries in string.h with nonnull attributes.  Since the purpose of the test
is to check that the warnings are issued for an inlined function, not that
the target headers are marked up, we can provide locally marked up
function declarations for Darwin.

tested on x68_64-darwin16, x86_64-linux-gnu,
applied to mainline,
thanks,
Iain.

gcc/testsuite/ChangeLog:

2019-10-19  Iain Sandoe  <iain@sandoe.co.uk>

	* gcc.dg/Wnonnull.c: Add attributed function declarations for
	memcpy and strlen for Darwin.

Comments

Andreas Schwab Oct. 19, 2019, 8:47 a.m. UTC | #1
On Okt 19 2019, Iain Sandoe <iain@sandoe.co.uk> wrote:

> This test has failed always on Darwin, because Darwin does not mark
> entries in string.h with nonnull attributes.  Since the purpose of the test
> is to check that the warnings are issued for an inlined function, not that
> the target headers are marked up, we can provide locally marked up
> function declarations for Darwin.

If the test depends on the non-std declarations, then it should use them
everywhere.

> diff --git a/gcc/testsuite/gcc.dg/Wnonnull.c b/gcc/testsuite/gcc.dg/Wnonnull.c
> index be89a5a755..a165baa99f 100644
> --- a/gcc/testsuite/gcc.dg/Wnonnull.c
> +++ b/gcc/testsuite/gcc.dg/Wnonnull.c
> @@ -2,7 +2,16 @@
>     { dg-do compile }
>     { dg-options "-O2 -Wall" } */
>  
> +#ifndef __APPLE__
>  #include <string.h>
> +#else
> +/* OSX headers do not mark up the nonnull elements yet.  */
> +# include <stddef.h>
> +extern size_t strlen (const char *__s)
> +		      __attribute ((pure)) __attribute ((nonnull (1)));
> +extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
> +		     size_t __n) __attribute ((nonnull (1, 2)));
> +#endif

Perhaps use __SIZE_TYPE__ instead of #include <stddef.h>?

Andreas.
Iain Sandoe Oct. 19, 2019, 8:56 a.m. UTC | #2
Andreas Schwab <schwab@linux-m68k.org> wrote:

> On Okt 19 2019, Iain Sandoe <iain@sandoe.co.uk> wrote:
> 
>> This test has failed always on Darwin, because Darwin does not mark
>> entries in string.h with nonnull attributes.  Since the purpose of the test
>> is to check that the warnings are issued for an inlined function, not that
>> the target headers are marked up, we can provide locally marked up
>> function declarations for Darwin.
> 
> If the test depends on the non-std declarations, then it should use them
> everywhere.

from my perspective, agreed, Martin?

>> diff --git a/gcc/testsuite/gcc.dg/Wnonnull.c b/gcc/testsuite/gcc.dg/Wnonnull.c
>> index be89a5a755..a165baa99f 100644
>> --- a/gcc/testsuite/gcc.dg/Wnonnull.c
>> +++ b/gcc/testsuite/gcc.dg/Wnonnull.c
>> @@ -2,7 +2,16 @@
>>    { dg-do compile }
>>    { dg-options "-O2 -Wall" } */
>> 
>> +#ifndef __APPLE__
>> #include <string.h>
>> +#else
>> +/* OSX headers do not mark up the nonnull elements yet.  */
>> +# include <stddef.h>
>> +extern size_t strlen (const char *__s)
>> +		      __attribute ((pure)) __attribute ((nonnull (1)));
>> +extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
>> +		     size_t __n) __attribute ((nonnull (1, 2)));
>> +#endif
> 
> Perhaps use __SIZE_TYPE__ instead of #include <stddef.h>?

if we make the definitions generic, then this would avoid any header includes.

Iain
Martin Sebor Oct. 19, 2019, 3:38 p.m. UTC | #3
On 10/19/19 2:56 AM, Iain Sandoe wrote:
> Andreas Schwab <schwab@linux-m68k.org> wrote:
> 
>> On Okt 19 2019, Iain Sandoe <iain@sandoe.co.uk> wrote:
>>
>>> This test has failed always on Darwin, because Darwin does not mark
>>> entries in string.h with nonnull attributes.  Since the purpose of the test
>>> is to check that the warnings are issued for an inlined function, not that
>>> the target headers are marked up, we can provide locally marked up
>>> function declarations for Darwin.
>>
>> If the test depends on the non-std declarations, then it should use them
>> everywhere.
> 
> from my perspective, agreed, Martin?

I don't see a problem with it.  I prefer tests that don't depend
on system headers to avoid these kinds of issues.

That said, it shouldn't matter if the declaration of a built-in
function has the nonnull attribute.  As long as the built-in has
one and isn't disabled GCC should use it.  I'd be curious to know
what is actually preventing the warning from triggering there.

Martin

> 
>>> diff --git a/gcc/testsuite/gcc.dg/Wnonnull.c b/gcc/testsuite/gcc.dg/Wnonnull.c
>>> index be89a5a755..a165baa99f 100644
>>> --- a/gcc/testsuite/gcc.dg/Wnonnull.c
>>> +++ b/gcc/testsuite/gcc.dg/Wnonnull.c
>>> @@ -2,7 +2,16 @@
>>>     { dg-do compile }
>>>     { dg-options "-O2 -Wall" } */
>>>
>>> +#ifndef __APPLE__
>>> #include <string.h>
>>> +#else
>>> +/* OSX headers do not mark up the nonnull elements yet.  */
>>> +# include <stddef.h>
>>> +extern size_t strlen (const char *__s)
>>> +		      __attribute ((pure)) __attribute ((nonnull (1)));
>>> +extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
>>> +		     size_t __n) __attribute ((nonnull (1, 2)));
>>> +#endif
>>
>> Perhaps use __SIZE_TYPE__ instead of #include <stddef.h>?
> 
> if we make the definitions generic, then this would avoid any header includes.
> 
> Iain
>
Iain Sandoe Oct. 20, 2019, 1:27 p.m. UTC | #4
Martin Sebor <msebor@gmail.com> wrote:

> On 10/19/19 2:56 AM, Iain Sandoe wrote:
>> Andreas Schwab <schwab@linux-m68k.org> wrote:
>>> On Okt 19 2019, Iain Sandoe <iain@sandoe.co.uk> wrote:
>>> 
>>>> This test has failed always on Darwin, because Darwin does not mark
>>>> entries in string.h with nonnull attributes.  Since the purpose of the test
>>>> is to check that the warnings are issued for an inlined function, not that
>>>> the target headers are marked up, we can provide locally marked up
>>>> function declarations for Darwin.
>>> 
>>> If the test depends on the non-std declarations, then it should use them
>>> everywhere.
>> from my perspective, agreed, Martin?
> 
> I don't see a problem with it.  I prefer tests that don't depend
> on system headers to avoid these kinds of issues

We can do that anyway then, - I can just adjust the current code tor remove the
special-casing, and to use __SIZE_TYPE__ instead of size_t everywhere, OK?

> That said, it shouldn't matter if the declaration of a built-in
> function has the nonnull attribute.  As long as the built-in has
> one and isn't disabled GCC should use it.  I'd be curious to know
> what is actually preventing the warning from triggering there.

This is secondary problem, the Darwin header implementation expands th
 memcpy to:
__builtin___memcpy_chk (dst, src,  __builtin_object_size (dst),  len);

Which, obviously, isn’t doing what you expect.

So, this does suggest that
a) for the test to be well-controlled, we need to avoid system headers
b) there might be some other builtins to check.

(but if we restrict the discussion to my understood purpose of the Wnonnull
 test, only (a) matters and that’s what’s proposed above).

thanks.
Iain
Martin Sebor Oct. 21, 2019, 1:16 p.m. UTC | #5
On 10/20/2019 07:27 AM, Iain Sandoe wrote:
> Martin Sebor <msebor@gmail.com> wrote:
> 
>> On 10/19/19 2:56 AM, Iain Sandoe wrote:
>>> Andreas Schwab <schwab@linux-m68k.org> wrote:
>>>> On Okt 19 2019, Iain Sandoe <iain@sandoe.co.uk> wrote:
>>>>
>>>>> This test has failed always on Darwin, because Darwin does not mark
>>>>> entries in string.h with nonnull attributes.  Since the purpose of the test
>>>>> is to check that the warnings are issued for an inlined function, not that
>>>>> the target headers are marked up, we can provide locally marked up
>>>>> function declarations for Darwin.
>>>>
>>>> If the test depends on the non-std declarations, then it should use them
>>>> everywhere.
>>> from my perspective, agreed, Martin?
>>
>> I don't see a problem with it.  I prefer tests that don't depend
>> on system headers to avoid these kinds of issues
> 
> We can do that anyway then, - I can just adjust the current code tor remove the
> special-casing, and to use __SIZE_TYPE__ instead of size_t everywhere, OK?

Sure.

> 
>> That said, it shouldn't matter if the declaration of a built-in
>> function has the nonnull attribute.  As long as the built-in has
>> one and isn't disabled GCC should use it.  I'd be curious to know
>> what is actually preventing the warning from triggering there.
> 
> This is secondary problem, the Darwin header implementation expands th
>   memcpy to:
> __builtin___memcpy_chk (dst, src,  __builtin_object_size (dst),  len);
> 
> Which, obviously, isn’t doing what you expect.

That suggests -Wno-system-headers might be getting in the way of
the warning.  I thought most middel-end warnings coped with it
but maybe not this one.  But getting rid of the include does
sound like the right way to deal with the test failing.

Thanks
Martin

> 
> So, this does suggest that
> a) for the test to be well-controlled, we need to avoid system headers
> b) there might be some other builtins to check.
> 
> (but if we restrict the discussion to my understood purpose of the Wnonnull
>   test, only (a) matters and that’s what’s proposed above).
> 
> thanks.
> Iain
> 
>
Iain Sandoe Oct. 21, 2019, 1:31 p.m. UTC | #6
Hi Martin,

Martin Sebor <msebor@gmail.com> wrote:

> On 10/20/2019 07:27 AM, Iain Sandoe wrote:
>> Martin Sebor <msebor@gmail.com> wrote:
>>> On 10/19/19 2:56 AM, Iain Sandoe wrote:
>>>> Andreas Schwab <schwab@linux-m68k.org> wrote:
>>>>> On Okt 19 2019, Iain Sandoe <iain@sandoe.co.uk> wrote:
>>>>> 
>>>>>> This test has failed always on Darwin, because Darwin does not mark
>>>>>> entries in string.h with nonnull attributes.  Since the purpose of the test
>>>>>> is to check that the warnings are issued for an inlined function, not that
>>>>>> the target headers are marked up, we can provide locally marked up
>>>>>> function declarations for Darwin.
>>>>> 
>>>>> If the test depends on the non-std declarations, then it should use them
>>>>> everywhere.
>>>> from my perspective, agreed, Martin?
>>> 
>>> I don't see a problem with it.  I prefer tests that don't depend
>>> on system headers to avoid these kinds of issues
>> We can do that anyway then, - I can just adjust the current code tor remove the
>> special-casing, and to use __SIZE_TYPE__ instead of size_t everywhere, OK?
> 
> Sure.

Will try to get to it later today.
(also to backport to 9 and 8 if that’s OK, IIRC the test was introduced in 8.x - slowly
 catching up on long-standing Darwin test fails).

>>> That said, it shouldn't matter if the declaration of a built-in
>>> function has the nonnull attribute.  As long as the built-in has
>>> one and isn't disabled GCC should use it.  I'd be curious to know
>>> what is actually preventing the warning from triggering there.
>> This is secondary problem, the Darwin header implementation expands th
>>  memcpy to:
>> __builtin___memcpy_chk (dst, src,  __builtin_object_size (dst),  len);
>> Which, obviously, isn’t doing what you expect.
> 
> That suggests -Wno-system-headers might be getting in the way of
> the warning.  I thought most middel-end warnings coped with it
> but maybe not this one.  But getting rid of the include does
> sound like the right way to deal with the test failing.

In this case it seems like the Darwin headers are applying _FORTIFY_SOURCE
(at least to the strings.h) automatically when the compiler is __GNUC__.  It might
be yet more fallout from the various __has_xxx that we don’t yet support.  In any
case, a separate issue I think.

thanks
Iain

> 
> Thanks
> Martin
> 
>> So, this does suggest that
>> a) for the test to be well-controlled, we need to avoid system headers
>> b) there might be some other builtins to check.
>> (but if we restrict the discussion to my understood purpose of the Wnonnull
>>  test, only (a) matters and that’s what’s proposed above).
>> thanks.
>> Iain
Iain Sandoe Oct. 22, 2019, 3:48 a.m. UTC | #7
Iain Sandoe <iain@sandoe.co.uk> wrote:

> Martin Sebor <msebor@gmail.com> wrote:
> 
>> On 10/20/2019 07:27 AM, Iain Sandoe wrote:
>>> Martin Sebor <msebor@gmail.com> wrote:
>>>> On 10/19/19 2:56 AM, Iain Sandoe wrote:
>>>>> Andreas Schwab <schwab@linux-m68k.org> wrote:
>>>>>> On Okt 19 2019, Iain Sandoe <iain@sandoe.co.uk> wrote:
>>>>>> 
>>>>>>> This test has failed always on Darwin, because Darwin does not mark
>>>>>>> entries in string.h with nonnull attributes.  Since the purpose of the test
>>>>>>> is to check that the warnings are issued for an inlined function, not that
>>>>>>> the target headers are marked up, we can provide locally marked up
>>>>>>> function declarations for Darwin.
>>>>>> 
>>>>>> If the test depends on the non-std declarations, then it should use them
>>>>>> everywhere.
>>>>> from my perspective, agreed, Martin?
>>>> 
>>>> I don't see a problem with it.  I prefer tests that don't depend
>>>> on system headers to avoid these kinds of issues
>>> We can do that anyway then, - I can just adjust the current code tor remove the
>>> special-casing, and to use __SIZE_TYPE__ instead of size_t everywhere, OK?
>> 
>> Sure.
> 
> Will try to get to it later today.

This is what I committed (will sort out backports, in due course).
tested on x86_64-linux-gnu and x86-64-darwin16.
thanks
Iain

===

[testsuite] Make the Wnonnull independent of system headers.

To avoid the result of this test depending on the implementation of
the system 'string.h', provide prototypes for the two functions used
in the test.

gcc/testsuite/ChangeLog:

2019-10-22  Iain Sandoe  <iain@sandoe.co.uk>

	* gcc.dg/Wnonnull.c: Provide prototypes for strlen and memcpy.
	Use __SIZE_TYPE__ instead of size_t.


diff --git a/gcc/testsuite/gcc.dg/Wnonnull.c b/gcc/testsuite/gcc.dg/Wnonnull.c
index a165baa99f..0ed06aabe6 100644
--- a/gcc/testsuite/gcc.dg/Wnonnull.c
+++ b/gcc/testsuite/gcc.dg/Wnonnull.c
@@ -2,16 +2,10 @@
    { dg-do compile }
    { dg-options "-O2 -Wall" } */
 
-#ifndef __APPLE__
-#include <string.h>
-#else
-/* OSX headers do not mark up the nonnull elements yet.  */
-# include <stddef.h>
-extern size_t strlen (const char *__s)
-		      __attribute ((pure)) __attribute ((nonnull (1)));
+extern __SIZE_TYPE__ strlen (const char *__s)
+			     __attribute ((pure)) __attribute ((nonnull (1)));
 extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
-		     size_t __n) __attribute ((nonnull (1, 2)));
-#endif
+		     __SIZE_TYPE__ __n) __attribute ((nonnull (1, 2)));
 
 char buf[100];
 
@@ -23,9 +17,9 @@ struct Test
 
 __attribute ((nonnull (1, 2)))
 inline char*
-my_strcpy (char *restrict dst, const char *restrict src, size_t size)
+my_strcpy (char *restrict dst, const char *restrict src, __SIZE_TYPE__ size)
 {
-  size_t len = strlen (src);        /* { dg-warning "argument 1 null where non-null expected" } */
+  __SIZE_TYPE__ len = strlen (src); /* { dg-warning "argument 1 null where non-null expected" } */
   if (len < size)
     memcpy (dst, src, len + 1);     /* { dg-warning "argument 2 null where non-null expected" } */
   else
Iain Sandoe Oct. 30, 2019, 9:01 p.m. UTC | #8
Iain Sandoe <iain@sandoe.co.uk> wrote:

> Iain Sandoe <iain@sandoe.co.uk> wrote:
> 
>> Martin Sebor <msebor@gmail.com> wrote:
>> 
>>> On 10/20/2019 07:27 AM, Iain Sandoe wrote:
>>>> Martin Sebor <msebor@gmail.com> wrote:
>>>>> On 10/19/19 2:56 AM, Iain Sandoe wrote:
>>>>>> Andreas Schwab <schwab@linux-m68k.org> wrote:
>>>>>>> On Okt 19 2019, Iain Sandoe <iain@sandoe.co.uk> wrote:
>>>>>>> 
>>>>>>>> This test has failed always on Darwin, because Darwin does not mark
>>>>>>>> entries in string.h with nonnull attributes.  Since the purpose of the test
>>>>>>>> is to check that the warnings are issued for an inlined function, not that
>>>>>>>> the target headers are marked up, we can provide locally marked up
>>>>>>>> function declarations for Darwin.
>>>>>>> 
>>>>>>> If the test depends on the non-std declarations, then it should use them
>>>>>>> everywhere.
>>>>>> from my perspective, agreed, Martin?
>>>>> 
>>>>> I don't see a problem with it.  I prefer tests that don't depend
>>>>> on system headers to avoid these kinds of issues
>>>> We can do that anyway then, - I can just adjust the current code tor remove the
>>>> special-casing, and to use __SIZE_TYPE__ instead of size_t everywhere, OK?
>>> 
>>> Sure.
>> 
>> Will try to get to it later today.
> 
> This is what I committed (will sort out backports, in due course).

backported to gcc-9 as r277647 (still needed on gcc-8).
Iain
Iain Sandoe Oct. 31, 2019, 8:14 p.m. UTC | #9
Iain Sandoe <iain@sandoe.co.uk> wrote:

> Iain Sandoe <iain@sandoe.co.uk> wrote:
> 
>> Iain Sandoe <iain@sandoe.co.uk> wrote:
>> 
>>> Martin Sebor <msebor@gmail.com> wrote:
>>> 
>>>> On 10/20/2019 07:27 AM, Iain Sandoe wrote:
>>>>> Martin Sebor <msebor@gmail.com> wrote:
>>>>>> On 10/19/19 2:56 AM, Iain Sandoe wrote:
>>>>>>> Andreas Schwab <schwab@linux-m68k.org> wrote:
>>>>>>>> On Okt 19 2019, Iain Sandoe <iain@sandoe.co.uk> wrote:
>>>>>>>> 
>>>>>>>>> This test has failed always on Darwin, because Darwin does not mark
>>>>>>>>> entries in string.h with nonnull attributes.  Since the purpose of the test
>>>>>>>>> is to check that the warnings are issued for an inlined function, not that
>>>>>>>>> the target headers are marked up, we can provide locally marked up
>>>>>>>>> function declarations for Darwin.
>>>>>>>> 
>>>>>>>> If the test depends on the non-std declarations, then it should use them
>>>>>>>> everywhere.
>>>>>>> from my perspective, agreed, Martin?
>>>>>> 
>>>>>> I don't see a problem with it.  I prefer tests that don't depend
>>>>>> on system headers to avoid these kinds of issues
>>>>> We can do that anyway then, - I can just adjust the current code tor remove the
>>>>> special-casing, and to use __SIZE_TYPE__ instead of size_t everywhere, OK?
>>>> 
>>>> Sure.
>>> 
>>> Will try to get to it later today.
>> 
>> This is what I committed (will sort out backports, in due course).
> 
> backported to gcc-9 as r277647 (still needed on gcc-8).

backported to gcc-8 as r277696.

I think that’s all that’s needed here, 
Iain

Patch
diff mbox series

diff --git a/gcc/testsuite/gcc.dg/Wnonnull.c b/gcc/testsuite/gcc.dg/Wnonnull.c
index be89a5a755..a165baa99f 100644
--- a/gcc/testsuite/gcc.dg/Wnonnull.c
+++ b/gcc/testsuite/gcc.dg/Wnonnull.c
@@ -2,7 +2,16 @@ 
    { dg-do compile }
    { dg-options "-O2 -Wall" } */
 
+#ifndef __APPLE__
 #include <string.h>
+#else
+/* OSX headers do not mark up the nonnull elements yet.  */
+# include <stddef.h>
+extern size_t strlen (const char *__s)
+		      __attribute ((pure)) __attribute ((nonnull (1)));
+extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
+		     size_t __n) __attribute ((nonnull (1, 2)));
+#endif
 
 char buf[100];