Re: [Mingw-w64-public] [PATCH 2/2] Ensure wait timeouts are respected
在 2019/3/2 1:16, LRN 写道: > On 01.03.2019 17:19, Liu Hao wrote: >> Wouldn't this be better, saving a call to `_pthread_get_tick_count()` ? >> >> >> (the compound assignment operator seems incorrect). >> > > New version is attached. > > > > +result = WaitForSingleObject ((HANDLE) handle, (DWORD) wait_time); > +if (result != WAIT_TIMEOUT) > + break; > + > +current_time = _pthread_get_tick_count (); > +if (current_time >= end_time || result != WAIT_TIMEOUT) > + break; The second `result != WAIT_TIMEOUT` is now unnecessary. I have deleted it, as well as the other one. BTW it looks like SF Git server is down now. I may push this later today. -- Best regards, LH_Mouse ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH] Add Microsoft OLE DB driver for SQL server
> This can be verified by printing the size of the enclosing struct > using GCC with our header, then comparing it with the result using > MSVC and Microsoft header. Nifty :-) Actually I've believed that **declaration** of nested structures is just a declaration, and no code/data will be generated for that. But it turns out that I was wrong. Thus, if I have the following sample.c file: ``` struct A { struct B {int b;}; struct C {int c;}; }; int main(void) { return sizeof (struct A); } ``` and compile it with GCC v8.2.1 on a Linux-based OS I get: ``` $ x86_64-pc-linux-gnu-gcc gcc -O2 -S -o - sample.c ... xorl%eax, %eax ret ``` Well, that's fine -- I get an empty `struct A`, just as it was declared. But if I compile the file with x86_64-w64-mingw32-gcc on Microsoft Windows: ``` $ x86_64-w64-mingw32-gcc -O2 -S -o - sample.c ... movl$8, %eax ... ret ``` Oops... I get a non-empty (!) `struct A` with two 4-bytes `int` fields! That's a bug I believe. :-( > It would result in duplicated definitions which I think is pretty bad Okay, I will publish new patch for reviewing on the next Tuesday, 3/5/2019, after 3:00 AM UTC. Unfortunately, I can't publish it earlier. Thanks! On 3/1/2019 7:33 PM, Liu Hao wrote: > 在 2019/3/1 15:14, Ruslan Garipov 写道: >>> It looks like we have ended up in a bug there >> >> I don't know :-( To summarize: MSVC, Intel C++ and GCC on Microsoft >> Windows fail to compile that sample C code. But clang for Microsoft >> Windows does compile the code (just like GCC for Linux-based OSes). >> clang-cl fails to compile of course; due to the back-end shared with >> MSVC I believe. >> > > This can be verified by printing the size of the enclosing struct using > GCC with our header, then comparing it with the result using MSVC and > Microsoft header. > >>> I think we should try promoting named nested structs and unions (so >>> they appear in the file scope). At least this works for both C and >>> C++, providing these names don't conflict with each other. >> >> If I understand you correctly, you propose to move declarations of the >> nested structures like `_Time2Val`, `_DateTimeVal` and so on out of >> declaration of the `SSVARIANT` structure (where they will be at the file >> scope)? For both C and C++. >> >> I've also proposed the same thing but for C only, because such moving >> out does not change senantics. Had I compiled the code with nested >> structures successfully by a C compiler, the nested structures are at >> unit scope I believe: >> > > Yes. This results in consistent code, and doesn't bring duplication of > the struct definition (outside the struct for C and inside it for C++). > >>> 6.7.2.1 Structure and union specifiers >>> 8 The presence of a struct-declaration-list in a >>> struct-or-union-specifier declares a new type, within a translation unit. >> >> But for C++ to preserve† isolation of the nested types (as it is in the >> original header file) we may stick with a fix I've already shown -- a >> fix proposed by you, Hao (declaring structure types before an anonymous >> union having members of those types). >> >> Therefore, I want to use `#ifdef __cplusplus` to separate these two >> solutions. >> >> † 10.3.10 Nested class declarations [class.nest] >> 1 ... The name of a nested class is local to its enclosing class. The >> nested class is in the scope of its enclosing class. >> >> > > It would result in duplicated definitions which I think is pretty bad. > > ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH 2/2] Ensure wait timeouts are respected
On 01.03.2019 17:19, Liu Hao wrote: > Wouldn't this be better, saving a call to `_pthread_get_tick_count()` ? > > > (the compound assignment operator seems incorrect). > New version is attached. From 0d93a97765716b5b642bf39663c5162af3f8ebf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=A0=D1=83=D1=81=D0=BB=D0=B0=D0=BD=20=D0=98=D0=B6=D0=B1?= =?UTF-8?q?=D1=83=D0=BB=D0=B0=D1=82=D0=BE=D0=B2?= Date: Thu, 28 Feb 2019 17:55:08 + Subject: [PATCH 2/2] Ensure wait timeouts are respected WaitFor*() functions may time out earlier than requested or later than requested. The "later" part is generally OK. The "earlier" part is not. Fix this by running the wait functions in a loop until the time actually runs out, or the function returns a non-timeout code. This does not apply to cases where timeout is 0 or INFINITE. Those still use plain WaitFor*() calls (the wrappers, obviously, still support these important corner-cases, but why call them when we know that the timeout is 0 or INFINITE?). This wouldn't have worked for handles that auto-reset (i.e. where WaitFor*() call wakes up on handle activation, and immediately resets the handle, preventing further WaitFor*() calls from waking up until the handle is activated again), but winpthreads does not use those. The timeout checker uses the coarse GetTickCount64() because it's fast and doesn't wrap around every 49 days. Though it might not be accurate enough, let's go with it first. On pre-Vista systems QueryPerformanceCounter() is used instead. --- mingw-w64-libraries/winpthreads/src/cond.c | 8 +-- mingw-w64-libraries/winpthreads/src/misc.c | 104 +++ mingw-w64-libraries/winpthreads/src/misc.h | 2 + mingw-w64-libraries/winpthreads/src/mutex.c | 2 +- mingw-w64-libraries/winpthreads/src/thread.c | 4 +- 5 files changed, 113 insertions(+), 7 deletions(-) diff --git a/mingw-w64-libraries/winpthreads/src/cond.c b/mingw-w64-libraries/winpthreads/src/cond.c index 3754a56..368ee8a 100644 --- a/mingw-w64-libraries/winpthreads/src/cond.c +++ b/mingw-w64-libraries/winpthreads/src/cond.c @@ -598,7 +598,7 @@ do_sema_b_wait_intern (HANDLE sema, int nointerrupt, DWORD timeout) DWORD res, dt; if (nointerrupt == 1) { -res = WaitForSingleObject(sema, timeout); +res = _pthread_wait_for_single_object(sema, timeout); switch (res) { case WAIT_TIMEOUT: r = ETIMEDOUT; @@ -622,7 +622,7 @@ do_sema_b_wait_intern (HANDLE sema, int nointerrupt, DWORD timeout) if (maxH == 2) { redo: - res = WaitForMultipleObjects(maxH, arr, 0, timeout); + res = _pthread_wait_for_multiple_objects(maxH, arr, 0, timeout); switch (res) { case WAIT_TIMEOUT: r = ETIMEDOUT; @@ -655,7 +655,7 @@ redo: if (timeout == INFINITE) { do { - res = WaitForSingleObject(sema, 40); + res = _pthread_wait_for_single_object(sema, 40); switch (res) { case WAIT_TIMEOUT: r = ETIMEDOUT; @@ -684,7 +684,7 @@ redo: dt = 20; do { if (dt > timeout) dt = timeout; -res = WaitForSingleObject(sema, dt); +res = _pthread_wait_for_single_object(sema, dt); switch (res) { case WAIT_TIMEOUT: r = ETIMEDOUT; diff --git a/mingw-w64-libraries/winpthreads/src/misc.c b/mingw-w64-libraries/winpthreads/src/misc.c index d8753f2..76b89d3 100644 --- a/mingw-w64-libraries/winpthreads/src/misc.c +++ b/mingw-w64-libraries/winpthreads/src/misc.c @@ -52,3 +52,107 @@ unsigned long long _pthread_rel_time_in_ms(const struct timespec *ts) return t1 - t2; } +static unsigned long long +_pthread_get_tick_count (long long *frequency) +{ +#if defined (_WIN32_WINNT) && (_WIN32_WINNT >= _WIN32_WINNT_VISTA) + (void) frequency; /* unused */ + return GetTickCount64 (); +#else + LARGE_INTEGER freq, timestamp; + + if (*frequency == 0) + { +if (QueryPerformanceFrequency ()) + *frequency = freq.QuadPart; +else + *frequency = -1; + } + + if (*frequency > 0 && QueryPerformanceCounter ()) +return timestamp.QuadPart / (*frequency / 1000); + + /* Fallback */ + return GetTickCount (); +#endif +} + +/* A wrapper around WaitForSingleObject() that ensures that + * the wait function does not time out before the time + * actually runs out. This is needed because WaitForSingleObject() + * might have poor accuracy, returning earlier than expected. + * On the other hand, returning a bit *later* than expected + * is acceptable in a preemptive multitasking environment. + */ +unsigned long +_pthread_wait_for_single_object (void *handle, unsigned long timeout) +{ + DWORD result; + unsigned long long start_time, end_time; + unsigned long wait_time; + long long frequency = 0; + + if (timeout == INFINITE || timeout == 0) +return WaitForSingleObject ((HANDLE) handle, (DWORD) timeout); + + start_time = _pthread_get_tick_count (); + end_time = start_time + timeout; + wait_time = timeout; + + do + { +unsigned long long current_time; + +
Re: [Mingw-w64-public] [PATCH 2/2] Ensure wait timeouts are respected
On 01.03.2019 17:19, Liu Hao wrote: > 在 2019/3/1 19:08, LRN 写道: >> New version is attached. >> > >> >> +result = WaitForMultipleObjects ((DWORD) count, (HANDLE *) handles, >> all, (DWORD) wait_time); >> +current_time = _pthread_get_tick_count (); >> +if (current_time >= end_time || result != WAIT_TIMEOUT) >> + break; > > Wouldn't this be better, saving a call to `_pthread_get_tick_count()` ? > > ``` > if (result != WAIT_TIMEOUT) > break; > current_time = _pthread_get_tick_count (); > if (current_time >= end_time) > break; > ``` Yes, but i'm not sure which is more optimal - two if() checks vs a possibly unneeded function call. > >> + >> +wait_time -= (DWORD) (end_time - current_time); > > It looks like this should be > > ``` > wait_time = (DWORD) (end_time - current_time); > ``` > > (the compound assignment operator seems incorrect). > ARGH. What is *wrong* with me? signature.asc Description: OpenPGP digital signature ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH] Add Microsoft OLE DB driver for SQL server
在 2019/3/1 15:14, Ruslan Garipov 写道: >> It looks like we have ended up in a bug there > > I don't know :-( To summarize: MSVC, Intel C++ and GCC on Microsoft > Windows fail to compile that sample C code. But clang for Microsoft > Windows does compile the code (just like GCC for Linux-based OSes). > clang-cl fails to compile of course; due to the back-end shared with > MSVC I believe. > This can be verified by printing the size of the enclosing struct using GCC with our header, then comparing it with the result using MSVC and Microsoft header. >> I think we should try promoting named nested structs and unions (so >> they appear in the file scope). At least this works for both C and >> C++, providing these names don't conflict with each other. > > If I understand you correctly, you propose to move declarations of the > nested structures like `_Time2Val`, `_DateTimeVal` and so on out of > declaration of the `SSVARIANT` structure (where they will be at the file > scope)? For both C and C++. > > I've also proposed the same thing but for C only, because such moving > out does not change senantics. Had I compiled the code with nested > structures successfully by a C compiler, the nested structures are at > unit scope I believe: > Yes. This results in consistent code, and doesn't bring duplication of the struct definition (outside the struct for C and inside it for C++). >> 6.7.2.1 Structure and union specifiers >> 8 The presence of a struct-declaration-list in a >> struct-or-union-specifier declares a new type, within a translation unit. > > But for C++ to preserve† isolation of the nested types (as it is in the > original header file) we may stick with a fix I've already shown -- a > fix proposed by you, Hao (declaring structure types before an anonymous > union having members of those types). > > Therefore, I want to use `#ifdef __cplusplus` to separate these two > solutions. > > † 10.3.10 Nested class declarations [class.nest] > 1 ... The name of a nested class is local to its enclosing class. The > nested class is in the scope of its enclosing class. > > It would result in duplicated definitions which I think is pretty bad. -- Best regards, LH_Mouse signature.asc Description: OpenPGP digital signature ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH 2/2] Ensure wait timeouts are respected
在 2019/3/1 19:08, LRN 写道: > New version is attached. > > > +result = WaitForMultipleObjects ((DWORD) count, (HANDLE *) handles, all, > (DWORD) wait_time); > +current_time = _pthread_get_tick_count (); > +if (current_time >= end_time || result != WAIT_TIMEOUT) > + break; Wouldn't this be better, saving a call to `_pthread_get_tick_count()` ? ``` if (result != WAIT_TIMEOUT) break; current_time = _pthread_get_tick_count (); if (current_time >= end_time) break; ``` > + > +wait_time -= (DWORD) (end_time - current_time); It looks like this should be ``` wait_time = (DWORD) (end_time - current_time); ``` (the compound assignment operator seems incorrect). > + } while (TRUE); -- Best regards, LH_Mouse ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH 1/2] Round up when converting nanoseconds to milliseconds
在 2019/3/1 19:08, LRN 写道: > > New version is attached. > > > OK, pushed this one. -- Best regards, LH_Mouse ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH] Remove support for --disable-secure-api.
On 28/02/2019 21:23, Martin Storsjö wrote: On Thu, 28 Feb 2019, Liu Hao wrote: 在 2019/2/28 22:57, Jacek Caban 写道: It could be useful a long time ago for avoiding using APIs that were new and not always present back then. Also, back then, most compatibility functions that we have now were missing. I don't see a point in supporting it any more. Signed-off-by: Jacek Caban --- I have no objection on removal of it. However I would like to hear about other's opinions. Sounds sensible to me. I presume that passing --enable-secure-api to configure will just give a harmless warning and won't break the build for everybody out there? Yes, it will give an unknown option warning. I pushed this and other patches. Thanks for reviews, Jacek ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH 2/2] Ensure wait timeouts are respected
On 01.03.2019 9:13, Liu Hao wrote: > 在 2019/3/1 下午1:34, LRN 写道: >> On 01.03.2019 5:53, Liu Hao wrote: >> >> GetTickCount64() then, i guess? >> > > `GetTIckCount64()` for Vista and above or `QueryPerformanceCounter()` > for XP. I really don't care about XP, since MSYS2 has officially dropped > XP support, but I think it might be configurable during build time by > always using `uint64_t` for timestamps and call `GetTickCount()` or the > 64-bit variant depending on the value of `_WIN32_WINNT`. > > New version is attached. From bc4c59a8ee9a709e1ced271c9fdcb2cc6c0ad466 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=A0=D1=83=D1=81=D0=BB=D0=B0=D0=BD=20=D0=98=D0=B6=D0=B1?= =?UTF-8?q?=D1=83=D0=BB=D0=B0=D1=82=D0=BE=D0=B2?= Date: Thu, 28 Feb 2019 17:55:08 + Subject: [PATCH 2/2] Ensure wait timeouts are respected WaitFor*() functions may time out earlier than requested or later than requested. The "later" part is generally OK. The "earlier" part is not. Fix this by running the wait functions in a loop until the time actually runs out, or the function returns a non-timeout code. This does not apply to cases where timeout is 0 or INFINITE. Those still use plain WaitFor*() calls (the wrappers, obviously, still support these important corner-cases, but why call them when we know that the timeout is 0 or INFINITE?). This wouldn't have worked for handles that auto-reset (i.e. where WaitFor*() call wakes up on handle activation, and immediately resets the handle, preventing further WaitFor*() calls from waking up until the handle is activated again), but winpthreads does not use those. The timeout checker uses the coarse GetTickCount() because it's fast. Though it might not be accurate enough, let's go with it first. --- mingw-w64-libraries/winpthreads/src/cond.c | 8 +-- mingw-w64-libraries/winpthreads/src/misc.c | 98 mingw-w64-libraries/winpthreads/src/misc.h | 2 + mingw-w64-libraries/winpthreads/src/mutex.c | 2 +- mingw-w64-libraries/winpthreads/src/thread.c | 4 +- 5 files changed, 107 insertions(+), 7 deletions(-) diff --git a/mingw-w64-libraries/winpthreads/src/cond.c b/mingw-w64-libraries/winpthreads/src/cond.c index 3754a56..368ee8a 100644 --- a/mingw-w64-libraries/winpthreads/src/cond.c +++ b/mingw-w64-libraries/winpthreads/src/cond.c @@ -598,7 +598,7 @@ do_sema_b_wait_intern (HANDLE sema, int nointerrupt, DWORD timeout) DWORD res, dt; if (nointerrupt == 1) { -res = WaitForSingleObject(sema, timeout); +res = _pthread_wait_for_single_object(sema, timeout); switch (res) { case WAIT_TIMEOUT: r = ETIMEDOUT; @@ -622,7 +622,7 @@ do_sema_b_wait_intern (HANDLE sema, int nointerrupt, DWORD timeout) if (maxH == 2) { redo: - res = WaitForMultipleObjects(maxH, arr, 0, timeout); + res = _pthread_wait_for_multiple_objects(maxH, arr, 0, timeout); switch (res) { case WAIT_TIMEOUT: r = ETIMEDOUT; @@ -655,7 +655,7 @@ redo: if (timeout == INFINITE) { do { - res = WaitForSingleObject(sema, 40); + res = _pthread_wait_for_single_object(sema, 40); switch (res) { case WAIT_TIMEOUT: r = ETIMEDOUT; @@ -684,7 +684,7 @@ redo: dt = 20; do { if (dt > timeout) dt = timeout; -res = WaitForSingleObject(sema, dt); +res = _pthread_wait_for_single_object(sema, dt); switch (res) { case WAIT_TIMEOUT: r = ETIMEDOUT; diff --git a/mingw-w64-libraries/winpthreads/src/misc.c b/mingw-w64-libraries/winpthreads/src/misc.c index d8753f2..c23f30f 100644 --- a/mingw-w64-libraries/winpthreads/src/misc.c +++ b/mingw-w64-libraries/winpthreads/src/misc.c @@ -52,3 +52,101 @@ unsigned long long _pthread_rel_time_in_ms(const struct timespec *ts) return t1 - t2; } +static unsigned long long +_pthread_get_tick_count (long long *frequency) +{ +#if defined (_WIN32_WINNT) && (_WIN32_WINNT >= _WIN32_WINNT_VISTA) + (void) frequency; /* unused */ + return GetTickCount64 (); +#else + LARGE_INTEGER freq, timestamp; + + if (*frequency == 0) + { +if (QueryPerformanceFrequency ()) + *frequency = freq.QuadPart; +else + *frequency = -1; + } + + if (*frequency > 0 && QueryPerformanceCounter ()) +return timestamp.QuadPart / (*frequency / 1000); + + /* Fallback */ + return GetTickCount (); +#endif +} + +/* A wrapper around WaitForSingleObject() that ensures that + * the wait function does not time out before the time + * actually runs out. This is needed because WaitForSingleObject() + * might have poor accuracy, returning earlier than expected. + * On the other hand, returning a bit *later* than expected + * is acceptable in a preemptive multitasking environment. + */ +unsigned long +_pthread_wait_for_single_object (void *handle, unsigned long timeout) +{ + DWORD result; + unsigned long long start_time, end_time; + unsigned long wait_time; + long long frequency = 0; + + if (timeout == INFINITE || timeout == 0) +return
Re: [Mingw-w64-public] [PATCH 1/2] Round up when converting nanoseconds to milliseconds
On 01.03.2019 5:48, Liu Hao wrote: > 在 2019/3/1 上午2:09, LRN 写道: >> If the caller provides ts_nsec in struct timespec, >> we lose precision when converting that time to milliseconds >> for our WaitFor*() calls. Make sure we round *up* when doing that >> conversion, as otherwise the wait time will be *less* than the caller >> expects. Users of pthreads on non-realtime systems are generally >> OK with functions returning a bit later than requested. But timing out >> *earlier* than the requested time is completely unexpected. >> --- >> mingw-w64-libraries/winpthreads/src/misc.c | 13 - >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> >> > > This seems an overkill. There is a much simpler solution: > > ``` > t += (unsigned long long) (ts->tv_nsec + 99) / 100; > ``` > > New version is attached. From 59fb104b98567fd4a56444daa4382e9e43279856 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=A0=D1=83=D1=81=D0=BB=D0=B0=D0=BD=20=D0=98=D0=B6=D0=B1?= =?UTF-8?q?=D1=83=D0=BB=D0=B0=D1=82=D0=BE=D0=B2?= Date: Thu, 28 Feb 2019 17:49:49 + Subject: [PATCH 1/2] Round up when converting nanoseconds to milliseconds If the caller provides ts_nsec in struct timespec, we lose precision when converting that time to milliseconds for our WaitFor*() calls. Make sure we round *up* when doing that conversion, as otherwise the wait time will be *less* than the caller expects. Users of pthreads on non-realtime systems are generally OK with functions returning a bit later than requested. But timing out *earlier* than the requested time is completely unexpected. --- mingw-w64-libraries/winpthreads/src/misc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mingw-w64-libraries/winpthreads/src/misc.c b/mingw-w64-libraries/winpthreads/src/misc.c index ab0488c..d8753f2 100644 --- a/mingw-w64-libraries/winpthreads/src/misc.c +++ b/mingw-w64-libraries/winpthreads/src/misc.c @@ -36,7 +36,8 @@ unsigned long long _pthread_time_in_ms(void) unsigned long long _pthread_time_in_ms_from_timespec(const struct timespec *ts) { unsigned long long t = (unsigned long long) ts->tv_sec * 1000LL; -t += (unsigned long long) (ts->tv_nsec / 100); +/* The +99 is here to ensure that the division always rounds up */ +t += (unsigned long long) (ts->tv_nsec + 99) / 100; return t; } -- 2.4.0 signature.asc Description: OpenPGP digital signature ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public