| Message ID | 5DDF751A-C750-4CC7-8272-966E663534CF@sandoe.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | [Darwin,testsuite,committed] Fix Wnonnull on Darwin. | expand |
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.
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
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 >
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
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 > >
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 <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 <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 <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
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];