[Mesa-dev] [PATCH] c11/threads: initialize timeout structure

[Emil Velikov]

On 6 October 2015 at 21:39, Jose Fonseca <jfonseca at vmware.com> wrote:
> On 06/10/15 20:30, Ian Romanick wrote:
>>
>> On 10/06/2015 12:04 PM, Matt Turner wrote:
>>>
>>> On Sat, Oct 3, 2015 at 5:19 PM, Jan Vesely <jano.vesely at gmail.com> wrote:
>>>>
>>>> Signed-off-by: Jan Vesely <jano.vesely at gmail.com>
>>>> ---
>>>>   include/c11/threads_posix.h | 6 ++++++
>>>>   1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/include/c11/threads_posix.h b/include/c11/threads_posix.h
>>>> index 3def6c4..ce9853b 100644
>>>> --- a/include/c11/threads_posix.h
>>>> +++ b/include/c11/threads_posix.h
>>>> @@ -136,8 +136,14 @@ cnd_timedwait(cnd_t *cond, mtx_t *mtx, const xtime
>>>> *xt)
>>>
>>>
>>> I'm confused. The docs here [1] give a different prototype for this
>>> function -- one without the xt argument. Is our implementation for
>>> some pre-C11 spec?
>>
>>
>> It's not without the xt argument... it's with that argument having a
>> different (const struct timespec* restrict time_point) type.  It looks
>> like mtx_timedlock (http://en.cppreference.com/w/c/thread/mtx_timedlock)
>> has the same issue.
>>
>> As far as I can tell, C11 struct timespec
>> (http://en.cppreference.com/w/c/chrono/timespec) is the same as Linux
>> struct timespec.  POSIX
>> (http://pubs.opengroup.org/onlinepubs/009695399/basedefs/time.h.html)
>> allows it to have extra fields, but I don't think that should be a
>> problem.  Maybe there's a conflict on Windows?  Maybe Jose knows?
>>
>>> [1] http://en.cppreference.com/w/c/thread/cnd_timedwait
>
>
> No, that prototype comes from the original source code [1], so I don't know
> the background story behind it (ie, if it's because it was based on an
> ealier C11 draft, or due to crossplatform reasons.
>
Side note - the original authors said "If you need a stable code,
please refer to $mesa_tree", when I reported an earlier bug :-)

>
> AFAICT, there's no conflict on Windows.  However, MSVC doesn't define
> timespec, while MinGW (at least some versions) do.
>
>
> In short, we could use "timespec" everywhere, relying on time.h definition
> on Linux, and defining our own struct timespec for Windows when not
> available.
>
>
> That is, some like:
>
> diff --git a/include/c11/threads.h b/include/c11/threads.h
> index 45823df..c8b3819 100644
> --- a/include/c11/threads.h
> +++ b/include/c11/threads.h
> @@ -41,11 +41,17 @@
>  typedef void (*tss_dtor_t)(void*);
>  typedef int (*thrd_start_t)(void*);
>
> -struct xtime {
> -    time_t sec;
> -    long nsec;
> +#if defined(_MSC_VER) || (defined(__MINGW__) &&
> !defined(___TIMESPEC_DEFINED))
> +
> +struct timespec {
> +    time_t tv_sec;
> +    long tv_nsec;
>  };
> -typedef struct xtime xtime;
> +
> +#endif
> +
> +// FIXME: actually rename `xtime` with `struct timespec`, sec -> tv_sec,
> etc.
> +#define xtime struct timespec
>
This hunk look great afaict.
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

-Emil