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

[David Majnemer]

On Monday, October 14, 2013, G M wrote:

> 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.
>

Strong objection to this. Two reasons:
1. Vista+ has it's own condition variables reader-writer locks, no need to
invent our own on that platform.
2. The environment reserves POSIX names, we are not in the business of
implementing POSIX on non-POSIX platforms and windows reserves the right to
one day implement 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/20131014/35b45b5e/attachment.html>