| Message ID | 1298829142-5550-2-git-send-email-weil@mail.berlios.de |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, Feb 27, 2011 at 7:52 PM, Stefan Weil <weil@mail.berlios.de> wrote: > timersub is needed by the latest vnc code. > > Signed-off-by: Stefan Weil <weil@mail.berlios.de> > --- > hosts/w32/include/sys/time.h | 24 ++++++++++++++++++++++++ > 1 files changed, 24 insertions(+), 0 deletions(-) > create mode 100644 hosts/w32/include/sys/time.h > > diff --git a/hosts/w32/include/sys/time.h b/hosts/w32/include/sys/time.h > new file mode 100644 > index 0000000..94056ff > --- /dev/null > +++ b/hosts/w32/include/sys/time.h Nack. The QEMU way of handling host peculiarities is to add wrappers, for example qemu_timersub in this case, and converting all callers. > @@ -0,0 +1,24 @@ > +/* > + * Extensions of MinGW sys/time.h > + * > + * Copyright (C) 2011 Stefan Weil > + * > + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. > + * See the COPYING.LIB file in the top-level directory. > + * > + */ > + > +#include_next <sys/time.h> Isn't include_next an extension by GCC? > + > +#ifndef timersub > +/* This is a copy from GNU C Library (GNU LGPL 2.1), sys/time.h. */ > +# define timersub(a, b, result) \ > + do { \ > + (result)->tv_sec = (a)->tv_sec - (b)->tv_sec; \ > + (result)->tv_usec = (a)->tv_usec - (b)->tv_usec; \ > + if ((result)->tv_usec < 0) { \ > + --(result)->tv_sec; \ > + (result)->tv_usec += 1000000; \ > + } \ > + } while (0) > +#endif > -- > 1.7.2.3 > > >
On 5 March 2011 09:34, Blue Swirl <blauwirbel@gmail.com> wrote: > On Sun, Feb 27, 2011 at 7:52 PM, Stefan Weil <weil@mail.berlios.de> wrote: >> +#include_next <sys/time.h> > > Isn't include_next an extension by GCC? Are gcc extensions forbidden? We already have plenty of code that uses gcc-specific syntax or gcc extensions... -- PMM
On Sat, Mar 5, 2011 at 11:42 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 5 March 2011 09:34, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Sun, Feb 27, 2011 at 7:52 PM, Stefan Weil <weil@mail.berlios.de> wrote: >>> +#include_next <sys/time.h> >> >> Isn't include_next an extension by GCC? > > Are gcc extensions forbidden? We already have plenty of > code that uses gcc-specific syntax or gcc extensions... No, but relying on them is not OK. For example gcc attributes are wrapped in macros to allow other implementations.
On 5 March 2011 09:48, Blue Swirl <blauwirbel@gmail.com> wrote: > On Sat, Mar 5, 2011 at 11:42 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 5 March 2011 09:34, Blue Swirl <blauwirbel@gmail.com> wrote: >>> On Sun, Feb 27, 2011 at 7:52 PM, Stefan Weil <weil@mail.berlios.de> wrote: >>>> +#include_next <sys/time.h> >>> >>> Isn't include_next an extension by GCC? >> >> Are gcc extensions forbidden? We already have plenty of >> code that uses gcc-specific syntax or gcc extensions... > > No, but relying on them is not OK. For example gcc attributes are > wrapped in macros to allow other implementations. Three random counter-examples: vl.c: machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */ That use of ?: is a gcc extension. target-i386/cpu.h: register struct CPUX86State *env asm(AREG0); Explicit register variables are a gcc extension. qemu-timer-common.c: static void __attribute__((constructor)) init_get_clock(void) gcc-specific attribute not hidden by a macro. -- PMM
On Sat, Mar 5, 2011 at 11:55 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 5 March 2011 09:48, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Sat, Mar 5, 2011 at 11:42 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 5 March 2011 09:34, Blue Swirl <blauwirbel@gmail.com> wrote: >>>> On Sun, Feb 27, 2011 at 7:52 PM, Stefan Weil <weil@mail.berlios.de> wrote: >>>>> +#include_next <sys/time.h> >>>> >>>> Isn't include_next an extension by GCC? >>> >>> Are gcc extensions forbidden? We already have plenty of >>> code that uses gcc-specific syntax or gcc extensions... >> >> No, but relying on them is not OK. For example gcc attributes are >> wrapped in macros to allow other implementations. > > Three random counter-examples: > > vl.c: > machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */ > > That use of ?: is a gcc extension. There's no need to use that, so those should be removed. > target-i386/cpu.h: > register struct CPUX86State *env asm(AREG0); > > Explicit register variables are a gcc extension. Is there any other way? > qemu-timer-common.c: > static void __attribute__((constructor)) init_get_clock(void) > > gcc-specific attribute not hidden by a macro. Probably should be wrapped, but I don't think we can avoid constructors.
Am 05.03.2011 10:34, schrieb Blue Swirl: > On Sun, Feb 27, 2011 at 7:52 PM, Stefan Weil <weil@mail.berlios.de> wrote: >> timersub is needed by the latest vnc code. >> >> Signed-off-by: Stefan Weil <weil@mail.berlios.de> >> --- >> hosts/w32/include/sys/time.h | 24 ++++++++++++++++++++++++ >> 1 files changed, 24 insertions(+), 0 deletions(-) >> create mode 100644 hosts/w32/include/sys/time.h >> >> diff --git a/hosts/w32/include/sys/time.h b/hosts/w32/include/sys/time.h >> new file mode 100644 >> index 0000000..94056ff >> --- /dev/null >> +++ b/hosts/w32/include/sys/time.h > > Nack. The QEMU way of handling host peculiarities is to add wrappers, > for example qemu_timersub in this case, and converting all callers. That's one way how things can be handled. I know at least one QEMU maintainer who does not like those workarounds, especially if they are needed because of w32/w64 operating systems, so I decided to choose a different approach here. My approach also can be easily removed as soon as MinGW is improved (I'm sure it will be improved). There is another QEMU way of handling host peculiarities: conditional compilation. Most of this kind of handling could be eliminated by my approach. > >> @@ -0,0 +1,24 @@ >> +/* >> + * Extensions of MinGW sys/time.h >> + * >> + * Copyright (C) 2011 Stefan Weil >> + * >> + * This work is licensed under the terms of the GNU LGPL, version >> 2.1 or later. >> + * See the COPYING.LIB file in the top-level directory. >> + * >> + */ >> + >> +#include_next <sys/time.h> > > Isn't include_next an extension by GCC? It is, but that's not critical given the fact that QEMU does not work without gcc. If needed, this dependency can be fixed easily by renaming the intermediate include files (and including the renamed files). I don't think that those intermediate files will be needed forever, so a clean and cheap solution like the one which I implemented seems to be more important to me. > >> + >> +#ifndef timersub >> +/* This is a copy from GNU C Library (GNU LGPL 2.1), sys/time.h. */ >> +# define timersub(a, b, result) >> \ >> + do { >> \ >> + (result)->tv_sec = (a)->tv_sec - (b)->tv_sec; >> \ >> + (result)->tv_usec = (a)->tv_usec - (b)->tv_usec; >> \ >> + if ((result)->tv_usec < 0) { >> \ >> + --(result)->tv_sec; >> \ >> + (result)->tv_usec += 1000000; >> \ >> + } >> \ >> + } while (0) >> +#endif >> -- >> 1.7.2.3 By the way: it's not possible to add timersub to existing files like oslib-win32.c or os-win32.c (another way to handle the problem) because of incompatible licenses. Regards, Stefan W.
diff --git a/hosts/w32/include/sys/time.h b/hosts/w32/include/sys/time.h new file mode 100644 index 0000000..94056ff --- /dev/null +++ b/hosts/w32/include/sys/time.h @@ -0,0 +1,24 @@ +/* + * Extensions of MinGW sys/time.h + * + * Copyright (C) 2011 Stefan Weil + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. + * + */ + +#include_next <sys/time.h> + +#ifndef timersub +/* This is a copy from GNU C Library (GNU LGPL 2.1), sys/time.h. */ +# define timersub(a, b, result) \ + do { \ + (result)->tv_sec = (a)->tv_sec - (b)->tv_sec; \ + (result)->tv_usec = (a)->tv_usec - (b)->tv_usec; \ + if ((result)->tv_usec < 0) { \ + --(result)->tv_sec; \ + (result)->tv_usec += 1000000; \ + } \ + } while (0) +#endif
timersub is needed by the latest vnc code. Signed-off-by: Stefan Weil <weil@mail.berlios.de> --- hosts/w32/include/sys/time.h | 24 ++++++++++++++++++++++++ 1 files changed, 24 insertions(+), 0 deletions(-) create mode 100644 hosts/w32/include/sys/time.h