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 > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
