| Message ID | 1410486339-2001-1-git-send-email-maarten@treewalker.org |
|---|---|
| State | Superseded |
| Headers | show |
Dear Maarten ter Huurne, On Fri, 12 Sep 2014 03:45:39 +0200, Maarten ter Huurne wrote: > Signed-off-by: Maarten ter Huurne <maarten@treewalker.org> > --- > .../rpi-userland-003-disable-timer_t-typedef.patch | 22 ++++++++++++++++++++++ > .../rpi-userland-004-include-for-HZ.patch | 19 +++++++++++++++++++ > ...land-005-do-not-check-__USE_FILE_OFFSET64.patch | 16 ++++++++++++++++ > package/rpi-userland/rpi-userland-006-mode_t.patch | 13 +++++++++++++ > 4 files changed, 70 insertions(+) > create mode 100644 package/rpi-userland/rpi-userland-003-disable-timer_t-typedef.patch > create mode 100644 package/rpi-userland/rpi-userland-004-include-for-HZ.patch > create mode 100644 package/rpi-userland/rpi-userland-005-do-not-check-__USE_FILE_OFFSET64.patch > create mode 100644 package/rpi-userland/rpi-userland-006-mode_t.patch Thanks. The naming of the patch files as well as the descriptions are good, but each patch is missing a SoB line, which we require. See http://buildroot.org/downloads/manual/manual.html#_format_and_licensing_of_the_package_patches. > diff --git a/package/rpi-userland/rpi-userland-003-disable-timer_t-typedef.patch b/package/rpi-userland/rpi-userland-003-disable-timer_t-typedef.patch > new file mode 100644 > index 0000000..e51a93f > --- /dev/null > +++ b/package/rpi-userland/rpi-userland-003-disable-timer_t-typedef.patch > @@ -0,0 +1,22 @@ > +I don't know which platform is missing the timer_t definition, but it would > +be better to check for that platform specifically instead of explicitly > +excluding platforms that do have it. > + > +This fixes compilation with musl libc, which does define timer_t but does > +not define __timer_t. > + > +Note that _HAVE_TIMER_T suggests it would prevent breakage, but it is > +unrealistic to expect every application to set that macro. Not even all CMake > +files in the userland package itself set it... > + > +--- userland-f9e6f9f3def8dc18dc0092cd695ccf53b8ba3efe.org/interface/vcos/pthreads/vcos_platform.h 2014-08-05 19:43:53.000000000 +0200 > ++++ rpi-userland-f9e6f9f3def8dc18dc0092cd695ccf53b8ba3efe/interface/vcos/pthreads/vcos_platform.h 2014-09-11 21:04:36.418943953 +0200 > +@@ -120,7 +120,7 @@ > + > + #define VCOS_ONCE_INIT PTHREAD_ONCE_INIT > + > +-#if defined(__arm__) && !defined(_HAVE_TIMER_T) && !defined(ANDROID) > ++#if 0 > + typedef __timer_t timer_t; > + #endif > + typedef struct VCOS_TIMER_T I'm just not entirely convinced by this one: it should be the job of the rpi-userland build system to check for timer_t, and define HAVE_TIMER_T accordingly. It's definitely not the job of each and every application to set that macro, but really rpi-userland's build system. Also, could you submit those patches upstream? We don't like much to carry a lot of patches in Buildroot, so the more patches can be merged upstream, the better. Thanks a lot! Thomas
On Friday 12 September 2014 09:32:53 Thomas Petazzoni wrote: > Thanks. The naming of the patch files as well as the descriptions are > good, but each patch is missing a SoB line, which we require. See > http://buildroot.org/downloads/manual/manual.html#_format_and_licensing_of > _the_package_patches. I'll add those and resubmit. > > diff --git > > a/package/rpi-userland/rpi-userland-003-disable-timer_t-typedef.patch > > b/package/rpi-userland/rpi-userland-003-disable-timer_t-typedef.patch > > new file mode 100644 > > index 0000000..e51a93f > > --- /dev/null > > +++ > > b/package/rpi-userland/rpi-userland-003-disable-timer_t-typedef.patch > > @@ -0,0 +1,22 @@ > > +I don't know which platform is missing the timer_t definition, but it > > would +be better to check for that platform specifically instead of > > explicitly +excluding platforms that do have it. > > + > > +This fixes compilation with musl libc, which does define timer_t but > > does +not define __timer_t. > > + > > +Note that _HAVE_TIMER_T suggests it would prevent breakage, but it is > > +unrealistic to expect every application to set that macro. Not even all > > CMake +files in the userland package itself set it... > > + > > +--- > > userland-f9e6f9f3def8dc18dc0092cd695ccf53b8ba3efe.org/interface/vcos/pt > > hreads/vcos_platform.h 2014-08-05 19:43:53.000000000 +0200 ++++ > > rpi-userland-f9e6f9f3def8dc18dc0092cd695ccf53b8ba3efe/interface/vcos/pt > > hreads/vcos_platform.h 2014-09-11 21:04:36.418943953 +0200 +@@ -120,7 > > +120,7 @@ > > + > > + #define VCOS_ONCE_INIT PTHREAD_ONCE_INIT > > + > > +-#if defined(__arm__) && !defined(_HAVE_TIMER_T) && !defined(ANDROID) > > ++#if 0 > > + typedef __timer_t timer_t; > > + #endif > > + typedef struct VCOS_TIMER_T > > I'm just not entirely convinced by this one: it should be the job of > the rpi-userland build system to check for timer_t, and define > HAVE_TIMER_T accordingly. It's definitely not the job of each and every > application to set that macro, but really rpi-userland's build system. That's what you'd expect from the HAVE_* name, but there isn't a timer_h check in the rpi-userland build system. There is a hardcoded "CFLAGS+=- D_HAVE_TIMER_T" for a single application inside rpi-userland, the other applications don't define it. The reason I wrote "every application" is that this header can be indirectly included by applications using EGL: $ output/host/usr/bin/pkg-config --cflags egl -I/home/mth/pi/buildroot/output/host/usr/arm-pingux-linux- musleabihf/sysroot/usr/include/ -I/home/mth/pi/buildroot/output/host/usr/arm-pingux-linux- musleabihf/sysroot/usr/include/interface/vcos/pthreads/ -I/home/mth/pi/buildroot/output/host/usr/arm-pingux-linux- musleabihf/sysroot/usr/include/interface/vmcs_host/linux/ In theory -D_HAVE_TIMER_H could be added to Cflags in egl.pc, but that might interfere with application packages. At least in the context of Buildroot, where none of the supported libcs require this typedef, disabling this check is the safest approach, in my opinion. Also I wonder if there really is a platform on which this typedef helps: it would only help on a platform that does implement POSIX timers (it's an optional feature), but for some reason doesn't define timer_t. It just doesn't sound very likely... Looking at the header again, I see it doesn't actually use timer_t anywhere. In fact, nowhere in the entire rpi_userland package is timer_t used. So the whole check + typedef can just be removed. > Also, could you submit those patches upstream? We don't like much to > carry a lot of patches in Buildroot, so the more patches can be merged > upstream, the better. https://github.com/raspberrypi/userland/issues/202 https://github.com/raspberrypi/userland/pull/201 Bye, Maarten
Dear Maarten ter Huurne, On Fri, 12 Sep 2014 16:56:37 +0200, Maarten ter Huurne wrote: > That's what you'd expect from the HAVE_* name, but there isn't a timer_h > check in the rpi-userland build system. There is a hardcoded "CFLAGS+=- > D_HAVE_TIMER_T" for a single application inside rpi-userland, the other > applications don't define it. > > The reason I wrote "every application" is that this header can be indirectly > included by applications using EGL: > > $ output/host/usr/bin/pkg-config --cflags egl > -I/home/mth/pi/buildroot/output/host/usr/arm-pingux-linux- > musleabihf/sysroot/usr/include/ > -I/home/mth/pi/buildroot/output/host/usr/arm-pingux-linux- > musleabihf/sysroot/usr/include/interface/vcos/pthreads/ > -I/home/mth/pi/buildroot/output/host/usr/arm-pingux-linux- > musleabihf/sysroot/usr/include/interface/vmcs_host/linux/ > > In theory -D_HAVE_TIMER_H could be added to Cflags in egl.pc, but that might > interfere with application packages. At least in the context of Buildroot, > where none of the supported libcs require this typedef, disabling this check > is the safest approach, in my opinion. > > Also I wonder if there really is a platform on which this typedef helps: it > would only help on a platform that does implement POSIX timers (it's an > optional feature), but for some reason doesn't define timer_t. It just > doesn't sound very likely... > > Looking at the header again, I see it doesn't actually use timer_t anywhere. > In fact, nowhere in the entire rpi_userland package is timer_t used. So the > whole check + typedef can just be removed. Ok, thanks a lot for the explanation. Then just remove the definition, and submit the patch upstream. > > > Also, could you submit those patches upstream? We don't like much to > > carry a lot of patches in Buildroot, so the more patches can be merged > > upstream, the better. > > https://github.com/raspberrypi/userland/issues/202 > https://github.com/raspberrypi/userland/pull/201 Great! Thomas
diff --git a/package/rpi-userland/rpi-userland-003-disable-timer_t-typedef.patch b/package/rpi-userland/rpi-userland-003-disable-timer_t-typedef.patch new file mode 100644 index 0000000..e51a93f --- /dev/null +++ b/package/rpi-userland/rpi-userland-003-disable-timer_t-typedef.patch @@ -0,0 +1,22 @@ +I don't know which platform is missing the timer_t definition, but it would +be better to check for that platform specifically instead of explicitly +excluding platforms that do have it. + +This fixes compilation with musl libc, which does define timer_t but does +not define __timer_t. + +Note that _HAVE_TIMER_T suggests it would prevent breakage, but it is +unrealistic to expect every application to set that macro. Not even all CMake +files in the userland package itself set it... + +--- userland-f9e6f9f3def8dc18dc0092cd695ccf53b8ba3efe.org/interface/vcos/pthreads/vcos_platform.h 2014-08-05 19:43:53.000000000 +0200 ++++ rpi-userland-f9e6f9f3def8dc18dc0092cd695ccf53b8ba3efe/interface/vcos/pthreads/vcos_platform.h 2014-09-11 21:04:36.418943953 +0200 +@@ -120,7 +120,7 @@ + + #define VCOS_ONCE_INIT PTHREAD_ONCE_INIT + +-#if defined(__arm__) && !defined(_HAVE_TIMER_T) && !defined(ANDROID) ++#if 0 + typedef __timer_t timer_t; + #endif + typedef struct VCOS_TIMER_T diff --git a/package/rpi-userland/rpi-userland-004-include-for-HZ.patch b/package/rpi-userland/rpi-userland-004-include-for-HZ.patch new file mode 100644 index 0000000..484aebe --- /dev/null +++ b/package/rpi-userland/rpi-userland-004-include-for-HZ.patch @@ -0,0 +1,19 @@ +There is no guarantee that <sys/param.h> defines HZ. And in musl libc, it +doesn't. Since this is a Linux-specific constant, include the Linux-specific +header for it. + +Note that HZ as defined by the system headers is the default value of HZ (100), +which might differ from the actual value of HZ in the kernel config. Ideally +userland would not use HZ at all and do all timing in milliseconds instead. + +--- userland-f9e6f9f3def8dc18dc0092cd695ccf53b8ba3efe.org/interface/vcos/pthreads/vcos_pthreads.c 2014-08-05 19:43:53.000000000 +0200 ++++ rpi-userland-f9e6f9f3def8dc18dc0092cd695ccf53b8ba3efe/interface/vcos/pthreads/vcos_pthreads.c 2014-09-11 21:49:48.497827644 +0200 +@@ -33,7 +33,7 @@ + #include <stdlib.h> + #include <stdio.h> + #include <sys/time.h> +-#include <sys/param.h> ++#include <linux/param.h> + + /* Cygwin doesn't always have prctl.h and it doesn't have PR_SET_NAME */ + #if defined( __linux__ ) diff --git a/package/rpi-userland/rpi-userland-005-do-not-check-__USE_FILE_OFFSET64.patch b/package/rpi-userland/rpi-userland-005-do-not-check-__USE_FILE_OFFSET64.patch new file mode 100644 index 0000000..6a1ab47 --- /dev/null +++ b/package/rpi-userland/rpi-userland-005-do-not-check-__USE_FILE_OFFSET64.patch @@ -0,0 +1,16 @@ +This looks like a sanity check that 64-bit file offset definitions were indeed +provided by the included headers. But __USE_FILE_OFFSET64 is an internal +define of glibc, so don't perform this check when compiling with a different +libc. + +--- userland-f9e6f9f3def8dc18dc0092cd695ccf53b8ba3efe.org/interface/vmcs_host/linux/vcfilesys.c 2014-08-05 19:43:53.000000000 +0200 ++++ rpi-userland-f9e6f9f3def8dc18dc0092cd695ccf53b8ba3efe/interface/vmcs_host/linux/vcfilesys.c 2014-09-11 22:01:51.016328294 +0200 +@@ -49,7 +49,7 @@ + #include <ctype.h> + #include <limits.h> + +-#if !defined(ANDROID) && !defined( __USE_FILE_OFFSET64 ) ++#if defined(__GLIBC__) && !defined( __USE_FILE_OFFSET64 ) + #error "__USE_FILE_OFFSET64 isn't defined" + #endif + diff --git a/package/rpi-userland/rpi-userland-006-mode_t.patch b/package/rpi-userland/rpi-userland-006-mode_t.patch new file mode 100644 index 0000000..6045ade --- /dev/null +++ b/package/rpi-userland/rpi-userland-006-mode_t.patch @@ -0,0 +1,13 @@ +The type of "st_mode" in "struct stat" is just "mode_t", not "__mode_t". + +--- userland-f9e6f9f3def8dc18dc0092cd695ccf53b8ba3efe.org/interface/vmcs_host/linux/vcfilesys.c 2014-08-05 19:43:53.000000000 +0200 ++++ rpi-userland-f9e6f9f3def8dc18dc0092cd695ccf53b8ba3efe/interface/vmcs_host/linux/vcfilesys.c 2014-09-12 00:28:46.553861915 +0200 +@@ -916,7 +916,7 @@ + + if (pathbuf) + { +- __mode_t mode = 0; ++ mode_t mode = 0; + struct stat sb; + + backslash_to_slash(pathbuf);
Signed-off-by: Maarten ter Huurne <maarten@treewalker.org> --- .../rpi-userland-003-disable-timer_t-typedef.patch | 22 ++++++++++++++++++++++ .../rpi-userland-004-include-for-HZ.patch | 19 +++++++++++++++++++ ...land-005-do-not-check-__USE_FILE_OFFSET64.patch | 16 ++++++++++++++++ package/rpi-userland/rpi-userland-006-mode_t.patch | 13 +++++++++++++ 4 files changed, 70 insertions(+) create mode 100644 package/rpi-userland/rpi-userland-003-disable-timer_t-typedef.patch create mode 100644 package/rpi-userland/rpi-userland-004-include-for-HZ.patch create mode 100644 package/rpi-userland/rpi-userland-005-do-not-check-__USE_FILE_OFFSET64.patch create mode 100644 package/rpi-userland/rpi-userland-006-mode_t.patch