[PATCH] [libcxx] Windows port of thread/mutex/chrono - initial review.

[G M]

Hi Everyone (esp Nico and pthread users)
I've just applied the chrono / thread patch here:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131007/090709.html

and am offering an initial opinion/review. I haven't yet run the code
seriously and looked at in detail but I have at had a bit of a look,
applied it and tried to run the libcxx test suite with it in. I hope to
return with more thoughts as I get more time to spend on it. I'm pretty
keen for it to make it in.

Here's my initial thoughts:

First, there might be a few small compiler platform mistakes. I couldn't
compile "as is" without compile error. But they weren't hard to fix.

* __mutex_base - I need the __yield function to be something like this for
me to compile, otherwise I get compiler errors:

inline _LIBCPP_INLINE_VISIBILITY
void __yield() _NOEXCEPT
{
#if defined(_LIBCPP_PTHREADS)
    sched_yield();
#else
    __thread_yield();
#endif
}

* futex.cpp - I needed to change 'value' to 'desired' in non __clang mode.
'value' is undefined in the cfe-commits version and probably just reflects
you haven't run or been able to run that path just yet.

_LIBCPP_INLINE_VISIBILITY
static void atomic_store_seq4(
    _Atomic(unsigned)* obj, unsigned desired)
{
#ifdef __clang__
    __c11_atomic_store(obj, desired, std::memory_order_seq_cst);
#else
    _InterlockedExchange((volatile long*)obj, desired); // need desired
here not value.
#endif
}

* Futex.cpp - good to fix these warnings up:

futex.cpp(103): warning C4800: 'BOOLEAN' : forcing value to bool 'true' or
'false' (performance warning)
futex.cpp(147): warning C4146: unary minus operator applied to unsigned
type, result still unsigned
futex.cpp(232): warning C4146: unary minus operator applied to unsigned
type, result still unsigned

* General statement: Can we follow the pthread model more closely here for
libcxx's Win32 path. Is there any good reason not to? i.e. with futex
becoming pthread_mutex etc. etc.? I think pthreads is a pretty tight and
well known interface and I think we should aim to have a default
implementation in libcxx that is easily swappable for anyone else's
implementation or even if it's not hot swappable so to speak. have the
win32 model more closely follow it.

I appreciate there will be some areas where pthread might not exactly match
or we need to break or extend the model, perhaps with regard to native
thread id's or something that pthread's doesn't/can't clearly model, but it
would be nice to have libcxx's code structured more so that the Win32
implementation of pthread looks more like all the other pthread
implementations i've seen. I think that would aid understanding and clean
up/clarify the interface more and ideally enable a plug in type possibility
where others can use winpthreads or libcxx_winpthreads, or brand x pthreads
with more ease and that everything appears to follow that similar model.

The idea, ideally, being that libcxx cmakelists.txt would be structured to
just have a pthread options to say what library libcxx will use, with
it's own one being "just" another option, possibly pulled in via
loadlibrary, maybe not. Just speculating at this stage..

I'm not saying that's essential but generally just getting the proposed
implementation for Win32 nearer to the common pthread model in name and
structure seems to me to add clarity to me and clean separation. It might
also get us to a point where there's less of a #if _LIBCPP_PTHREAD one
path, #else something quite diferent but not, which appears to be currently
the case. It might be fine the way it is, but as an early review that's a
discussion point I think.
The patch needs some refactoring to do that but in many cases it appears
the code is all there, it's "just" about putting that code behind an
interface that is structued and named the same way as the regular pthreads
path. etc. It may need some "extensions" in some points to do what we want,
but that hopefully doesn't deflate the generally point of getting more
similar to a "regular" pthread interface.

I attempted to hack another third party pthreads library I'm using for
testing until I saw the code I'm reviewing now. I would be happy to try to
get that working with the proposed code to just see if we have things has
flexible as we could. I don't intend to use it that other library, I expect
to use the on I'm reviewing here if it were refactored a little in the ways
I'm saying, but getting some other implementations to work (which I'm
proposing to help with) that the one i'm reviewing here would help test if
the flexibility is there and let others use these other implementations if
they want.

* The _LIBCPP_PTHREADS macro name is a little confusing to me. I think it
means, "use the existing path that say libcxx uses for pthreads today,
otherwise using the new / built in one". That's almost feels contradictory
to what I'd expect, but I'm not sure, so I'll just raise the subject and
see what others say. It'd be great if such a macro isn't needed or if we
can document what we are saying with such macros. But generally if the the
system can detect known pthread implementations and play with them whilst
offering a path for something that is unknown but "conforming" that would
be great. If this is all pie and the sky that's fine, but it's a discussion
point again.

* The inline assembler might be able to be factored out, or out more:
The win32 platform and clang appear to support the intrinsic bit test
functions and see their are psdk intrinsics that provide the same thing. I
don't think it needs the assembler but I might be wrong; or in less cases.
* On my machine the wait_for.pass.cpp file from the test suite stalled and
entered and endless loop. I don't know if that's related to your changes or
mine, I suspect mine actually, so I'll look into that myself. Nico, I'd be
curious if it runs for you with your patch applied and the win32 path in
use; I'll look at it more myself though and report back when I get time.

* Some thought / additional code might be required in cmakelists.txt to
make all this work properly i.e. finding the right headers / libraries,
making sure the additional required libraries get brought in for pthreads.
I haven't got that far yet. On my machine I get missing symbols with this
patch re. NtXXX functions. They aren't hard to resolve, but it would appear
(if I'm correct) that by default these libraries aren't wired up for msvc
etc yet.

* Phabricator. This patch might benefit from being discussed there. I
haven't used it before nor have an account yet but I'll get one if that's a
shared opinion. What do people think?
* In conclusion, I'm looking forward to a revised version of this patch
landing and using it. I think the implementation could be factored out of
libcxx and/or made more "conforming" to regular pthread and that would
appear useful from practical and clarity perspectives. If libcxx could be
made to use an interchangable implementation that might be nice. I'm
not tied to any necessity of that but getting the internal model closer
before acceptance seem of value. Something is better than nothing here as
long as it works though would be my guess. I would be happy to integrate to
that resulting effort some work I did integrating another pthread library
to test libccx's flexibility for such changes though I don't propose to use
the library I'm thinking of, I'd use the one proposed that I'm reviewing
here. i'd like to see it get in.

I hope these early comments are of value and not too far off the mark..

Thanks
Glen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131015/cd6c015e/attachment.html>