<br><br>On Monday, October 14, 2013, G M  wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><p>Hi Everyone (esp Nico and pthread users)</p><div>I've just applied the chrono / thread patch here:</div>
<div><a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131007/090709.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131007/090709.html</a></div>
<div> </div><div>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.</div>

<div> </div><div>Here's my initial thoughts:</div><p>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.</p>
<p>
* __mutex_base - I need the __yield function to be something like this for me to compile, otherwise I get compiler errors:</p>
<p>inline _LIBCPP_INLINE_VISIBILITY<br>void __yield() _NOEXCEPT<br>{<br>#if defined(_LIBCPP_PTHREADS)<br>    sched_yield();<br>#else<br>    __thread_yield();<br>#endif<br>}</p><p>* 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.</p>


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


#endif<br>}</p><p>* Futex.cpp - good to fix these warnings up:</p><p>futex.cpp(103): warning C4800: 'BOOLEAN' : forcing value to bool 'true' or 'false' (performance warning)<br>futex.cpp(147): warning C4146: unary minus operator applied to unsigned type, result still unsigned<br>


futex.cpp(232): warning C4146: unary minus operator applied to unsigned type, result still unsigned</p><p>* 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.</p>
</div></blockquote><div><br></div><div>Strong objection to this. Two reasons:</div><div>1. Vista+ has it's own condition variables reader-writer locks, no need to invent our own on that platform.</div><div>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.</div>
<br><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">

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


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


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


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

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


<p>* 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.</p>


<p>* The inline assembler might be able to be factored out, or out more:<br>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.</p>


<div>* 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.</div>

<div> </div><div>* 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.</div>


<p>* 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?</p><div>* 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.</div>

<div> </div><div>I hope these early comments are of value and not too far off the mark..</div><div> </div>
<div>Thanks<br>Glen</div><div><br> </div></div>
</blockquote>