| Message ID | 1303203461-30776-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Apr 19, 2011 at 11:57 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > Work around a SPARC glibc bug which caused the epoll_create1 configure > test to wrongly claim that the function was present. Some versions of > SPARC glibc provided the function in the library but didn't declare > it in the include file; the result is that gcc warns about an implicit > declaration but a link succeeds. So we build the configure test with > -Werror to avoid the test passing but then a -Werror qemu build > failing. But then epoll would not be used. How about checking for the buggy glibc and doing something similar to how Solaris madvise() problem is handled in osdep.c? Both this and madvise() hack could be configure time checks, though.
On 19 April 2011 20:37, Blue Swirl <blauwirbel@gmail.com> wrote: > On Tue, Apr 19, 2011 at 11:57 AM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> Work around a SPARC glibc bug which caused the epoll_create1 configure >> test to wrongly claim that the function was present. Some versions of >> SPARC glibc provided the function in the library but didn't declare >> it in the include file; the result is that gcc warns about an implicit >> declaration but a link succeeds. So we build the configure test with >> -Werror to avoid the test passing but then a -Werror qemu build >> failing. > > But then epoll would not be used. I think that's fine -- on a system which isn't advertising epoll in its include files we shouldn't be trying to use it. It might be buggy, or not the same function at all, for instance. Anybody who actually cares about epoll can upgrade their libc :-) -- PMM
On Tue, Apr 19, 2011 at 10:48 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 April 2011 20:37, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Tue, Apr 19, 2011 at 11:57 AM, Peter Maydell >> <peter.maydell@linaro.org> wrote: >>> Work around a SPARC glibc bug which caused the epoll_create1 configure >>> test to wrongly claim that the function was present. Some versions of >>> SPARC glibc provided the function in the library but didn't declare >>> it in the include file; the result is that gcc warns about an implicit >>> declaration but a link succeeds. So we build the configure test with >>> -Werror to avoid the test passing but then a -Werror qemu build >>> failing. >> >> But then epoll would not be used. > > I think that's fine -- on a system which isn't advertising epoll > in its include files we shouldn't be trying to use it. It might > be buggy, or not the same function at all, for instance. > > Anybody who actually cares about epoll can upgrade their libc :-) Maybe epoll is not so interesting as madvise. But the check is not very specific, there could be some unrelated warning with the headers. How about checking in the compiled file for for example EPOLLIN, that should give a clear build failure if the header is missing?
On 19 April 2011 20:59, Blue Swirl <blauwirbel@gmail.com> wrote: > On Tue, Apr 19, 2011 at 10:48 PM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> On 19 April 2011 20:37, Blue Swirl <blauwirbel@gmail.com> wrote: >>> But then epoll would not be used. >> >> I think that's fine -- on a system which isn't advertising epoll >> in its include files we shouldn't be trying to use it. It might >> be buggy, or not the same function at all, for instance. >> >> Anybody who actually cares about epoll can upgrade their libc :-) > > Maybe epoll is not so interesting as madvise. Indeed, it isn't; the only reason we check for it in configure is so we know whether we can pass through the relevant syscalls for linux-user mode. it's a pretty recent innovation so any guest app using it should have a fallback path if it gets ENOSYS. Incidentally, note that this configure check is for epoll_create1(), not epoll_create(). [Some systems have only the former, so it has a separate configure check.] madvise() is used by qemu itself, so we care more there. > But the check is not very specific, there could be some unrelated > warning with the headers. The check isn't supposed to be very specific; the idea is that it does basically what the actual qemu source code does, ie just use the function in a program that's compiled with -Werror. If there's an unrelated problem with the header that produces a compile warning then we also want that to cause the test to fail, because that too will cause qemu compilation to fail later. I think that the ideal for configure tests is that they test for exactly the set of functionality used by the program itself; that's what I'm trying to do here. > How about checking in the compiled file for > for example EPOLLIN, that should give a clear build failure if the > header is missing? If the header was missing then that would already be causing a compilation failure. The issue with this particular version of SPARC glibc is that the header is present but doesn't declare the function. Looking for EPOLLIN isn't a useful test for "is epoll_create1() present and OK?" because: (1) EPOLLIN will be defined on systems which only have epoll_create(), so "is EPOLLIN present?" can be true when "is epoll_create1() OK?" is false (2) we don't ever actually use EPOLLIN, so checking for it could potentially cause us to not use epoll_create1() even if it was present and usable -- PMM
On Tue, Apr 19, 2011 at 11:16 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 April 2011 20:59, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Tue, Apr 19, 2011 at 10:48 PM, Peter Maydell >> <peter.maydell@linaro.org> wrote: >>> On 19 April 2011 20:37, Blue Swirl <blauwirbel@gmail.com> wrote: >>>> But then epoll would not be used. >>> >>> I think that's fine -- on a system which isn't advertising epoll >>> in its include files we shouldn't be trying to use it. It might >>> be buggy, or not the same function at all, for instance. >>> >>> Anybody who actually cares about epoll can upgrade their libc :-) >> >> Maybe epoll is not so interesting as madvise. > > Indeed, it isn't; the only reason we check for it in configure is > so we know whether we can pass through the relevant syscalls for > linux-user mode. it's a pretty recent innovation so any guest app > using it should have a fallback path if it gets ENOSYS. > Incidentally, note that this configure check is for epoll_create1(), > not epoll_create(). [Some systems have only the former, so it has > a separate configure check.] > > madvise() is used by qemu itself, so we care more there. > >> But the check is not very specific, there could be some unrelated >> warning with the headers. > > The check isn't supposed to be very specific; the idea is that it > does basically what the actual qemu source code does, ie just > use the function in a program that's compiled with -Werror. If there's > an unrelated problem with the header that produces a compile > warning then we also want that to cause the test to fail, because > that too will cause qemu compilation to fail later. > > I think that the ideal for configure tests is that they test > for exactly the set of functionality used by the program itself; > that's what I'm trying to do here. > >> How about checking in the compiled file for >> for example EPOLLIN, that should give a clear build failure if the >> header is missing? > > If the header was missing then that would already be causing a > compilation failure. The issue with this particular version of > SPARC glibc is that the header is present but doesn't declare the > function. > > Looking for EPOLLIN isn't a useful test for "is epoll_create1() > present and OK?" because: > (1) EPOLLIN will be defined on systems which only have epoll_create(), > so "is EPOLLIN present?" can be true when "is epoll_create1() OK?" is false > (2) we don't ever actually use EPOLLIN, so checking for it could > potentially cause us to not use epoll_create1() even if it was > present and usable Sorry, I just picked a define without much thought. A more specific one would be flags parameter of epoll_create1(), like EPOLL_CLOEXEC or EPOLL_NONBLOCK. We don't use them now since the target system call argument is passed untranslated to host, but that is actually not correct, since the bit definitions could be different. So checking for one of those should be OK.
On 19 April 2011 21:36, Blue Swirl <blauwirbel@gmail.com> wrote: > Sorry, I just picked a define without much thought. A more specific > one would be flags parameter of epoll_create1(), like EPOLL_CLOEXEC or > EPOLL_NONBLOCK. We don't use them now since the target system call > argument is passed untranslated to host, but that is actually not > correct, since the bit definitions could be different. So checking for > one of those should be OK. Unfortunately the header file on the system in question defines both EPOLL_CLOEXEC and EPOLL_NONBLOCK even though it doesn't prototype epoll_create1(). So this idea won't work. The bug we are effectively trying to work around is the one fixed by this libc patch: http://sourceware.org/ml/libc-alpha/2010-08/msg00128.html The only problem with the header is that it doesn't declare the function, so the only way to detect it is to do something that will fail if the function isn't declared, like compiling -Werror. -- PMM
On Wed, Apr 20, 2011 at 9:53 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 April 2011 21:36, Blue Swirl <blauwirbel@gmail.com> wrote: >> Sorry, I just picked a define without much thought. A more specific >> one would be flags parameter of epoll_create1(), like EPOLL_CLOEXEC or >> EPOLL_NONBLOCK. We don't use them now since the target system call >> argument is passed untranslated to host, but that is actually not >> correct, since the bit definitions could be different. So checking for >> one of those should be OK. > > Unfortunately the header file on the system in question defines > both EPOLL_CLOEXEC and EPOLL_NONBLOCK even though it doesn't > prototype epoll_create1(). So this idea won't work. > The bug we are effectively trying to work around is the one fixed > by this libc patch: > http://sourceware.org/ml/libc-alpha/2010-08/msg00128.html > > The only problem with the header is that it doesn't declare the > function, so the only way to detect it is to do something that > will fail if the function isn't declared, like compiling -Werror. This also fails without -Werror: #include <sys/epoll.h> int main(void) { epoll_create1; return 0; }
diff --git a/configure b/configure index da2da04..d9a8fdc 100755 --- a/configure +++ b/configure @@ -2229,7 +2229,12 @@ int main(void) return 0; } EOF -if compile_prog "$ARCH_CFLAGS" "" ; then +# We need to build this with -Werror to handle a bug in some +# SPARC glibc where the function is defined but not declared +# in the header file. Without -Werror gcc warns about an implicit +# declaration of the function but the link succeeds; however +# qemu itself will fail to build if it is compiled -Werror. +if compile_prog "$ARCH_CFLAGS -Werror" "" ; then epoll_create1=yes fi
Work around a SPARC glibc bug which caused the epoll_create1 configure test to wrongly claim that the function was present. Some versions of SPARC glibc provided the function in the library but didn't declare it in the include file; the result is that gcc warns about an implicit declaration but a link succeeds. So we build the configure test with -Werror to avoid the test passing but then a -Werror qemu build failing. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- configure | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)