| Message ID | 20220805201358.2348750-1-sam@gentoo.org |
|---|---|
| State | New |
| Headers | show |
| Series | stdlib: tests: don't double-define _FORTIFY_SOURCE | expand |
On 2022-08-05 16:13, Sam James via Libc-alpha wrote: > If using -D_FORITFY_SOURCE=3 (in my case, I've patched GCC to add > =3 instead of =2 (we've done =2 for years in Gentoo)), building > glibc tests will fail on testmb like: > ``` > <command-line>: error: "_FORTIFY_SOURCE" redefined [-Werror] > <built-in>: note: this is the location of the previous definition > cc1: all warnings being treated as errors > make[2]: *** [../o-iterator.mk:9: /var/tmp/portage/sys-libs/glibc-2.36/work/build-x86-x86_64-pc-linux-gnu-nptl/stdlib/testmb.o] Error 1 > make[2]: *** Waiting for unfinished jobs.... > ``` > > It's just because we're always setting -D_FORTIFY_SOURCE=2 > rather than unsetting it first. If F_S is already 2, it's harmless, > but if it's another value (say, 1, or 3), the compiler will bawk. > > (I'm not aware of a reason this couldn't be tested with =3, > but the toolchain support is limited for that (too new), and we want > to run the tests everywhere possible.) > > Signed-off-by: Sam James <sam@gentoo.org> > --- > stdlib/Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/stdlib/Makefile b/stdlib/Makefile > index f7b25c1981..d8b59022cc 100644 > --- a/stdlib/Makefile > +++ b/stdlib/Makefile > @@ -380,7 +380,7 @@ CFLAGS-tst-qsort.c += $(stack-align-test-flags) > CFLAGS-tst-makecontext.c += -funwind-tables > CFLAGS-tst-makecontext2.c += $(stack-align-test-flags) > > -CFLAGS-testmb.c += -D_FORTIFY_SOURCE=2 -Wall -Werror > +CFLAGS-testmb.c += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Werror I think we'll be better off with -D_FORTIFY_SOURCE=3 here since the intent is to test the fortified versions of the mb functions and =3 will give the maximum coverage. The downside is that it will emit a warning when building with older gcc (because _FORTIFY_SOURCE=3 is not supported there and protection will downgrade to _FORTIFY_SOURCE=2) so the -Werror will need to go away and maybe even need -Wno-error. Alternatively, some magic here to determine the maximum fortification level wouldn't hurt, but I won't gate your patch on that :) I can work on that bit. Thanks, Sid
> On 8 Aug 2022, at 14:26, Siddhesh Poyarekar <siddhesh@gotplt.org> wrote: > > On 2022-08-05 16:13, Sam James via Libc-alpha wrote: >> If using -D_FORITFY_SOURCE=3 (in my case, I've patched GCC to add >> =3 instead of =2 (we've done =2 for years in Gentoo)), building >> glibc tests will fail on testmb like: >> ``` >> <command-line>: error: "_FORTIFY_SOURCE" redefined [-Werror] >> <built-in>: note: this is the location of the previous definition >> cc1: all warnings being treated as errors >> make[2]: *** [../o-iterator.mk:9: /var/tmp/portage/sys-libs/glibc-2.36/work/build-x86-x86_64-pc-linux-gnu-nptl/stdlib/testmb.o] Error 1 >> make[2]: *** Waiting for unfinished jobs.... >> ``` >> It's just because we're always setting -D_FORTIFY_SOURCE=2 >> rather than unsetting it first. If F_S is already 2, it's harmless, >> but if it's another value (say, 1, or 3), the compiler will bawk. >> (I'm not aware of a reason this couldn't be tested with =3, >> but the toolchain support is limited for that (too new), and we want >> to run the tests everywhere possible.) >> Signed-off-by: Sam James <sam@gentoo.org> >> --- >> stdlib/Makefile | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> diff --git a/stdlib/Makefile b/stdlib/Makefile >> index f7b25c1981..d8b59022cc 100644 >> --- a/stdlib/Makefile >> +++ b/stdlib/Makefile >> @@ -380,7 +380,7 @@ CFLAGS-tst-qsort.c += $(stack-align-test-flags) >> CFLAGS-tst-makecontext.c += -funwind-tables >> CFLAGS-tst-makecontext2.c += $(stack-align-test-flags) >> -CFLAGS-testmb.c += -D_FORTIFY_SOURCE=2 -Wall -Werror >> +CFLAGS-testmb.c += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Werror > > I think we'll be better off with -D_FORTIFY_SOURCE=3 here since the intent is to test the fortified versions of the mb functions and =3 will give the maximum coverage. The downside is that it will emit a warning when building with older gcc (because _FORTIFY_SOURCE=3 is not supported there and protection will downgrade to _FORTIFY_SOURCE=2) so the -Werror will need to go away and maybe even need -Wno-error. > This is a fair point, although I now see we've actually got libc_cv_predef_fortify_source which sets CPPUNDEFS for exactly this sort of problem anyway. I don't see the warning with gcc-11 + -Werror + F_S=3 on a test program. I can build some older GCCs as I should probably keep them around anyway though. > Alternatively, some magic here to determine the maximum fortification level wouldn't hurt, but I won't gate your patch on that :) I can work on that bit. I started looking at that and I'm not sure there's a point. includes/features.h downgrades us appropriately. I think we can unconditionally set F_S=3 if I'm right about GCC not caring, as all the logic is on the glibc side, right? We can always split this into two if you want: 1. The original commit (I can convert it to use libc_cv_predef_fortify_source's result) & backport it to 2.36 2. Another to crank to =3 and don't backport it in case I'm missing something. > > Thanks, > Sid best, sam
On 2022-08-08 18:30, Sam James wrote: > This is a fair point, although I now see we've actually got libc_cv_predef_fortify_source > which sets CPPUNDEFS for exactly this sort of problem anyway. > > I don't see the warning with gcc-11 + -Werror + F_S=3 on a test program. I can build some > older GCCs as I should probably keep them around anyway though. Interesting, because you should have seen the warning about _FORTIFY_SOURCE=3 not being supported; I'm surprised that it doesn't fail due to that warning. >> Alternatively, some magic here to determine the maximum fortification level wouldn't hurt, but I won't gate your patch on that :) I can work on that bit. > > I started looking at that and I'm not sure there's a point. includes/features.h downgrades us appropriately. I think we can unconditionally > set F_S=3 if I'm right about GCC not caring, as all the logic is on the glibc side, right? It's the warning I'm thinking about avoiding. > > We can always split this into two if you want: > 1. The original commit (I can convert it to use libc_cv_predef_fortify_source's result) & backport it to 2.36 Sounds good to me. > 2. Another to crank to =3 and don't backport it in case I'm missing something. > Thanks, Sid
> On 10 Aug 2022, at 15:29, Siddhesh Poyarekar <siddhesh@gotplt.org> wrote: > > On 2022-08-08 18:30, Sam James wrote: >> This is a fair point, although I now see we've actually got libc_cv_predef_fortify_source >> which sets CPPUNDEFS for exactly this sort of problem anyway. >> I don't see the warning with gcc-11 + -Werror + F_S=3 on a test program. I can build some >> older GCCs as I should probably keep them around anyway though. > > Interesting, because you should have seen the warning about _FORTIFY_SOURCE=3 not being supported; I'm surprised that it doesn't fail due to that warning. ... and I see it now. I don't know what I did the other day. Oops. > >>> Alternatively, some magic here to determine the maximum fortification level wouldn't hurt, but I won't gate your patch on that :) I can work on that bit. >> I started looking at that and I'm not sure there's a point. includes/features.h downgrades us appropriately. I think we can unconditionally >> set F_S=3 if I'm right about GCC not caring, as all the logic is on the glibc side, right? > > It's the warning I'm thinking about avoiding. > >> We can always split this into two if you want: >> 1. The original commit (I can convert it to use libc_cv_predef_fortify_source's result) & backport it to 2.36 > > Sounds good to me. > >> 2. Another to crank to =3 and don't backport it in case I'm missing something. > Okay, sounds like a plan, cheers. I'll get on it. > Thanks, > Sid
> On 10 Aug 2022, at 16:06, Sam James via Libc-alpha <libc-alpha@sourceware.org> wrote: > > > >> On 10 Aug 2022, at 15:29, Siddhesh Poyarekar <siddhesh@gotplt.org> wrote: >> >> On 2022-08-08 18:30, Sam James wrote: >>> This is a fair point, although I now see we've actually got libc_cv_predef_fortify_source >>> which sets CPPUNDEFS for exactly this sort of problem anyway. >>> I don't see the warning with gcc-11 + -Werror + F_S=3 on a test program. I can build some >>> older GCCs as I should probably keep them around anyway though. >> >> Interesting, because you should have seen the warning about _FORTIFY_SOURCE=3 not being supported; I'm surprised that it doesn't fail due to that warning. > > ... and I see it now. I don't know what I did the other day. Oops. > >> >>>> Alternatively, some magic here to determine the maximum fortification level wouldn't hurt, but I won't gate your patch on that :) I can work on that bit. >>> I started looking at that and I'm not sure there's a point. includes/features.h downgrades us appropriately. I think we can unconditionally >>> set F_S=3 if I'm right about GCC not caring, as all the logic is on the glibc side, right? >> >> It's the warning I'm thinking about avoiding. >> >>> We can always split this into two if you want: >>> 1. The original commit (I can convert it to use libc_cv_predef_fortify_source's result) & backport it to 2.36 >> >> Sounds good to me. >> >>> 2. Another to crank to =3 and don't backport it in case I'm missing something. >> > > Okay, sounds like a plan, cheers. I'll get on it. > I ended up getting frustrated with some over engineering and got distracted with fixing up autoconf-archive's macro for it. Could you push the this one or do you want me to resend it? Cheers!
On 2023-02-02 13:21, Sam James wrote: > I ended up getting frustrated with some over engineering and got distracted with fixing up > autoconf-archive's macro for it. > > Could you push the this one or do you want me to resend it? Sorry I forgot to push it, done now. Sid
diff --git a/stdlib/Makefile b/stdlib/Makefile index f7b25c1981..d8b59022cc 100644 --- a/stdlib/Makefile +++ b/stdlib/Makefile @@ -380,7 +380,7 @@ CFLAGS-tst-qsort.c += $(stack-align-test-flags) CFLAGS-tst-makecontext.c += -funwind-tables CFLAGS-tst-makecontext2.c += $(stack-align-test-flags) -CFLAGS-testmb.c += -D_FORTIFY_SOURCE=2 -Wall -Werror +CFLAGS-testmb.c += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Werror # Run a test on the header files we use.
If using -D_FORITFY_SOURCE=3 (in my case, I've patched GCC to add =3 instead of =2 (we've done =2 for years in Gentoo)), building glibc tests will fail on testmb like: ``` <command-line>: error: "_FORTIFY_SOURCE" redefined [-Werror] <built-in>: note: this is the location of the previous definition cc1: all warnings being treated as errors make[2]: *** [../o-iterator.mk:9: /var/tmp/portage/sys-libs/glibc-2.36/work/build-x86-x86_64-pc-linux-gnu-nptl/stdlib/testmb.o] Error 1 make[2]: *** Waiting for unfinished jobs.... ``` It's just because we're always setting -D_FORTIFY_SOURCE=2 rather than unsetting it first. If F_S is already 2, it's harmless, but if it's another value (say, 1, or 3), the compiler will bawk. (I'm not aware of a reason this couldn't be tested with =3, but the toolchain support is limited for that (too new), and we want to run the tests everywhere possible.) Signed-off-by: Sam James <sam@gentoo.org> --- stdlib/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)