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

Tue Oct 15 01:07:55 PDT 2013

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

Yes, please. It's impossible to review now.

> * 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
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>