[03/25] threads: update for late C11 changes

Submitted by Nicolai Hähnle on Oct. 22, 2017, 7:07 p.m.

Details

Message ID 20171022190808.9406-4-nhaehnle@gmail.com
State New
Headers show
Series "Asynchronous flushes and ddebug core rewrite" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Nicolai Hähnle Oct. 22, 2017, 7:07 p.m.
From: Nicolai Hähnle <nicolai.haehnle@amd.com>

C11 threads were changed to use struct timespec instead of xtime, and
thrd_sleep got a second argument.

See http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1554.htm and
http://en.cppreference.com/w/c/thread/{thrd_sleep,cnd_timedwait,mtx_timedlock}

Note that cnd_timedwait is spec'd to be relative to TIME_UTC / CLOCK_REALTIME.

Cc: Jose Fonseca <jfonseca@vmware.com>
---
 include/c11/threads.h           | 11 -----------
 include/c11/threads_posix.h     | 39 +++++++++++++++------------------------
 include/c11/threads_win32.h     | 37 +++++++++++++++++++------------------
 src/egl/drivers/dri2/egl_dri2.c | 24 +++++++++++++-----------
 4 files changed, 47 insertions(+), 64 deletions(-)

Patch hide | download patch | download mbox

diff --git a/include/c11/threads.h b/include/c11/threads.h
index 573348d8091..3c3f23a8ab8 100644
--- a/include/c11/threads.h
+++ b/include/c11/threads.h
@@ -23,42 +23,31 @@ 
  * FITNESS FOR A PARTICULAR PURPOSE, TITLE AND NON-INFRINGEMENT. IN NO EVENT
  * SHALL THE COPYRIGHT HOLDERS OR ANYONE DISTRIBUTING THE SOFTWARE BE LIABLE
  * FOR ANY DAMAGES OR OTHER LIABILITY, WHETHER IN CONTRACT, TORT OR OTHERWISE,
  * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
  * DEALINGS IN THE SOFTWARE.
  */
 #ifndef EMULATED_THREADS_H_INCLUDED_
 #define EMULATED_THREADS_H_INCLUDED_
 
 #include <time.h>
-#ifdef _MSC_VER
-#include <thr/xtimec.h>  // for xtime
-#endif
 
 #ifndef TIME_UTC
 #define TIME_UTC 1
 #endif
 
 #include "c99_compat.h" /* for `inline` */
 
 /*---------------------------- types ----------------------------*/
 typedef void (*tss_dtor_t)(void*);
 typedef int (*thrd_start_t)(void*);
 
-#ifndef _MSC_VER
-struct xtime {
-    time_t sec;
-    long nsec;
-};
-typedef struct xtime xtime;
-#endif
-
 
 /*-------------------- enumeration constants --------------------*/
 enum {
     mtx_plain     = 0,
     mtx_try       = 1,
     mtx_timed     = 2,
     mtx_recursive = 4
 };
 
 enum {
diff --git a/include/c11/threads_posix.h b/include/c11/threads_posix.h
index 43e803ee8d1..7bf6a0f6ef6 100644
--- a/include/c11/threads_posix.h
+++ b/include/c11/threads_posix.h
@@ -125,33 +125,29 @@  cnd_init(cnd_t *cond)
 // 7.25.3.4
 static inline int
 cnd_signal(cnd_t *cond)
 {
     assert(cond != NULL);
     return (pthread_cond_signal(cond) == 0) ? thrd_success : thrd_error;
 }
 
 // 7.25.3.5
 static inline int
-cnd_timedwait(cnd_t *cond, mtx_t *mtx, const xtime *xt)
+cnd_timedwait(cnd_t *cond, mtx_t *mtx, const struct timespec *abs_time)
 {
-    struct timespec abs_time;
     int rt;
 
     assert(mtx != NULL);
     assert(cond != NULL);
-    assert(xt != NULL);
+    assert(abs_time != NULL);
 
-    abs_time.tv_sec = xt->sec;
-    abs_time.tv_nsec = xt->nsec;
-
-    rt = pthread_cond_timedwait(cond, mtx, &abs_time);
+    rt = pthread_cond_timedwait(cond, mtx, abs_time);
     if (rt == ETIMEDOUT)
         return thrd_busy;
     return (rt == 0) ? thrd_success : thrd_error;
 }
 
 // 7.25.3.6
 static inline int
 cnd_wait(cnd_t *cond, mtx_t *mtx)
 {
     assert(mtx != NULL);
@@ -228,38 +224,35 @@  mtx_lock(mtx_t *mtx)
 }
 
 static inline int
 mtx_trylock(mtx_t *mtx);
 
 static inline void
 thrd_yield(void);
 
 // 7.25.4.4
 static inline int
-mtx_timedlock(mtx_t *mtx, const xtime *xt)
+mtx_timedlock(mtx_t *mtx, const struct timespec *ts)
 {
     assert(mtx != NULL);
-    assert(xt != NULL);
+    assert(ts != NULL);
 
     {
 #ifdef EMULATED_THREADS_USE_NATIVE_TIMEDLOCK
-    struct timespec ts;
     int rt;
-    ts.tv_sec = xt->sec;
-    ts.tv_nsec = xt->nsec;
-    rt = pthread_mutex_timedlock(mtx, &ts);
+    rt = pthread_mutex_timedlock(mtx, ts);
     if (rt == 0)
         return thrd_success;
     return (rt == ETIMEDOUT) ? thrd_busy : thrd_error;
 #else
     time_t expire = time(NULL);
-    expire += xt->sec;
+    expire += ts->tv_sec;
     while (mtx_trylock(mtx) != thrd_success) {
         time_t now = time(NULL);
         if (expire < now)
             return thrd_busy;
         // busy loop!
         thrd_yield();
     }
     return thrd_success;
 #endif
     }
@@ -335,27 +328,24 @@  thrd_join(thrd_t thr, int *res)
     void *code;
     if (pthread_join(thr, &code) != 0)
         return thrd_error;
     if (res)
         *res = (int)(intptr_t)code;
     return thrd_success;
 }
 
 // 7.25.5.7
 static inline void
-thrd_sleep(const xtime *xt)
+thrd_sleep(const struct timespec *time_point, struct timespec *remaining)
 {
-    struct timespec req;
-    assert(xt);
-    req.tv_sec = xt->sec;
-    req.tv_nsec = xt->nsec;
-    nanosleep(&req, NULL);
+    assert(time_point != NULL);
+    nanosleep(time_point, remaining);
 }
 
 // 7.25.5.8
 static inline void
 thrd_yield(void)
 {
     sched_yield();
 }
 
 
@@ -385,21 +375,22 @@  tss_get(tss_t key)
 // 7.25.6.4
 static inline int
 tss_set(tss_t key, void *val)
 {
     return (pthread_setspecific(key, val) == 0) ? thrd_success : thrd_error;
 }
 
 
 /*-------------------- 7.25.7 Time functions --------------------*/
 // 7.25.6.1
+#if 0
 static inline int
-xtime_get(xtime *xt, int base)
+timespec_get(struct timespec *ts, int base)
 {
-    if (!xt) return 0;
+    if (!ts) return 0;
     if (base == TIME_UTC) {
-        xt->sec = time(NULL);
-        xt->nsec = 0;
+        clock_gettime(CLOCK_REALTIME, ts);
         return base;
     }
     return 0;
 }
+#endif
diff --git a/include/c11/threads_win32.h b/include/c11/threads_win32.h
index af7df4b9ef5..1d892ff2c01 100644
--- a/include/c11/threads_win32.h
+++ b/include/c11/threads_win32.h
@@ -139,23 +139,23 @@  static unsigned __stdcall impl_thrd_routine(void *p)
 {
     struct impl_thrd_param pack;
     int code;
     memcpy(&pack, p, sizeof(struct impl_thrd_param));
     free(p);
     code = pack.func(pack.arg);
     impl_tss_dtor_invoke();
     return (unsigned)code;
 }
 
-static DWORD impl_xtime2msec(const xtime *xt)
+static DWORD impl_timespec2msec(const struct timespec *ts)
 {
-    return (DWORD)((xt->sec * 1000U) + (xt->nsec / 1000000L));
+    return (DWORD)((ts->tv_sec * 1000U) + (ts->tv_nsec / 1000000L));
 }
 
 #ifdef EMULATED_THREADS_USE_NATIVE_CALL_ONCE
 struct impl_call_once_param { void (*func)(void); };
 static BOOL CALLBACK impl_call_once_callback(PINIT_ONCE InitOnce, PVOID Parameter, PVOID *Context)
 {
     struct impl_call_once_param *param = (struct impl_call_once_param*)Parameter;
     (param->func)();
     ((void)InitOnce); ((void)Context);  // suppress warning
     return TRUE;
@@ -199,34 +199,34 @@  static void impl_cond_do_signal(cnd_t *cond, int broadcast)
             nsignal = cond->to_unblock = 1;
             cond->blocked--;
         }
     }
     LeaveCriticalSection(&cond->monitor);
 
     if (0 < nsignal)
         ReleaseSemaphore(cond->sem_queue, nsignal, NULL);
 }
 
-static int impl_cond_do_wait(cnd_t *cond, mtx_t *mtx, const xtime *xt)
+static int impl_cond_do_wait(cnd_t *cond, mtx_t *mtx, const struct timespec *ts)
 {
     int nleft = 0;
     int ngone = 0;
     int timeout = 0;
     DWORD w;
 
     WaitForSingleObject(cond->sem_gate, INFINITE);
     cond->blocked++;
     ReleaseSemaphore(cond->sem_gate, 1, NULL);
 
     mtx_unlock(mtx);
 
-    w = WaitForSingleObject(cond->sem_queue, xt ? impl_xtime2msec(xt) : INFINITE);
+    w = WaitForSingleObject(cond->sem_queue, ts ? impl_timespec2msec(ts) : INFINITE);
     timeout = (w == WAIT_TIMEOUT);
  
     EnterCriticalSection(&cond->monitor);
     if ((nleft = cond->to_unblock) != 0) {
         if (timeout) {
             if (cond->blocked != 0) {
                 cond->blocked--;
             } else {
                 cond->gone++;
             }
@@ -371,29 +371,29 @@  cnd_signal(cnd_t *cond)
 #ifdef EMULATED_THREADS_USE_NATIVE_CV
     WakeConditionVariable(&cond->condvar);
 #else
     impl_cond_do_signal(cond, 0);
 #endif
     return thrd_success;
 }
 
 // 7.25.3.5
 static inline int
-cnd_timedwait(cnd_t *cond, mtx_t *mtx, const xtime *xt)
+cnd_timedwait(cnd_t *cond, mtx_t *mtx, const struct timespec *abs_time)
 {
-    if (!cond || !mtx || !xt) return thrd_error;
+    if (!cond || !mtx || !abs_time) return thrd_error;
 #ifdef EMULATED_THREADS_USE_NATIVE_CV
-    if (SleepConditionVariableCS(&cond->condvar, mtx, impl_xtime2msec(xt)))
+    if (SleepConditionVariableCS(&cond->condvar, mtx, impl_timespec2msec(abs_time)))
         return thrd_success;
     return (GetLastError() == ERROR_TIMEOUT) ? thrd_busy : thrd_error;
 #else
-    return impl_cond_do_wait(cond, mtx, xt);
+    return impl_cond_do_wait(cond, mtx, ts);
 #endif
 }
 
 // 7.25.3.6
 static inline int
 cnd_wait(cnd_t *cond, mtx_t *mtx)
 {
     if (!cond || !mtx) return thrd_error;
 #ifdef EMULATED_THREADS_USE_NATIVE_CV
     SleepConditionVariableCS(&cond->condvar, mtx, INFINITE);
@@ -431,26 +431,26 @@  mtx_init(mtx_t *mtx, int type)
 static inline int
 mtx_lock(mtx_t *mtx)
 {
     if (!mtx) return thrd_error;
     EnterCriticalSection(mtx);
     return thrd_success;
 }
 
 // 7.25.4.4
 static inline int
-mtx_timedlock(mtx_t *mtx, const xtime *xt)
+mtx_timedlock(mtx_t *mtx, const struct timespec *ts)
 {
     time_t expire, now;
-    if (!mtx || !xt) return thrd_error;
+    if (!mtx || !ts) return thrd_error;
     expire = time(NULL);
-    expire += xt->sec;
+    expire += ts->tv_sec;
     while (mtx_trylock(mtx) != thrd_success) {
         now = time(NULL);
         if (expire < now)
             return thrd_busy;
         // busy loop!
         thrd_yield();
     }
     return thrd_success;
 }
 
@@ -572,24 +572,25 @@  thrd_join(thrd_t thr, int *res)
             return thrd_error;
         }
         *res = (int)code;
     }
     CloseHandle(thr);
     return thrd_success;
 }
 
 // 7.25.5.7
 static inline void
-thrd_sleep(const xtime *xt)
+thrd_sleep(const struct timespec *time_point, struct timespec *remaining)
 {
-    assert(xt);
-    Sleep(impl_xtime2msec(xt));
+    assert(time_point);
+    assert(!remaining); /* not implemented */
+    Sleep(impl_timespec2msec(time_point));
 }
 
 // 7.25.5.8
 static inline void
 thrd_yield(void)
 {
     SwitchToThread();
 }
 
 
@@ -627,20 +628,20 @@  tss_get(tss_t key)
 static inline int
 tss_set(tss_t key, void *val)
 {
     return TlsSetValue(key, val) ? thrd_success : thrd_error;
 }
 
 
 /*-------------------- 7.25.7 Time functions --------------------*/
 // 7.25.6.1
 static inline int
-xtime_get(xtime *xt, int base)
+timespec_get(struct timespec *ts, int base)
 {
-    if (!xt) return 0;
+    if (!ts) return 0;
     if (base == TIME_UTC) {
-        xt->sec = time(NULL);
-        xt->nsec = 0;
+        ts->tv_sec = time(NULL);
+        ts->tv_nsec = 0;
         return base;
     }
     return 0;
 }
diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 503450542e5..c4771cb45e7 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -3065,24 +3065,20 @@  dri2_dup_native_fence_fd(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync)
 static EGLint
 dri2_client_wait_sync(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync,
                       EGLint flags, EGLTime timeout)
 {
    _EGLContext *ctx = _eglGetCurrentContext();
    struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
    struct dri2_egl_context *dri2_ctx = dri2_egl_context(ctx);
    struct dri2_egl_sync *dri2_sync = dri2_egl_sync(sync);
    unsigned wait_flags = 0;
 
-   /* timespecs for cnd_timedwait */
-   struct timespec current;
-   xtime expire;
-
    EGLint ret = EGL_CONDITION_SATISFIED_KHR;
 
    /* The EGL_KHR_fence_sync spec states:
     *
     *    "If no context is current for the bound API,
     *     the EGL_SYNC_FLUSH_COMMANDS_BIT_KHR bit is ignored.
     */
    if (dri2_ctx && flags & EGL_SYNC_FLUSH_COMMANDS_BIT_KHR)
       wait_flags |= __DRI2_FENCE_FLAG_FLUSH_COMMANDS;
 
@@ -3109,33 +3105,39 @@  dri2_client_wait_sync(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync,
       }
 
       /* if timeout is EGL_FOREVER_KHR, it should wait without any timeout.*/
       if (timeout == EGL_FOREVER_KHR) {
          mtx_lock(&dri2_sync->mutex);
          cnd_wait(&dri2_sync->cond, &dri2_sync->mutex);
          mtx_unlock(&dri2_sync->mutex);
       } else {
          /* if reusable sync has not been yet signaled */
          if (dri2_sync->base.SyncStatus != EGL_SIGNALED_KHR) {
+            /* timespecs for cnd_timedwait */
+            struct timespec current;
+            struct timespec expire;
+
+            /* We override the clock to monotonic when creating the condition
+             * variable. */
             clock_gettime(CLOCK_MONOTONIC, &current);
 
             /* calculating when to expire */
-            expire.nsec = timeout % 1000000000L;
-            expire.sec = timeout / 1000000000L;
+            expire.tv_nsec = timeout % 1000000000L;
+            expire.tv_sec = timeout / 1000000000L;
 
-            expire.nsec += current.tv_nsec;
-            expire.sec += current.tv_sec;
+            expire.tv_nsec += current.tv_nsec;
+            expire.tv_sec += current.tv_sec;
 
             /* expire.nsec now is a number between 0 and 1999999998 */
-            if (expire.nsec > 999999999L) {
-               expire.sec++;
-               expire.nsec -= 1000000000L;
+            if (expire.tv_nsec > 999999999L) {
+               expire.tv_sec++;
+               expire.tv_nsec -= 1000000000L;
             }
 
             mtx_lock(&dri2_sync->mutex);
             ret = cnd_timedwait(&dri2_sync->cond, &dri2_sync->mutex, &expire);
             mtx_unlock(&dri2_sync->mutex);
 
             if (ret == thrd_busy) {
                if (dri2_sync->base.SyncStatus == EGL_UNSIGNALED_KHR) {
                   ret = EGL_TIMEOUT_EXPIRED_KHR;
                } else {

Comments

Hi Nicolai,

On 22 October 2017 at 20:07, Nicolai Hähnle <nhaehnle@gmail.com> wrote:
> From: Nicolai Hähnle <nicolai.haehnle@amd.com>
>
> C11 threads were changed to use struct timespec instead of xtime, and
> thrd_sleep got a second argument.
>
As xtime was replaced with timespec there's a couple of odd bits in the code.


> See http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1554.htm and
> http://en.cppreference.com/w/c/thread/{thrd_sleep,cnd_timedwait,mtx_timedlock}
>
> Note that cnd_timedwait is spec'd to be relative to TIME_UTC / CLOCK_REALTIME.
>
> Cc: Jose Fonseca <jfonseca@vmware.com>
> ---
>  include/c11/threads.h           | 11 -----------
>  include/c11/threads_posix.h     | 39 +++++++++++++++------------------------
>  include/c11/threads_win32.h     | 37 +++++++++++++++++++------------------
>  src/egl/drivers/dri2/egl_dri2.c | 24 +++++++++++++-----------
>  4 files changed, 47 insertions(+), 64 deletions(-)
>
> diff --git a/include/c11/threads.h b/include/c11/threads.h
> index 573348d8091..3c3f23a8ab8 100644
> --- a/include/c11/threads.h
> +++ b/include/c11/threads.h
> @@ -23,42 +23,31 @@
>   * FITNESS FOR A PARTICULAR PURPOSE, TITLE AND NON-INFRINGEMENT. IN NO EVENT
>   * SHALL THE COPYRIGHT HOLDERS OR ANYONE DISTRIBUTING THE SOFTWARE BE LIABLE
>   * FOR ANY DAMAGES OR OTHER LIABILITY, WHETHER IN CONTRACT, TORT OR OTHERWISE,
>   * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>   * DEALINGS IN THE SOFTWARE.
>   */
>  #ifndef EMULATED_THREADS_H_INCLUDED_
>  #define EMULATED_THREADS_H_INCLUDED_
>
>  #include <time.h>
> -#ifdef _MSC_VER
> -#include <thr/xtimec.h>  // for xtime
> -#endif
>
>  #ifndef TIME_UTC
>  #define TIME_UTC 1
>  #endif
>
>  #include "c99_compat.h" /* for `inline` */
>
>  /*---------------------------- types ----------------------------*/
>  typedef void (*tss_dtor_t)(void*);
>  typedef int (*thrd_start_t)(void*);
>
> -#ifndef _MSC_VER
> -struct xtime {
> -    time_t sec;
> -    long nsec;
> -};
> -typedef struct xtime xtime;
> -#endif
> -

We don't have a fall-back declaration of the struct, yet we use it
below and provide a timespec_get() implementation.
I'd imagine you haven't tested this on Windows (hence Jose in CC)?

Quick search suggests that MSVC 2015 was the first one that introduces
the struct and timespec_get.

If we're safe as-is, please add a comment with some details - I'd
imagine Jose had better knowledge in the area.


>  /*-------------------- 7.25.7 Time functions --------------------*/
>  // 7.25.6.1
> +#if 0
I'd just drop the hunk mentioning that timespec_get() is part of time.h


Thank
Emil
On 23.10.2017 13:15, Emil Velikov wrote:
> Hi Nicolai,
> 
> On 22 October 2017 at 20:07, Nicolai Hähnle <nhaehnle@gmail.com> wrote:
>> From: Nicolai Hähnle <nicolai.haehnle@amd.com>
>>
>> C11 threads were changed to use struct timespec instead of xtime, and
>> thrd_sleep got a second argument.
>>
> As xtime was replaced with timespec there's a couple of odd bits in the code.
> 
> 
>> See http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1554.htm and
>> http://en.cppreference.com/w/c/thread/{thrd_sleep,cnd_timedwait,mtx_timedlock}
>>
>> Note that cnd_timedwait is spec'd to be relative to TIME_UTC / CLOCK_REALTIME.
>>
>> Cc: Jose Fonseca <jfonseca@vmware.com>
>> ---
>>   include/c11/threads.h           | 11 -----------
>>   include/c11/threads_posix.h     | 39 +++++++++++++++------------------------
>>   include/c11/threads_win32.h     | 37 +++++++++++++++++++------------------
>>   src/egl/drivers/dri2/egl_dri2.c | 24 +++++++++++++-----------
>>   4 files changed, 47 insertions(+), 64 deletions(-)
>>
>> diff --git a/include/c11/threads.h b/include/c11/threads.h
>> index 573348d8091..3c3f23a8ab8 100644
>> --- a/include/c11/threads.h
>> +++ b/include/c11/threads.h
>> @@ -23,42 +23,31 @@
>>    * FITNESS FOR A PARTICULAR PURPOSE, TITLE AND NON-INFRINGEMENT. IN NO EVENT
>>    * SHALL THE COPYRIGHT HOLDERS OR ANYONE DISTRIBUTING THE SOFTWARE BE LIABLE
>>    * FOR ANY DAMAGES OR OTHER LIABILITY, WHETHER IN CONTRACT, TORT OR OTHERWISE,
>>    * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>>    * DEALINGS IN THE SOFTWARE.
>>    */
>>   #ifndef EMULATED_THREADS_H_INCLUDED_
>>   #define EMULATED_THREADS_H_INCLUDED_
>>
>>   #include <time.h>
>> -#ifdef _MSC_VER
>> -#include <thr/xtimec.h>  // for xtime
>> -#endif
>>
>>   #ifndef TIME_UTC
>>   #define TIME_UTC 1
>>   #endif
>>
>>   #include "c99_compat.h" /* for `inline` */
>>
>>   /*---------------------------- types ----------------------------*/
>>   typedef void (*tss_dtor_t)(void*);
>>   typedef int (*thrd_start_t)(void*);
>>
>> -#ifndef _MSC_VER
>> -struct xtime {
>> -    time_t sec;
>> -    long nsec;
>> -};
>> -typedef struct xtime xtime;
>> -#endif
>> -
> 
> We don't have a fall-back declaration of the struct, yet we use it
> below and provide a timespec_get() implementation.
> I'd imagine you haven't tested this on Windows (hence Jose in CC)?

Right on both counts, as I don't really have a way of testing on Windows.


> Quick search suggests that MSVC 2015 was the first one that introduces
> the struct and timespec_get.
> 
> If we're safe as-is, please add a comment with some details - I'd
> imagine Jose had better knowledge in the area.
> 
> 
>>   /*-------------------- 7.25.7 Time functions --------------------*/
>>   // 7.25.6.1
>> +#if 0
> I'd just drop the hunk mentioning that timespec_get() is part of time.h

I wasn't sure when timespec_get was introduced, so I thought it safer to 
keep it around for a while in case we run into systems where time.h 
doesn't provide it.

I can add a comment, or remove it entirely if you think that's not a 
concern.

Cheers,
Nicolai

> 
> 
> Thank
> Emil
>
On 23 October 2017 at 12:20, Nicolai Hähnle <nhaehnle@gmail.com> wrote:

>>> -#ifndef _MSC_VER
>>> -struct xtime {
>>> -    time_t sec;
>>> -    long nsec;
>>> -};
>>> -typedef struct xtime xtime;
>>> -#endif
>>> -
>>
>>
>> We don't have a fall-back declaration of the struct, yet we use it
>> below and provide a timespec_get() implementation.
>> I'd imagine you haven't tested this on Windows (hence Jose in CC)?
>
>
> Right on both counts, as I don't really have a way of testing on Windows.
>
There's an Appveyour integration similar to Travis. One should be able
to select the MSVC/toolchain version, but I'm not versed enough for
examples :-\
Let's see what Jose will say on the topic.

>
>> Quick search suggests that MSVC 2015 was the first one that introduces
>> the struct and timespec_get.
>>
>> If we're safe as-is, please add a comment with some details - I'd
>> imagine Jose had better knowledge in the area.
>>
>>
>>>   /*-------------------- 7.25.7 Time functions --------------------*/
>>>   // 7.25.6.1
>>> +#if 0
>>
>> I'd just drop the hunk mentioning that timespec_get() is part of time.h
>
>
> I wasn't sure when timespec_get was introduced, so I thought it safer to
> keep it around for a while in case we run into systems where time.h doesn't
> provide it.
>
> I can add a comment, or remove it entirely if you think that's not a
> concern.
>
Valid point - I'd assumed it was available everywhere (tm). I'd leave
it for now, thus we can toggle based on $heuristics if applicable.

-Emil
Am Dienstag, den 24.10.2017, 18:21 +0100 schrieb Emil Velikov:
> 
> > Right on both counts, as I don't really have a way of testing on
> > Windows.
> > 
> 
> There's an Appveyour integration similar to Travis. One should be
> able to select the MSVC/toolchain version, but I'm not versed enough
> for examples :-\
> Let's see what Jose will say on the topic.

Appveour seems to be pretty streightforward on how to define which MSVC
version to use, i.e. there is a line 

  os: Visual Studio 2013

and later one has to give the right number to scons (apparently
msvc2013 =12.0), but for the build a prebuild llvm version  

   llvm-3.3.1-msvc2013-mtd.7z

is pulled in, and I'm quite confident that linking against this with
any version of MSVC that is not "2013" will fail.

Best, 
Gert
On 23/10/17 20:15, Emil Velikov wrote:
> Hi Nicolai,
> 
> On 22 October 2017 at 20:07, Nicolai Hähnle <nhaehnle@gmail.com> wrote:
>> From: Nicolai Hähnle <nicolai.haehnle@amd.com>
>>
>> C11 threads were changed to use struct timespec instead of xtime, and
>> thrd_sleep got a second argument.
>>
> As xtime was replaced with timespec there's a couple of odd bits in the code.
> 
> 
>> See http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1554.htm and
>> http://en.cppreference.com/w/c/thread/{thrd_sleep,cnd_timedwait,mtx_timedlock}
>>
>> Note that cnd_timedwait is spec'd to be relative to TIME_UTC / CLOCK_REALTIME.
>>
>> Cc: Jose Fonseca <jfonseca@vmware.com>
>> ---
>>   include/c11/threads.h           | 11 -----------
>>   include/c11/threads_posix.h     | 39 +++++++++++++++------------------------
>>   include/c11/threads_win32.h     | 37 +++++++++++++++++++------------------
>>   src/egl/drivers/dri2/egl_dri2.c | 24 +++++++++++++-----------
>>   4 files changed, 47 insertions(+), 64 deletions(-)
>>
>> diff --git a/include/c11/threads.h b/include/c11/threads.h
>> index 573348d8091..3c3f23a8ab8 100644
>> --- a/include/c11/threads.h
>> +++ b/include/c11/threads.h
>> @@ -23,42 +23,31 @@
>>    * FITNESS FOR A PARTICULAR PURPOSE, TITLE AND NON-INFRINGEMENT. IN NO EVENT
>>    * SHALL THE COPYRIGHT HOLDERS OR ANYONE DISTRIBUTING THE SOFTWARE BE LIABLE
>>    * FOR ANY DAMAGES OR OTHER LIABILITY, WHETHER IN CONTRACT, TORT OR OTHERWISE,
>>    * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>>    * DEALINGS IN THE SOFTWARE.
>>    */
>>   #ifndef EMULATED_THREADS_H_INCLUDED_
>>   #define EMULATED_THREADS_H_INCLUDED_
>>
>>   #include <time.h>
>> -#ifdef _MSC_VER
>> -#include <thr/xtimec.h>  // for xtime
>> -#endif
>>
>>   #ifndef TIME_UTC
>>   #define TIME_UTC 1
>>   #endif
>>
>>   #include "c99_compat.h" /* for `inline` */
>>
>>   /*---------------------------- types ----------------------------*/
>>   typedef void (*tss_dtor_t)(void*);
>>   typedef int (*thrd_start_t)(void*);
>>
>> -#ifndef _MSC_VER
>> -struct xtime {
>> -    time_t sec;
>> -    long nsec;
>> -};
>> -typedef struct xtime xtime;
>> -#endif
>> -
> 
> We don't have a fall-back declaration of the struct, yet we use it
> below and provide a timespec_get() implementation.
> I'd imagine you haven't tested this on Windows (hence Jose in CC)?
> 
> Quick search suggests that MSVC 2015 was the first one that introduces
> the struct and timespec_get.
> 
> If we're safe as-is, please add a comment with some details - I'd
> imagine Jose had better knowledge in the area.
> 
> 
>>   /*-------------------- 7.25.7 Time functions --------------------*/
>>   // 7.25.6.1
>> +#if 0
> I'd just drop the hunk mentioning that timespec_get() is part of time.h
> 
> 
> Thank
> Emil

If there's doubt, I suggest testing Visual Studio with AppVeyor by 
pushing the changes as a feature branch to FDO's git -- I believe that 
should trigger an AppVeyor build.  (Push to a github repos hooked into 
Appveyor, depending on what people are more confortable with.)

Alternatively reach out to Brian or Roland.

I'm currently on PTO, so I'm afraid I don't have the time nor a devel 
setup to try this out..

Jose
On 26/10/17 13:55, Jose Fonseca wrote:
> On 23/10/17 20:15, Emil Velikov wrote:
>> Hi Nicolai,
>>
>> On 22 October 2017 at 20:07, Nicolai Hähnle <nhaehnle@gmail.com> wrote:
>>> From: Nicolai Hähnle <nicolai.haehnle@amd.com>
>>>
>>> C11 threads were changed to use struct timespec instead of xtime, and
>>> thrd_sleep got a second argument.
>>>
>> As xtime was replaced with timespec there's a couple of odd bits in 
>> the code.
>>
>>
>>> See http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1554.htm and
>>> http://en.cppreference.com/w/c/thread/{thrd_sleep,cnd_timedwait,mtx_timedlock} 
>>>
>>>
>>> Note that cnd_timedwait is spec'd to be relative to TIME_UTC / 
>>> CLOCK_REALTIME.
>>>
>>> Cc: Jose Fonseca <jfonseca@vmware.com>
>>> ---
>>>   include/c11/threads.h           | 11 -----------
>>>   include/c11/threads_posix.h     | 39 
>>> +++++++++++++++------------------------
>>>   include/c11/threads_win32.h     | 37 
>>> +++++++++++++++++++------------------
>>>   src/egl/drivers/dri2/egl_dri2.c | 24 +++++++++++++-----------
>>>   4 files changed, 47 insertions(+), 64 deletions(-)
>>>
>>> diff --git a/include/c11/threads.h b/include/c11/threads.h
>>> index 573348d8091..3c3f23a8ab8 100644
>>> --- a/include/c11/threads.h
>>> +++ b/include/c11/threads.h
>>> @@ -23,42 +23,31 @@
>>>    * FITNESS FOR A PARTICULAR PURPOSE, TITLE AND NON-INFRINGEMENT. IN 
>>> NO EVENT
>>>    * SHALL THE COPYRIGHT HOLDERS OR ANYONE DISTRIBUTING THE SOFTWARE 
>>> BE LIABLE
>>>    * FOR ANY DAMAGES OR OTHER LIABILITY, WHETHER IN CONTRACT, TORT OR 
>>> OTHERWISE,
>>>    * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE 
>>> USE OR OTHER
>>>    * DEALINGS IN THE SOFTWARE.
>>>    */
>>>   #ifndef EMULATED_THREADS_H_INCLUDED_
>>>   #define EMULATED_THREADS_H_INCLUDED_
>>>
>>>   #include <time.h>
>>> -#ifdef _MSC_VER
>>> -#include <thr/xtimec.h>  // for xtime
>>> -#endif
>>>
>>>   #ifndef TIME_UTC
>>>   #define TIME_UTC 1
>>>   #endif
>>>
>>>   #include "c99_compat.h" /* for `inline` */
>>>
>>>   /*---------------------------- types ----------------------------*/
>>>   typedef void (*tss_dtor_t)(void*);
>>>   typedef int (*thrd_start_t)(void*);
>>>
>>> -#ifndef _MSC_VER
>>> -struct xtime {
>>> -    time_t sec;
>>> -    long nsec;
>>> -};
>>> -typedef struct xtime xtime;
>>> -#endif
>>> -
>>
>> We don't have a fall-back declaration of the struct, yet we use it
>> below and provide a timespec_get() implementation.
>> I'd imagine you haven't tested this on Windows (hence Jose in CC)?
>>
>> Quick search suggests that MSVC 2015 was the first one that introduces
>> the struct and timespec_get.
>>
>> If we're safe as-is, please add a comment with some details - I'd
>> imagine Jose had better knowledge in the area.
>>
>>
>>>   /*-------------------- 7.25.7 Time functions --------------------*/
>>>   // 7.25.6.1
>>> +#if 0
>> I'd just drop the hunk mentioning that timespec_get() is part of time.h
>>
>>
>> Thank
>> Emil
> 
> If there's doubt, I suggest testing Visual Studio with AppVeyor by 
> pushing the changes as a feature branch to FDO's git -- I believe that 
> should trigger an AppVeyor build.  (Push to a github repos hooked into 
> Appveyor, depending on what people are more confortable with.)
> 
> Alternatively reach out to Brian or Roland.
> 
> I'm currently on PTO, so I'm afraid I don't have the time nor a devel 
> setup to try this out..
> 
> Jose
I forgot to say: assuming we don't need xtime anymore, at a glance, the 
patch looks ok to me.


Jose
Am Donnerstag, den 26.10.2017, 13:55 +0900 schrieb Jose Fonseca:
> 
> 
> If there's doubt, I suggest testing Visual Studio with AppVeyor by 
> pushing the changes as a feature branch to FDO's git -- I believe
> that should trigger an AppVeyor build.  (Push to a github repos
> hooked into Appveyor, depending on what people are more confortable
> with.)

Since I have AppVeyor set up, I tested it, but I applied  only this
patch (the series didn't apply on top of 16cfbef44cf0b1969).  

The build fails though, I guess since the setup is with MSVC 2013. 

I'll happily trigger another build, if someone points me to an AppVeyor
file that uses a later version of MSVC (that would probably require
LLVM build with that other version). 

Build log: 

https://ci.appveyor.com/project/gerddie/mesa/build/65

lp_context.c
c:\projects\mesa\include\c11\threads_win32.h(151) : error C2037: left
of 'tv_sec' specifies undefined struct/union 'timespec'
c:\projects\mesa\include\c11\threads_win32.h(151) : error C2037: left
of 'tv_nsec' specifies undefined struct/union 'timespec'
c:\projects\mesa\include\c11\threads_win32.h(151) : warning C4033:
'impl_timespec2msec' must return a value
c:\projects\mesa\include\c11\threads_win32.h(389) : error C2065: 'ts' :
undeclared identifier
c:\projects\mesa\include\c11\threads_win32.h(389) : warning C4047:
'function' : 'const timespec *' differs in levels of indirection from
'int'
c:\projects\mesa\include\c11\threads_win32.h(389) : 

Best, 
Gert
On 10/25/2017 10:55 PM, Jose Fonseca wrote:
> On 23/10/17 20:15, Emil Velikov wrote:
>> Hi Nicolai,
>>
>> On 22 October 2017 at 20:07, Nicolai Hähnle <nhaehnle@gmail.com> wrote:
>>> From: Nicolai Hähnle <nicolai.haehnle@amd.com>
>>>
>>> C11 threads were changed to use struct timespec instead of xtime, and
>>> thrd_sleep got a second argument.
>>>
>> As xtime was replaced with timespec there's a couple of odd bits in
>> the code.
>>
>>
>>> See http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1554.htm and
>>> http://en.cppreference.com/w/c/thread/{thrd_sleep,cnd_timedwait,mtx_timedlock}
>>>
>>>
>>> Note that cnd_timedwait is spec'd to be relative to TIME_UTC /
>>> CLOCK_REALTIME.
>>>
>>> Cc: Jose Fonseca <jfonseca@vmware.com>
>>> ---
>>>   include/c11/threads.h           | 11 -----------
>>>   include/c11/threads_posix.h     | 39
>>> +++++++++++++++------------------------
>>>   include/c11/threads_win32.h     | 37
>>> +++++++++++++++++++------------------
>>>   src/egl/drivers/dri2/egl_dri2.c | 24 +++++++++++++-----------
>>>   4 files changed, 47 insertions(+), 64 deletions(-)
>>>
>>> diff --git a/include/c11/threads.h b/include/c11/threads.h
>>> index 573348d8091..3c3f23a8ab8 100644
>>> --- a/include/c11/threads.h
>>> +++ b/include/c11/threads.h
>>> @@ -23,42 +23,31 @@
>>>    * FITNESS FOR A PARTICULAR PURPOSE, TITLE AND NON-INFRINGEMENT. IN
>>> NO EVENT
>>>    * SHALL THE COPYRIGHT HOLDERS OR ANYONE DISTRIBUTING THE SOFTWARE
>>> BE LIABLE
>>>    * FOR ANY DAMAGES OR OTHER LIABILITY, WHETHER IN CONTRACT, TORT OR
>>> OTHERWISE,
>>>    * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
>>> USE OR OTHER
>>>    * DEALINGS IN THE SOFTWARE.
>>>    */
>>>   #ifndef EMULATED_THREADS_H_INCLUDED_
>>>   #define EMULATED_THREADS_H_INCLUDED_
>>>
>>>   #include <time.h>
>>> -#ifdef _MSC_VER
>>> -#include <thr/xtimec.h>  // for xtime
>>> -#endif
>>>
>>>   #ifndef TIME_UTC
>>>   #define TIME_UTC 1
>>>   #endif
>>>
>>>   #include "c99_compat.h" /* for `inline` */
>>>
>>>   /*---------------------------- types ----------------------------*/
>>>   typedef void (*tss_dtor_t)(void*);
>>>   typedef int (*thrd_start_t)(void*);
>>>
>>> -#ifndef _MSC_VER
>>> -struct xtime {
>>> -    time_t sec;
>>> -    long nsec;
>>> -};
>>> -typedef struct xtime xtime;
>>> -#endif
>>> -
>>
>> We don't have a fall-back declaration of the struct, yet we use it
>> below and provide a timespec_get() implementation.
>> I'd imagine you haven't tested this on Windows (hence Jose in CC)?
>>
>> Quick search suggests that MSVC 2015 was the first one that introduces
>> the struct and timespec_get.
>>
>> If we're safe as-is, please add a comment with some details - I'd
>> imagine Jose had better knowledge in the area.
>>
>>
>>>   /*-------------------- 7.25.7 Time functions --------------------*/
>>>   // 7.25.6.1
>>> +#if 0
>> I'd just drop the hunk mentioning that timespec_get() is part of time.h
>>
>>
>> Thank
>> Emil
>
> If there's doubt, I suggest testing Visual Studio with AppVeyor by
> pushing the changes as a feature branch to FDO's git -- I believe that
> should trigger an AppVeyor build.  (Push to a github repos hooked into
> Appveyor, depending on what people are more confortable with.)
>
> Alternatively reach out to Brian or Roland.

I applied this patch to today's ToT and got some compiler errors:


scons: Building targets ...
cl /Fobuild\windows-x86-debug\gallium\tests\unit\pipe_barrier_test.obj 
/c src\ga
llium\tests\unit\pipe_barrier_test.c /nologo /Od /Oi /Oy- /W3 /wd4018 
/wd4056 /w
d4244 /wd4267 /wd4305 /wd4351 /wd4756 /wd4800 /wd4996 /arch:SSE2 /MTd 
-FIinttype
s.h /D__STDC_CONSTANT_MACROS /D__STDC_FORMAT_MACROS 
/D__STDC_LIMIT_MACROS /DHAVE
_NO_AUTOCONF /DDEBUG /DWIN32 /D_WINDOWS /D_WIN32_WINNT=0x0601 
/DWINVER=0x0601 /D
VC_EXTRALEAN /D_USE_MATH_DEFINES /D_CRT_SECURE_NO_WARNINGS 
/D_CRT_SECURE_NO_DEPR
ECATE /D_SCL_SECURE_NO_WARNINGS /D_SCL_SECURE_NO_DEPRECATE 
/D_ALLOW_KEYWORD_MACR
OS /D_HAS_EXCEPTIONS=0 /D_DEBUG /DPIPE_SUBSYSTEM_WINDOWS_USER 
/DPACKAGE_VERSION=
\"17.4.0-devel\" 
/DPACKAGE_BUGREPORT=\"https://bugs.freedesktop.org/enter_bug.cg
i?product=Mesa\" /Iinclude /Isrc\gallium\include /Isrc\gallium\auxiliary 
/Isrc\g
allium\drivers /Isrc\gallium\winsys /Ibuild\windows-x86-debug /Isrc 
/Isrc /Ibuil
d\windows-x86-debug\gallium\tests\unit\indices 
/Isrc\gallium\tests\unit\indices
/Ibuild\windows-x86-debug\gallium\tests\unit\util 
/Isrc\gallium\tests\unit\util
/Z7
pipe_barrier_test.c
c:\users\brian\projects\mesa\include\c11\threads_win32.h(389): error 
C2065: 'ts'
: undeclared identifier
c:\users\brian\projects\mesa\include\c11\threads_win32.h(389): warning 
C4047: 'f
unction': 'const timespec *' differs in levels of indirection from 'int'
c:\users\brian\projects\mesa\include\c11\threads_win32.h(389): warning 
C4024: 'i
mpl_cond_do_wait': different types for formal and actual parameter 3
c:\users\brian\projects\mesa\include\c11\threads_win32.h(639): error 
C2084: func
tion 'int timespec_get(timespec *const ,const int)' already has a body
C:\Program Files\Windows Kits\10\include\10.0.10240.0\ucrt\time.h(539): 
note: se
e previous definition of 'timespec_get'
scons: *** 
[build\windows-x86-debug\gallium\tests\unit\pipe_barrier_test.obj] Er
ror 2
scons: building terminated because of errors.


I don't have time to investigate/fix.  Maybe you can, Nicolai.

-Brian