diff mbox

[2/3] w32: Add macro timersub to sys/time.h

Message ID 1298829142-5550-2-git-send-email-weil@mail.berlios.de
State Superseded
Headers show

Commit Message

Stefan Weil Feb. 27, 2011, 5:52 p.m. UTC
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

Comments

Blue Swirl March 5, 2011, 9:34 a.m. UTC | #1
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
>
>
>
Peter Maydell March 5, 2011, 9:42 a.m. UTC | #2
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
Blue Swirl March 5, 2011, 9:48 a.m. UTC | #3
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.
Peter Maydell March 5, 2011, 9:55 a.m. UTC | #4
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
Blue Swirl March 5, 2011, 10:31 a.m. UTC | #5
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.
Stefan Weil March 6, 2011, 10:15 p.m. UTC | #6
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 mbox

Patch

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