| Message ID | 20241004115447.3906620-1-jwakely@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | libstdc++: Test 17_intro/names.cc with -D_FORTIFY_SOURCE=2 [PR116210] | expand |
On Fri, Oct 04, 2024 at 12:52:11PM +0100, Jonathan Wakely wrote: > This doesn't really belong in our testsuite, because the sole purpose of > the new test is to find bugs in the Glibc wrappers (like the one linked > below). But maybe it's a kindness to do it in our testsuite, because we > already have this test in place, and one Glibc bug was already found > thanks to Sam running the existing test with _FORTIFY_SOURCE defined. > > Should we do this? I think so. While those bugs are glibc bugs, libstdc++ uses libc headers and so if they have namespace cleanness issues, so does libstdc++. > Add a new testcase that repeats 17_intro/names.cc but with > _FORTIFY_SOURCE defined, to find problems in Glibc fortify wrappers like > https://sourceware.org/bugzilla/show_bug.cgi?id=32052 (which is fixed > now). > > libstdc++-v3/ChangeLog: > > PR libstdc++/116210 > * testsuite/17_intro/names.cc (sz): Undef for versions of Glibc > that use it in the fortify wrappers. > * testsuite/17_intro/names_fortify.cc: New test. Jakub
On Fri, 4 Oct 2024 at 13:28, Jakub Jelinek <jakub@redhat.com> wrote: > > On Fri, Oct 04, 2024 at 12:52:11PM +0100, Jonathan Wakely wrote: > > This doesn't really belong in our testsuite, because the sole purpose of > > the new test is to find bugs in the Glibc wrappers (like the one linked > > below). But maybe it's a kindness to do it in our testsuite, because we > > already have this test in place, and one Glibc bug was already found > > thanks to Sam running the existing test with _FORTIFY_SOURCE defined. > > > > Should we do this? > > I think so. While those bugs are glibc bugs, libstdc++ uses libc headers > and so if they have namespace cleanness issues, so does libstdc++. Yeah, we have lots of #undef in that test to deal with libc headers that we can't change, but for Glibc we know we can fix problems much more easily than for e.g. proprietary UNIX headers. > > > Add a new testcase that repeats 17_intro/names.cc but with > > _FORTIFY_SOURCE defined, to find problems in Glibc fortify wrappers like > > https://sourceware.org/bugzilla/show_bug.cgi?id=32052 (which is fixed > > now). > > > > libstdc++-v3/ChangeLog: > > > > PR libstdc++/116210 > > * testsuite/17_intro/names.cc (sz): Undef for versions of Glibc > > that use it in the fortify wrappers. > > * testsuite/17_intro/names_fortify.cc: New test. > > Jakub >
On 2024-10-04 07:52, Jonathan Wakely wrote: > This doesn't really belong in our testsuite, because the sole purpose of > the new test is to find bugs in the Glibc wrappers (like the one linked > below). But maybe it's a kindness to do it in our testsuite, because we > already have this test in place, and one Glibc bug was already found > thanks to Sam running the existing test with _FORTIFY_SOURCE defined. > > Should we do this? > > -- >8 -- > > Add a new testcase that repeats 17_intro/names.cc but with > _FORTIFY_SOURCE defined, to find problems in Glibc fortify wrappers like > https://sourceware.org/bugzilla/show_bug.cgi?id=32052 (which is fixed > now). > > libstdc++-v3/ChangeLog: > > PR libstdc++/116210 > * testsuite/17_intro/names.cc (sz): Undef for versions of Glibc > that use it in the fortify wrappers. > * testsuite/17_intro/names_fortify.cc: New test. > --- > libstdc++-v3/testsuite/17_intro/names.cc | 7 +++++++ > libstdc++-v3/testsuite/17_intro/names_fortify.cc | 6 ++++++ > 2 files changed, 13 insertions(+) > create mode 100644 libstdc++-v3/testsuite/17_intro/names_fortify.cc > > diff --git a/libstdc++-v3/testsuite/17_intro/names.cc b/libstdc++-v3/testsuite/17_intro/names.cc > index 6b9a3639aad..bbf45b93dee 100644 > --- a/libstdc++-v3/testsuite/17_intro/names.cc > +++ b/libstdc++-v3/testsuite/17_intro/names.cc > @@ -377,4 +377,11 @@ > #undef y > #endif > > +#if defined __GLIBC_PREREQ && defined _FORTIFY_SOURCE > +# if __GLIBC_PREREQ(2,35) && ! __GLIBC_PREREQ(2,41) > +// https://sourceware.org/bugzilla/show_bug.cgi?id=32052 > +# undef sz > +# endif > +#endif We've backported the fix to stable branches, so the version check isn't really that reliable. Sid
On 2024-10-04 10:03, Siddhesh Poyarekar wrote: >> diff --git a/libstdc++-v3/testsuite/17_intro/names.cc >> b/libstdc++-v3/testsuite/17_intro/names.cc >> index 6b9a3639aad..bbf45b93dee 100644 >> --- a/libstdc++-v3/testsuite/17_intro/names.cc >> +++ b/libstdc++-v3/testsuite/17_intro/names.cc >> @@ -377,4 +377,11 @@ >> #undef y >> #endif >> +#if defined __GLIBC_PREREQ && defined _FORTIFY_SOURCE >> +# if __GLIBC_PREREQ(2,35) && ! __GLIBC_PREREQ(2,41) >> +// https://sourceware.org/bugzilla/show_bug.cgi?id=32052 >> +# undef sz >> +# endif >> +#endif > > We've backported the fix to stable branches, so the version check isn't > really that reliable. I suppose if you really want to test this in libstdc++, you could do (untested) something like: #if !defined __GLIBC_PREREQ || !defined _FORTIFY_SOURCE || !defined sz # define sz ( #endif Sid
On Fri, Oct 04, 2024 at 10:03:36AM -0400, Siddhesh Poyarekar wrote: > > Add a new testcase that repeats 17_intro/names.cc but with > > _FORTIFY_SOURCE defined, to find problems in Glibc fortify wrappers like > > https://sourceware.org/bugzilla/show_bug.cgi?id=32052 (which is fixed > > now). > > > > libstdc++-v3/ChangeLog: > > > > PR libstdc++/116210 > > * testsuite/17_intro/names.cc (sz): Undef for versions of Glibc > > that use it in the fortify wrappers. > > * testsuite/17_intro/names_fortify.cc: New test. > > --- > > libstdc++-v3/testsuite/17_intro/names.cc | 7 +++++++ > > libstdc++-v3/testsuite/17_intro/names_fortify.cc | 6 ++++++ > > 2 files changed, 13 insertions(+) > > create mode 100644 libstdc++-v3/testsuite/17_intro/names_fortify.cc > > > > diff --git a/libstdc++-v3/testsuite/17_intro/names.cc b/libstdc++-v3/testsuite/17_intro/names.cc > > index 6b9a3639aad..bbf45b93dee 100644 > > --- a/libstdc++-v3/testsuite/17_intro/names.cc > > +++ b/libstdc++-v3/testsuite/17_intro/names.cc > > @@ -377,4 +377,11 @@ > > #undef y > > #endif > > +#if defined __GLIBC_PREREQ && defined _FORTIFY_SOURCE > > +# if __GLIBC_PREREQ(2,35) && ! __GLIBC_PREREQ(2,41) > > +// https://sourceware.org/bugzilla/show_bug.cgi?id=32052 > > +# undef sz > > +# endif > > +#endif > > We've backported the fix to stable branches, so the version check isn't > really that reliable. That doesn't matter that much. The worst that happens is that with those older fixed glibc versions the testing will not test that symbol. What is more important is that it is checked on the latest glibc, so when people test gcc with that version, they'll notice if it regresses. Jakub
On Fri, 4 Oct 2024 at 15:05, Siddhesh Poyarekar <siddhesh@gotplt.org> wrote: > > On 2024-10-04 07:52, Jonathan Wakely wrote: > > This doesn't really belong in our testsuite, because the sole purpose of > > the new test is to find bugs in the Glibc wrappers (like the one linked > > below). But maybe it's a kindness to do it in our testsuite, because we > > already have this test in place, and one Glibc bug was already found > > thanks to Sam running the existing test with _FORTIFY_SOURCE defined. > > > > Should we do this? > > > > -- >8 -- > > > > Add a new testcase that repeats 17_intro/names.cc but with > > _FORTIFY_SOURCE defined, to find problems in Glibc fortify wrappers like > > https://sourceware.org/bugzilla/show_bug.cgi?id=32052 (which is fixed > > now). > > > > libstdc++-v3/ChangeLog: > > > > PR libstdc++/116210 > > * testsuite/17_intro/names.cc (sz): Undef for versions of Glibc > > that use it in the fortify wrappers. > > * testsuite/17_intro/names_fortify.cc: New test. > > --- > > libstdc++-v3/testsuite/17_intro/names.cc | 7 +++++++ > > libstdc++-v3/testsuite/17_intro/names_fortify.cc | 6 ++++++ > > 2 files changed, 13 insertions(+) > > create mode 100644 libstdc++-v3/testsuite/17_intro/names_fortify.cc > > > > diff --git a/libstdc++-v3/testsuite/17_intro/names.cc b/libstdc++-v3/testsuite/17_intro/names.cc > > index 6b9a3639aad..bbf45b93dee 100644 > > --- a/libstdc++-v3/testsuite/17_intro/names.cc > > +++ b/libstdc++-v3/testsuite/17_intro/names.cc > > @@ -377,4 +377,11 @@ > > #undef y > > #endif > > > > +#if defined __GLIBC_PREREQ && defined _FORTIFY_SOURCE > > +# if __GLIBC_PREREQ(2,35) && ! __GLIBC_PREREQ(2,41) > > +// https://sourceware.org/bugzilla/show_bug.cgi?id=32052 > > +# undef sz > > +# endif > > +#endif > > We've backported the fix to stable branches, so the version check isn't > really that reliable. Yeah, but it doesn't matter if we #undef sz on some Glibc systems that don't actually have the bug.
Jakub Jelinek <jakub@redhat.com> writes: > On Fri, Oct 04, 2024 at 12:52:11PM +0100, Jonathan Wakely wrote: >> This doesn't really belong in our testsuite, because the sole purpose of >> the new test is to find bugs in the Glibc wrappers (like the one linked >> below). But maybe it's a kindness to do it in our testsuite, because we >> already have this test in place, and one Glibc bug was already found >> thanks to Sam running the existing test with _FORTIFY_SOURCE defined. >> >> Should we do this? > > I think so. While those bugs are glibc bugs, libstdc++ uses libc headers > and so if they have namespace cleanness issues, so does libstdc++. > >> Add a new testcase that repeats 17_intro/names.cc but with >> _FORTIFY_SOURCE defined, to find problems in Glibc fortify wrappers like >> https://sourceware.org/bugzilla/show_bug.cgi?id=32052 (which is fixed >> now). I think yes as well -- we've had a lot of discussions in glibc about getting to a place where we have tests to check the usability of headers (there's some for this specific namespace problem but there's some bigger stuff wrt parsing from Clang and so on) but we're not there yet. This feels like a cheap way of catching issues, and the fact that nobody noticed between 2.35 and 2.40 (i.e. ~3 years) means it's worthwhile IMO. >> >> libstdc++-v3/ChangeLog: >> >> PR libstdc++/116210 >> * testsuite/17_intro/names.cc (sz): Undef for versions of Glibc >> that use it in the fortify wrappers. >> * testsuite/17_intro/names_fortify.cc: New test. > > Jakub thanks, sam
diff --git a/libstdc++-v3/testsuite/17_intro/names.cc b/libstdc++-v3/testsuite/17_intro/names.cc index 6b9a3639aad..bbf45b93dee 100644 --- a/libstdc++-v3/testsuite/17_intro/names.cc +++ b/libstdc++-v3/testsuite/17_intro/names.cc @@ -377,4 +377,11 @@ #undef y #endif +#if defined __GLIBC_PREREQ && defined _FORTIFY_SOURCE +# if __GLIBC_PREREQ(2,35) && ! __GLIBC_PREREQ(2,41) +// https://sourceware.org/bugzilla/show_bug.cgi?id=32052 +# undef sz +# endif +#endif + #include <bits/stdc++.h> diff --git a/libstdc++-v3/testsuite/17_intro/names_fortify.cc b/libstdc++-v3/testsuite/17_intro/names_fortify.cc new file mode 100644 index 00000000000..c975412074b --- /dev/null +++ b/libstdc++-v3/testsuite/17_intro/names_fortify.cc @@ -0,0 +1,6 @@ +// { dg-do compile { target *-*-linux* } } +// { dg-add-options no_pch } + +#define _FORTIFY_SOURCE 2 +// Now we can define the macros to poison uses of non-reserved names: +#include "names.cc"