diff mbox

[for-2.4] oslib-win32: only provide localtime_r/gmtime_r if missing

Message ID 1438337876-13558-1-git-send-email-berrange@redhat.com
State New
Headers show

Commit Message

Daniel P. Berrangé July 31, 2015, 10:17 a.m. UTC
The oslib-win32 file currently provides a localtime_r and
gmtime_r replacement unconditionally. Some versions of
Mingw64 would provide crude macros for localtime_r/gmtime_r
which QEMU takes care to disable. Latest versions of Mingw64
now provide actual functions for localtime_r/gmtime_r, but
with a twist that you have to include unistd.h or pthread.h
before including time.h.  By luck some files in QEMU have
such an include order, resulting in compile errors:

  CC    util/osdep.o
In file included from include/qemu-common.h:48:0,
                 from util/osdep.c:48:
include/sysemu/os-win32.h:77:12: error: redundant redeclaration of 'gmtime_r' [-Werror=redundant-decls]
 struct tm *gmtime_r(const time_t *timep, struct tm *result);
            ^
In file included from include/qemu-common.h:35:0,
                 from util/osdep.c:48:
/usr/i686-w64-mingw32/sys-root/mingw/include/time.h:272:107: note: previous definition of 'gmtime_r' was here
In file included from include/qemu-common.h:48:0,
                 from util/osdep.c:48:
include/sysemu/os-win32.h:79:12: error: redundant redeclaration of 'localtime_r' [-Werror=redundant-decls]
 struct tm *localtime_r(const time_t *timep, struct tm *result);
            ^
In file included from include/qemu-common.h:35:0,
                 from util/osdep.c:48:
/usr/i686-w64-mingw32/sys-root/mingw/include/time.h:269:107: note: previous definition of 'localtime_r' was here

This change adds a configure test to see if localtime_r
exits, and only enables the QEMU impl if missing. We also
re-arrange qemu-common.h try attempt to guarantee that all
source files get unistd.h before time.h and thus see the
localtime_r/gmtime_r defs.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 configure                 | 34 ++++++++++++++++++++++++++++++++++
 include/qemu-common.h     |  4 +++-
 include/sysemu/os-win32.h |  2 ++
 util/oslib-win32.c        |  2 ++
 4 files changed, 41 insertions(+), 1 deletion(-)

Comments

Paolo Bonzini July 31, 2015, 12:49 p.m. UTC | #1
On 31/07/2015 12:17, Daniel P. Berrange wrote:
> 
>   CC    util/osdep.o
> In file included from include/qemu-common.h:48:0,
>                  from util/osdep.c:48:
> include/sysemu/os-win32.h:77:12: error: redundant redeclaration of 'gmtime_r' [-Werror=redundant-decls]
>  struct tm *gmtime_r(const time_t *timep, struct tm *result);
>             ^
> In file included from include/qemu-common.h:35:0,
>                  from util/osdep.c:48:
> /usr/i686-w64-mingw32/sys-root/mingw/include/time.h:272:107: note: previous definition of 'gmtime_r' was here
> In file included from include/qemu-common.h:48:0,
>                  from util/osdep.c:48:
> include/sysemu/os-win32.h:79:12: error: redundant redeclaration of 'localtime_r' [-Werror=redundant-decls]
>  struct tm *localtime_r(const time_t *timep, struct tm *result);
>             ^
> In file included from include/qemu-common.h:35:0,
>                  from util/osdep.c:48:
> /usr/i686-w64-mingw32/sys-root/mingw/include/time.h:269:107: note: previous definition of 'localtime_r' was here
> 
> This change adds a configure test to see if localtime_r
> exits, and only enables the QEMU impl if missing. We also
> re-arrange qemu-common.h try attempt to guarantee that all
> source files get unistd.h before time.h and thus see the
> localtime_r/gmtime_r defs.

These are only with --enable-werror, right?  The patch shouldn't be
necessary for 2.4.

Paolo
Daniel P. Berrangé July 31, 2015, 1:33 p.m. UTC | #2
On Fri, Jul 31, 2015 at 02:49:49PM +0200, Paolo Bonzini wrote:
> 
> 
> On 31/07/2015 12:17, Daniel P. Berrange wrote:
> > 
> >   CC    util/osdep.o
> > In file included from include/qemu-common.h:48:0,
> >                  from util/osdep.c:48:
> > include/sysemu/os-win32.h:77:12: error: redundant redeclaration of 'gmtime_r' [-Werror=redundant-decls]
> >  struct tm *gmtime_r(const time_t *timep, struct tm *result);
> >             ^
> > In file included from include/qemu-common.h:35:0,
> >                  from util/osdep.c:48:
> > /usr/i686-w64-mingw32/sys-root/mingw/include/time.h:272:107: note: previous definition of 'gmtime_r' was here
> > In file included from include/qemu-common.h:48:0,
> >                  from util/osdep.c:48:
> > include/sysemu/os-win32.h:79:12: error: redundant redeclaration of 'localtime_r' [-Werror=redundant-decls]
> >  struct tm *localtime_r(const time_t *timep, struct tm *result);
> >             ^
> > In file included from include/qemu-common.h:35:0,
> >                  from util/osdep.c:48:
> > /usr/i686-w64-mingw32/sys-root/mingw/include/time.h:269:107: note: previous definition of 'localtime_r' was here
> > 
> > This change adds a configure test to see if localtime_r
> > exits, and only enables the QEMU impl if missing. We also
> > re-arrange qemu-common.h try attempt to guarantee that all
> > source files get unistd.h before time.h and thus see the
> > localtime_r/gmtime_r defs.
> 
> These are only with --enable-werror, right?  The patch shouldn't be
> necessary for 2.4.

True, these are warnings only, so only fatal if you turn on Werror


Regards,
Daniel
Stefan Weil July 31, 2015, 5:58 p.m. UTC | #3
Am 31.07.2015 um 14:49 schrieb Paolo Bonzini:
> On 31/07/2015 12:17, Daniel P. Berrange wrote:
>>    CC    util/osdep.o
>> In file included from include/qemu-common.h:48:0,
>>                   from util/osdep.c:48:
>> include/sysemu/os-win32.h:77:12: error: redundant redeclaration of 'gmtime_r' [-Werror=redundant-decls]
>>   struct tm *gmtime_r(const time_t *timep, struct tm *result);
>>              ^
>> In file included from include/qemu-common.h:35:0,
>>                   from util/osdep.c:48:
>> /usr/i686-w64-mingw32/sys-root/mingw/include/time.h:272:107: note: previous definition of 'gmtime_r' was here
>> In file included from include/qemu-common.h:48:0,
>>                   from util/osdep.c:48:
>> include/sysemu/os-win32.h:79:12: error: redundant redeclaration of 'localtime_r' [-Werror=redundant-decls]
>>   struct tm *localtime_r(const time_t *timep, struct tm *result);
>>              ^
>> In file included from include/qemu-common.h:35:0,
>>                   from util/osdep.c:48:
>> /usr/i686-w64-mingw32/sys-root/mingw/include/time.h:269:107: note: previous definition of 'localtime_r' was here
>>
>> This change adds a configure test to see if localtime_r
>> exits, and only enables the QEMU impl if missing. We also
>> re-arrange qemu-common.h try attempt to guarantee that all
>> source files get unistd.h before time.h and thus see the
>> localtime_r/gmtime_r defs.
> These are only with --enable-werror, right?  The patch shouldn't be
> necessary for 2.4.
>
> Paolo

It isn't necessary for 2.4. There are more severe problems which also
seem to be triggered by recent changes in the MinGW-w64 toolchain:

64 bit executables crash at longjmp from generated code,
and networking fails because of asynchronous connect calls
which were never implemented correctly in the slirp code.
I'll try to summarize all that I know in a separate mail later.

I only recently got reports about these problems and think
that it is too late to fix them in 2.4.0.

Stefan
Daniel P. Berrangé Aug. 5, 2015, 9:52 a.m. UTC | #4
On Fri, Jul 31, 2015 at 07:58:45PM +0200, Stefan Weil wrote:
> Am 31.07.2015 um 14:49 schrieb Paolo Bonzini:
> >On 31/07/2015 12:17, Daniel P. Berrange wrote:
> >>   CC    util/osdep.o
> >>In file included from include/qemu-common.h:48:0,
> >>                  from util/osdep.c:48:
> >>include/sysemu/os-win32.h:77:12: error: redundant redeclaration of 'gmtime_r' [-Werror=redundant-decls]
> >>  struct tm *gmtime_r(const time_t *timep, struct tm *result);
> >>             ^
> >>In file included from include/qemu-common.h:35:0,
> >>                  from util/osdep.c:48:
> >>/usr/i686-w64-mingw32/sys-root/mingw/include/time.h:272:107: note: previous definition of 'gmtime_r' was here
> >>In file included from include/qemu-common.h:48:0,
> >>                  from util/osdep.c:48:
> >>include/sysemu/os-win32.h:79:12: error: redundant redeclaration of 'localtime_r' [-Werror=redundant-decls]
> >>  struct tm *localtime_r(const time_t *timep, struct tm *result);
> >>             ^
> >>In file included from include/qemu-common.h:35:0,
> >>                  from util/osdep.c:48:
> >>/usr/i686-w64-mingw32/sys-root/mingw/include/time.h:269:107: note: previous definition of 'localtime_r' was here
> >>
> >>This change adds a configure test to see if localtime_r
> >>exits, and only enables the QEMU impl if missing. We also
> >>re-arrange qemu-common.h try attempt to guarantee that all
> >>source files get unistd.h before time.h and thus see the
> >>localtime_r/gmtime_r defs.
> >These are only with --enable-werror, right?  The patch shouldn't be
> >necessary for 2.4.
> >
> >Paolo
> 
> It isn't necessary for 2.4. There are more severe problems which also
> seem to be triggered by recent changes in the MinGW-w64 toolchain:
> 
> 64 bit executables crash at longjmp from generated code,
> and networking fails because of asynchronous connect calls
> which were never implemented correctly in the slirp code.
> I'll try to summarize all that I know in a separate mail later.
> 
> I only recently got reports about these problems and think
> that it is too late to fix them in 2.4.0.

Could you post the info about these problems, or point me to the bug
reports. I'm currently trying to get a patchset of mine working on
Win32 so need to understand what's broken with Mingw/Win32 currently.

Regards,
Daniel
Stefan Weil Aug. 5, 2015, 11:03 a.m. UTC | #5
Am 05.08.2015 um 11:52 schrieb Daniel P. Berrange:
> On Fri, Jul 31, 2015 at 07:58:45PM +0200, Stefan Weil wrote:
>> Am 31.07.2015 um 14:49 schrieb Paolo Bonzini:
>>> On 31/07/2015 12:17, Daniel P. Berrange wrote:
>>>>    CC    util/osdep.o
>>>> In file included from include/qemu-common.h:48:0,
>>>>                   from util/osdep.c:48:
>>>> include/sysemu/os-win32.h:77:12: error: redundant redeclaration of 'gmtime_r' [-Werror=redundant-decls]
>>>>   struct tm *gmtime_r(const time_t *timep, struct tm *result);
>>>>              ^
>>>> In file included from include/qemu-common.h:35:0,
>>>>                   from util/osdep.c:48:
>>>> /usr/i686-w64-mingw32/sys-root/mingw/include/time.h:272:107: note: previous definition of 'gmtime_r' was here
>>>> In file included from include/qemu-common.h:48:0,
>>>>                   from util/osdep.c:48:
>>>> include/sysemu/os-win32.h:79:12: error: redundant redeclaration of 'localtime_r' [-Werror=redundant-decls]
>>>>   struct tm *localtime_r(const time_t *timep, struct tm *result);
>>>>              ^
>>>> In file included from include/qemu-common.h:35:0,
>>>>                   from util/osdep.c:48:
>>>> /usr/i686-w64-mingw32/sys-root/mingw/include/time.h:269:107: note: previous definition of 'localtime_r' was here
>>>>
>>>> This change adds a configure test to see if localtime_r
>>>> exits, and only enables the QEMU impl if missing. We also
>>>> re-arrange qemu-common.h try attempt to guarantee that all
>>>> source files get unistd.h before time.h and thus see the
>>>> localtime_r/gmtime_r defs.
>>> These are only with --enable-werror, right?  The patch shouldn't be
>>> necessary for 2.4.
>>>
>>> Paolo
>> It isn't necessary for 2.4. There are more severe problems which also
>> seem to be triggered by recent changes in the MinGW-w64 toolchain:
>>
>> 64 bit executables crash at longjmp from generated code,
>> and networking fails because of asynchronous connect calls
>> which were never implemented correctly in the slirp code.
>> I'll try to summarize all that I know in a separate mail later.
>>
>> I only recently got reports about these problems and think
>> that it is too late to fix them in 2.4.0.
> Could you post the info about these problems, or point me to the bug
> reports. I'm currently trying to get a patchset of mine working on
> Win32 so need to understand what's broken with Mingw/Win32 currently.
>
> Regards,
> Daniel

Sorry for the delay - I have currently only sporadic
internet access.

I recently upgraded my build environment from Debian Wheezy
to Debian Jessie. Obviously the newer MinGW-w64 packages
introduced several problems for QEMU:

* Compiler warnings caused by newer compiler and changes in
system include files

* Modified order of include files and definitions. This results in
a wrong sigsetjmp call in cpu-exec.c for 64 bit code.
That code will crash as soon as there is a longjmp returning
from generated TCG code because that code does not support
stack unwinding.

* Asynchronous connect call. Obviously TCP connect was
synchronous with earlier versions. Now it can also fail,
but Windows does not set errno (slirp expects errno code).
This results in buggy networking with all Windows versions
of QEMU (any connect will fail).

Here are two hacks to fix these problems:

Fix sigsetjmp for w64
http://repo.or.cz/w/qemu/ar7.git/commit/8fa9c07c9a33174905e67589bea6be3e278712cb

slirp: Fix non blocking connect for w32
http://repo.or.cz/w/qemu/ar7.git/commit/b3f21d56ad3f36562d396685de8ff4981af6b805

At least the first patch (maybe also the 2nd one) will
also be needed for old versions of QEMU as they
don't depend on the QEMU version but on the
version of MinGW-w64.

Regards
Stefan
Liviu Ionescu Aug. 5, 2015, 4:51 p.m. UTC | #6
> On 05 Aug 2015, at 14:03, Stefan Weil <sw@weilnetz.de> wrote:
> 
> Fix sigsetjmp for w64
> http://repo.or.cz/w/qemu/ar7.git/commit/8fa9c07c9a33174905e67589bea6be3e278712cb


I tried to apply your patch to my branch and I got:

Running QEMU make install...
  CC    gnuarmeclipse-softmmu/cpu-exec.o
/Host/Work/qemu/gnuarmeclipse-qemu.git/cpu-exec.c:37:0: warning: "sigsetjmp" redefined
 #define sigsetjmp(env, savesigs) _setjmp(env, NULL)
 ^
In file included from /Host/Work/qemu/gnuarmeclipse-qemu.git/include/qemu-common.h:47:0,
                 from /Host/Work/qemu/gnuarmeclipse-qemu.git/target-arm/cpu.h:39,
                 from /Host/Work/qemu/gnuarmeclipse-qemu.git/cpu-exec.c:20:
/Host/Work/qemu/gnuarmeclipse-qemu.git/include/sysemu/os-win32.h:72:0: note: this is the location of the previous definition
 #define sigsetjmp(env, savemask) setjmp(env)
 ^


I did not check the latest version in the repository (waiting for 2.4), but could you verify, maybe a better location for your patch would be in os-win32.h?


Regards,

Liviu
Paolo Bonzini Aug. 5, 2015, 4:56 p.m. UTC | #7
On 05/08/2015 18:51, Liviu Ionescu wrote:
> 
>> On 05 Aug 2015, at 14:03, Stefan Weil <sw@weilnetz.de> wrote:
>>
>> Fix sigsetjmp for w64
>> http://repo.or.cz/w/qemu/ar7.git/commit/8fa9c07c9a33174905e67589bea6be3e278712cb
> 
> 
> I tried to apply your patch to my branch and I got:
> 
> Running QEMU make install...
>   CC    gnuarmeclipse-softmmu/cpu-exec.o
> /Host/Work/qemu/gnuarmeclipse-qemu.git/cpu-exec.c:37:0: warning: "sigsetjmp" redefined
>  #define sigsetjmp(env, savesigs) _setjmp(env, NULL)
>  ^
> In file included from /Host/Work/qemu/gnuarmeclipse-qemu.git/include/qemu-common.h:47:0,
>                  from /Host/Work/qemu/gnuarmeclipse-qemu.git/target-arm/cpu.h:39,
>                  from /Host/Work/qemu/gnuarmeclipse-qemu.git/cpu-exec.c:20:
> /Host/Work/qemu/gnuarmeclipse-qemu.git/include/sysemu/os-win32.h:72:0: note: this is the location of the previous definition
>  #define sigsetjmp(env, savemask) setjmp(env)
>  ^
> 
> 
> I did not check the latest version in the repository (waiting for
> 2.4), but could you verify, maybe a better location for your patch
> would be in os-win32.h?

os-win32.h has the following already:

#if defined(_WIN64)
# undef setjmp
# define setjmp(env) _setjmp(env, NULL)
#endif
#define sigjmp_buf jmp_buf
#define sigsetjmp(env, savemask) setjmp(env)


And Stefan's patch has this in cpu-exec.c:

#if defined(_WIN64)
#define sigsetjmp(env, savesigs) _setjmp(env, NULL)
#endif

The above should not be necessary if the definition of sigsetjmp is
already picked up from os-win32.h.

Since os-win32.h is usually included through qemu-common.h, my guess was
that cpu-exec.c was missing an inclusion of qemu-common.h.  However, all
target-*/cpu.h files include qemu-common.h, so I am not sure why things
break for Stefan...

Paolo
Liviu Ionescu Aug. 5, 2015, 6:39 p.m. UTC | #8
> On 05 Aug 2015, at 19:56, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> ... I am not sure why things break for Stefan...

I confirm Stefan's conclusion, neither in my configuration adding 

#include "qemu-common.h"

... in cpu-exec.c makes any difference.

however adding:

#if defined(_WIN64)
#ifdef sigsetjmp
#undef sigsetjmp
#endif
#define sigsetjmp(env, savesigs) _setjmp(env, NULL)
#endif

... fixes the problem, my custom QEMU happily blinks the LEDs on Win 8.1 64-bits (see below).

perhaps a headers check would be helpful, such mysterious behaviours usually back fire at a certain point.


regards,

Liviu


GNU ARM Eclipse 64-bits QEMU v2.3.50 (qemu-system-gnuarmeclipse.exe).
Board: 'STM32F4-Discovery' (ST Discovery kit for STM32F407/417 lines).
Device: 'STM32F407VG' (Cortex-M4 r0p0, MPU), Flash: 1024 kB, RAM: 128 kB.
Command line: 'f4' (2 bytes).
Cortex-M4 r0p0 core initialised.
GDB Server listening on: 'tcp::1234'...
Cortex-M4 r0p0 core reset.
... connection accepted from 127.0.0.1.

PRIGROUP unimplemented
Execute 'mon system_reset'.
Cortex-M4 r0p0 core reset.

main(argc=1, argv=["f4"]);
Hello ARM World!
System clock: 168000000Hz
Standard output message.
Standard error message.
[Green LED On]
[Green LED Off]
Second 1
[Green LED On]
[Green LED Off]
Second 2
[Green LED On]
[Green LED Off]
Second 3
[Green LED On]
[Green LED Off]
Second 4
[Green LED On]
[Green LED Off]
Second 5
QEMU exit(0)
Stefan Weil Aug. 5, 2015, 8:30 p.m. UTC | #9
Am 05.08.2015 um 20:39 schrieb Liviu Ionescu:
>> On 05 Aug 2015, at 19:56, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> ... I am not sure why things break for Stefan...
> I confirm Stefan's conclusion, neither in my configuration adding
>
> #include "qemu-common.h"
>
> ... in cpu-exec.c makes any difference.
>
> however adding:
>
> #if defined(_WIN64)
> #ifdef sigsetjmp
> #undef sigsetjmp
> #endif
> #define sigsetjmp(env, savesigs) _setjmp(env, NULL)
> #endif
>
> ... fixes the problem, my custom QEMU happily blinks the LEDs on Win 8.1 64-bits (see below).
>
> perhaps a headers check would be helpful, such mysterious behaviours usually back fire at a certain point.
>
>
> regards,
>
> Liviu

http://qemu.weilnetz.de/test/cpu-exec.i shows the result of
the C preprocessor:

cpu-exec.c gets QEMU's os-win32.h with our definition of setjmp
early, but the system header file setjmp.h is included later, and
that file re-defines our definitions. Including setjmp.h from
os-win32.h would solve the problem, but I think there is a
better solution.

I am planning to remove the special definitions for _WIN64 from
os-win32.h and add them to cpu-exec.c, similar to the code
above (which can be shortened a little)
but with some comment lines added.

As I already said, this modification is needed for all versions
of QEMU, and it will stay unfixed in 2.4.0 which is nearly finished.

Regards
Stefan
Liviu Ionescu Aug. 5, 2015, 9:42 p.m. UTC | #10
> On 05 Aug 2015, at 23:30, Stefan Weil <sw@weilnetz.de> wrote:
> 
> ... As I already said, this modification is needed for all versions
> of QEMU, and it will stay unfixed in 2.4.0 which is nearly finished.

your patch is already in my branch, and I already publicly released GNU ARM Eclipse QEMU for Windows 64-bits, so no problem, take your time and work on a solution you consider appropriate.

regards,

Liviu
Kevin Wolf Aug. 6, 2015, 8:44 a.m. UTC | #11
Am 05.08.2015 um 22:30 hat Stefan Weil geschrieben:
> Am 05.08.2015 um 20:39 schrieb Liviu Ionescu:
> >>On 05 Aug 2015, at 19:56, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>
> >>... I am not sure why things break for Stefan...
> >I confirm Stefan's conclusion, neither in my configuration adding
> >
> >#include "qemu-common.h"
> >
> >... in cpu-exec.c makes any difference.
> >
> >however adding:
> >
> >#if defined(_WIN64)
> >#ifdef sigsetjmp
> >#undef sigsetjmp
> >#endif
> >#define sigsetjmp(env, savesigs) _setjmp(env, NULL)
> >#endif
> >
> >... fixes the problem, my custom QEMU happily blinks the LEDs on Win 8.1 64-bits (see below).
> >
> >perhaps a headers check would be helpful, such mysterious behaviours usually back fire at a certain point.
> >
> >
> >regards,
> >
> >Liviu
> 
> http://qemu.weilnetz.de/test/cpu-exec.i shows the result of
> the C preprocessor:
> 
> cpu-exec.c gets QEMU's os-win32.h with our definition of setjmp
> early, but the system header file setjmp.h is included later, and
> that file re-defines our definitions. Including setjmp.h from
> os-win32.h would solve the problem, but I think there is a
> better solution.
> 
> I am planning to remove the special definitions for _WIN64 from
> os-win32.h and add them to cpu-exec.c, similar to the code
> above (which can be shortened a little)
> but with some comment lines added.

What happens with the callers outside of cpu-exec.c then? Fixing the
problem inside os-win32.h seems more robust to me.

Kevin
Stefan Weil Aug. 6, 2015, 10:12 a.m. UTC | #12
Am 06.08.2015 um 10:44 schrieb Kevin Wolf:
> Am 05.08.2015 um 22:30 hat Stefan Weil geschrieben:
>> Am 05.08.2015 um 20:39 schrieb Liviu Ionescu:
>>>> On 05 Aug 2015, at 19:56, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>
>>>> ... I am not sure why things break for Stefan...
>>> I confirm Stefan's conclusion, neither in my configuration adding
>>>
>>> #include "qemu-common.h"
>>>
>>> ... in cpu-exec.c makes any difference.
>>>
>>> however adding:
>>>
>>> #if defined(_WIN64)
>>> #ifdef sigsetjmp
>>> #undef sigsetjmp
>>> #endif
>>> #define sigsetjmp(env, savesigs) _setjmp(env, NULL)
>>> #endif
>>>
>>> ... fixes the problem, my custom QEMU happily blinks the LEDs on Win 8.1 64-bits (see below).
>>>
>>> perhaps a headers check would be helpful, such mysterious behaviours usually back fire at a certain point.
>>>
>>>
>>> regards,
>>>
>>> Liviu
>> http://qemu.weilnetz.de/test/cpu-exec.i shows the result of
>> the C preprocessor:
>>
>> cpu-exec.c gets QEMU's os-win32.h with our definition of setjmp
>> early, but the system header file setjmp.h is included later, and
>> that file re-defines our definitions. Including setjmp.h from
>> os-win32.h would solve the problem, but I think there is a
>> better solution.
>>
>> I am planning to remove the special definitions for _WIN64 from
>> os-win32.h and add them to cpu-exec.c, similar to the code
>> above (which can be shortened a little)
>> but with some comment lines added.
> What happens with the callers outside of cpu-exec.c then? Fixing the
> problem inside os-win32.h seems more robust to me.
>
> Kevin

(sig)setjmp/(sig)longjmp without TCG generated code on the stack
does not need special handling because stack unwinding works
for compiled normal C code.

We only have a problem on 64 bit Windows with code generated
at run time by TCG because stack unwinding does not work there,
so a siglongjmp which tries to exit from that code will crash QEMU
when trying to unwind the call stack.

As far as I see, the only siglongjmp exits from TCG generated code
are in cpu-exec.c (functions cpu_loop_exit and
cpu_resume_from_signal). The matching sigsetjmp is also in
cpu-exec.c, and here we have to disable stack unwinding
by calling _setjmp(cpu->jmp_env, NULL).

Stefan
Richard Henderson Aug. 6, 2015, 5:12 p.m. UTC | #13
On 08/06/2015 03:12 AM, Stefan Weil wrote:
> (sig)setjmp/(sig)longjmp without TCG generated code on the stack
> does not need special handling because stack unwinding works
> for compiled normal C code.
> 
> We only have a problem on 64 bit Windows with code generated
> at run time by TCG because stack unwinding does not work there,
> so a siglongjmp which tries to exit from that code will crash QEMU
> when trying to unwind the call stack.

I suppose we could fix that, for 2.5, anyway.
It would appear that RtlAddFunctionTable is
the proper interface.

It would probably also help debugging just as
much as adding the elf unwind info did.

> As far as I see, the only siglongjmp exits from TCG generated code
> are in cpu-exec.c (functions cpu_loop_exit and
> cpu_resume_from_signal). The matching sigsetjmp is also in
> cpu-exec.c, and here we have to disable stack unwinding
> by calling _setjmp(cpu->jmp_env, NULL).

That sounds right.


r~
Peter Maydell Aug. 10, 2015, 10:25 a.m. UTC | #14
On 5 August 2015 at 12:03, Stefan Weil <sw@weilnetz.de> wrote:
> I recently upgraded my build environment from Debian Wheezy
> to Debian Jessie. Obviously the newer MinGW-w64 packages
> introduced several problems for QEMU:
>
> * Compiler warnings caused by newer compiler and changes in
> system include files
>
> * Modified order of include files and definitions. This results in
> a wrong sigsetjmp call in cpu-exec.c for 64 bit code.
> That code will crash as soon as there is a longjmp returning
> from generated TCG code because that code does not support
> stack unwinding.
>
> * Asynchronous connect call. Obviously TCP connect was
> synchronous with earlier versions. Now it can also fail,
> but Windows does not set errno (slirp expects errno code).
> This results in buggy networking with all Windows versions
> of QEMU (any connect will fail).

Could somebody add some notes to the 'Known issues' section
of the changelog summarising the problems, please (whether
it affects all Windows build environments or only some,
whether this is a regression or not, etc):
http://wiki.qemu.org/ChangeLog/2.4#Known_issues

thanks
-- PMM
Stefan Weil Aug. 10, 2015, 11:39 a.m. UTC | #15
Am 10.08.2015 um 12:25 schrieb Peter Maydell:

> Could somebody add some notes to the 'Known issues' section
> of the changelog summarising the problems, please (whether
> it affects all Windows build environments or only some,
> whether this is a regression or not, etc):
> http://wiki.qemu.org/ChangeLog/2.4#Known_issues
> 
> thanks
> -- PMM
> 

Done. I also added the BIOS regression and the missing
GTK ui translations.

Stefan
Paolo Bonzini Aug. 10, 2015, 2 p.m. UTC | #16
On 10/08/2015 13:39, Stefan Weil wrote:
> > Could somebody add some notes to the 'Known issues' section
> > of the changelog summarising the problems, please (whether
> > it affects all Windows build environments or only some,
> > whether this is a regression or not, etc):
> > http://wiki.qemu.org/ChangeLog/2.4#Known_issues
> > 
> > thanks
> > -- PMM
>
> Done. I also added the BIOS regression

Isn't it harmless anyway?

Paolo
Stefan Weil Aug. 10, 2015, 8:22 p.m. UTC | #17
Am 10.08.2015 um 16:00 schrieb Paolo Bonzini:
> 
> 
> On 10/08/2015 13:39, Stefan Weil wrote:
>>> Could somebody add some notes to the 'Known issues' section
>>> of the changelog summarising the problems, please (whether
>>> it affects all Windows build environments or only some,
>>> whether this is a regression or not, etc):
>>> http://wiki.qemu.org/ChangeLog/2.4#Known_issues
>>>
>>> thanks
>>> -- PMM
>>
>> Done. I also added the BIOS regression
> 
> Isn't it harmless anyway?
> 
> Paolo

I assume it is harmless for most users who don't
use much BIOS functionality.

Nevertheless there are several outl function calls
with wrong argument order. Those calls won't work
as expected. I don't know how important this is.

My QEMU tree includes trace messages for unaligned
i/o, so users of my precompiled QEMU for Windows
will see a message during system boot.

Regards
Stefan
diff mbox

Patch

diff --git a/configure b/configure
index 704b34c..322f97b 100755
--- a/configure
+++ b/configure
@@ -1726,6 +1726,37 @@  else
 fi
 
 ##########################################
+# Mingw64 localtime_r/gmtime_r check
+
+if test "$mingw32" = "yes"; then
+    # Some versions of Mingw32/64 lack localtime_r
+    # and gmtime_r entirely
+    #
+    # Some versions of Mingw64 define a macro for
+    # localtime_r/gmtime_r/etc
+    #
+    # Some versions of Ming64 will define functions
+    # for localtime_r/gmtime_r, but only if you have
+    # _POSIX_THREAD_SAFE_FUNCTIONS defined. For fun
+    # though, unistd.h and pthread.h both define
+    # that for you.
+    #
+    # So this #undef localtime_r and #include <unistd.h>
+    # are not in fact redundant
+cat > $TMPC << EOF
+#include <unistd.h>
+#include <time.h>
+#undef localtime_r
+int main(void) { localtime_r(NULL, NULL); return 0; }
+EOF
+    if compile_prog "" "" ; then
+        localtime_r="yes"
+    else
+	localtime_r="no"
+    fi
+fi
+
+##########################################
 # pkg-config probe
 
 if ! has "$pkg_config_exe"; then
@@ -4993,6 +5024,9 @@  fi
 if test "$zero_malloc" = "yes" ; then
   echo "CONFIG_ZERO_MALLOC=y" >> $config_host_mak
 fi
+if test "$localtime_r" = "yes" ; then
+  echo "CONFIG_LOCALTIME_R=y" >> $config_host_mak
+fi
 if test "$qom_cast_debug" = "yes" ; then
   echo "CONFIG_QOM_CAST_DEBUG=y" >> $config_host_mak
 fi
diff --git a/include/qemu-common.h b/include/qemu-common.h
index fb3da6c..e9e3f59 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -32,10 +32,12 @@ 
 #include <strings.h>
 #include <inttypes.h>
 #include <limits.h>
+/* Put unistd.h before time.h as that triggers localtime_r/gmtime_r
+ * function availability on recentish Mingw64 platforms */
+#include <unistd.h>
 #include <time.h>
 #include <ctype.h>
 #include <errno.h>
-#include <unistd.h>
 #include <fcntl.h>
 #include <sys/stat.h>
 #include <sys/time.h>
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index 4035c4f..ba65a25 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -73,10 +73,12 @@ 
 #define siglongjmp(env, val) longjmp(env, val)
 
 /* Missing POSIX functions. Don't use MinGW-w64 macros. */
+#ifndef CONFIG_LOCALTIME_R
 #undef gmtime_r
 struct tm *gmtime_r(const time_t *timep, struct tm *result);
 #undef localtime_r
 struct tm *localtime_r(const time_t *timep, struct tm *result);
+#endif /* CONFIG_LOCALTIME_R */
 
 
 static inline void os_setup_signal_handling(void) {}
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 730a670..08f5a9c 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -95,6 +95,7 @@  void qemu_anon_ram_free(void *ptr, size_t size)
     }
 }
 
+#ifndef CONFIG_LOCALTIME_R
 /* FIXME: add proper locking */
 struct tm *gmtime_r(const time_t *timep, struct tm *result)
 {
@@ -118,6 +119,7 @@  struct tm *localtime_r(const time_t *timep, struct tm *result)
     }
     return p;
 }
+#endif /* CONFIG_LOCALTIME_R */
 
 void qemu_set_block(int fd)
 {