<div dir="ltr"><p>Hello Everyone, especially Windows developers.</p><div>This patch attempts to clean up libcxx's intrinsic support for Windows a little bit for when building with Visual Studio's cl.exe compiler.</div>
<div> </div><div><div>I'd appreciate it if any Win32 experts out their could review this patch (or try it, see test case and notes at the end!) as the regular expert in residence, Reid, might not be available and I am keen to change these files further once this patch is committed. Windows reviewers for libcxx actually using Visual Studio seem in short supply (probably because libcxx on Windows doesn't quite compile yet, but nearly).</div>
</div><p>There are several elements to this patch (least to most important):</p><p>1. Just some simple tidy ups to move the include's to the start of the file where people expect them.</p><p>2. Some other clarity changes:<br>
On Windows, __builtin_ctzl(unsigned long) was implemented by calling _builtin_ctz(unsinged int). same for clzl calling clz.<br>This doesn't quite make sense, conceptually at least, because sizeof(unsigned int) <= sizeof(unsigned long).<br>
It makes more sense to implement things the other way around and have ctz call ctzl etc. if at all.<br>So this patch makes that change.<br>Practically, it makes no difference on Windows (which is all this post relates to) because sizeof(long) == sizeof(int) but I find it clearer.</p>
<div>3. Fixes to make __builtin_ctzll and __builtin_clzll available for 32 bit compiles.</div><div>These were implemented using the _BitScanForward64 and _BitScanReverse64 functions. The problem with this is that the ll routines are available in 32 and 64 bit compilers but that xxxx64 intriniscs they depend on are only available in 64bit compiler. This meant 32 bit compiles got errors because _BitScanForward64 and _BitScanReverse64 vanished in 32 bit builds.</div>
<div>This patch solves that problem by implementing new functions _libcpp_bsf64 and _libcpp_bsr64 that provide the exact same functionality of _BitScanForward64 and _BitScanReverse64 and making clzll and ctzll depend on them as these new routines exist in both 32bit and 64 bit compilations. The _libcpp_bsf64/_libcpp_bsr64 functions are implemented by calling the _BitScanReverseForward64 and _BitScanReverse64 directly in 64 bit mode as they are available, however in 32 bit mode they emulate the 64 bit intrinsics by calling their 32 bit counter parts _BitScanForward/Reverse instead.</div>
<div> </div><div>(FYI: It might be possible to name _libcpp_bsf64 and _libcpp_bsr64 as _BitScanReverseForward64 and _BitScanReverse64 in 64 bit mode but this would just encourage code that would compile under libcxx but that would fail under WIN64 without libcxx so this path was not taken.)</div>
<div> </div><div>4. Some bug fixes to the above routines.</div><div>In the process of examining this code I discovered that the original author understandably may have assumed that the _BitScanReverse/Forward/Reverse/64 functions returned offsets compatible with the ctz/l/ll and clz/l/ll functions as they were returning the untouched indexes directly. However this is incorrect the index needs further manipulation to calculate the correct result. This patch corrects these bugs.</div>
<div> </div><p>5. It also introduces a few comments regarding further possible changes in the future, some of which I hope to do next.</p><div> </div><div>6. Test case and Windows Reviewer info: </div><div> </div><div>If it's any assistance to those who might review this code, here are relevant notes from the publicly available docs. More info on these routines is easy to find.</div>
<div><p>— Built-in Function: int __builtin_clz (unsigned int x)<br>Returns the number of leading 0-bits in x, starting at the most significant bit position. If x is 0, the result is undefined</p><p>— Built-in Function: int __builtin_ctz (unsigned int x)<br>
Returns the number of trailing 0-bits in x, starting at the least significant bit position. If x is 0, the result is undefined. </p><p>_BitScanForward, _BitScanForward64<br>Search the mask data from least significant bit (LSB) to the most significant bit (MSB) for a set bit (1).</p>
<p>_BitScanReverse, _BitScanReverse64<br>Search the mask data from most significant bit (MSB) to least significant bit (LSB) for a set bit (1).</p></div><p> </p><div>Attached test case</div><div>-------------------------</div>
<div> </div><div>The attached test case somewhat mitigates reviewer effort. If compiled and run, it produces an error message if there is a problem testing these functions or and on success it is silent.</div><div> </div>
<div>It compiles as is with clang++ and g++ and but MS's compiler needs more effort. As is cl.exe generates an error compiling the test case because it can't handle libcxx's <type_traits> which libcxx's pulls in indirectly via <limits>.</div>
<div> </div><div>If you try, you will get this error:</div><div>----</div><div>/libcxx/include\type_traits(927) : error C2681: 'add_rvalue_reference<_Tp*>::type' : invalid expression type for dynamic_c<br>ast<br>
/libcxx/include\type_traits(1404) : error C2516: 'std::common_type<_Tp>::type' : is not a legal base class</div><p>If you look at line 927 of <type_traits></p><div>template<typename _Tp> char &__is_polymorphic_impl(<br>
    typename enable_if<sizeof((_Tp*)dynamic_cast<const volatile void*>(declval<_Tp*>())) != 0,<br>                       int>::type);</div><div>----</div><div> </div><div>Any advice on how to fix this error would be greatly appreciated. There is no known work around from the people I have spoke to.</div>
<div> </div><div>However, if you are a willing reviewer using Visual Studio who has libcxx from trunk on your machine (you don't need to compile libcxx into library (you can't anyway), this is a header only fix, try this:</div>
<div> </div><div>* Ensure a test copy of libcxx is on your machine from trunk in c:\libcxx (or you will need to edit the test case to say where you put it).</div><div>* Edit bstestcase.cpp and uncomment the two includes on the first few lines of the program.</div>
<div>* Try compiling bstestcase.cpp in 32 bit mode against the old libcxx (prior to applying this patch), it will fail to compile because BitScanReverse64 and BitScanForward64 are missing. If so, you have demonstrated one claimed fault from above.</div>
<div>* Try then compiling with the 64 bit compiler. It should compile. Now Run it. You should see an assertion error. If so you have demonstrated the bug in the intrinsic's that I am trying to fix.</div><div>* Now apply the patch and try again, compile in both 32 bit mode and 64 bit mode. You should find the test case works in both cases.</div>
<div>* If you tried all that without error, and no one has written in with and LGTM already, please send an LGTM to cfe-commits!</div><div> </div><div>If you are even as smart as to fix the dynamic_cast problem of type_traits, buy yourself a beers (ok two) and try compiling the test case again with the includes you uncommented re-commented out. The test case should work in that event.</div>
<div> </div><div><div>If any reviewer gets stuck and wants to email me privately, I'll try and help. You may need other patches of mine not yet committed to make the above patch work but that shouldn't be the case. If you do, they are listed below. They are getting a bit stale so they might not stick. They have been LGTM'd by Reid but not yet had final approval/committal by Howard so they are not in libcxx yet. People experimenting with Visual Studio and libcxx may find them useful though in the mean time while they await that.</div>
<div> </div><div><a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130916/089252.html" target="_blank"><font color="#ac193d">http://lists.cs.uiuc.edu/</font><font color="#ac193d">pipermail/cfe-commits/Week-of-</font><font color="#ac193d">Mon-20130916/089252.html</font></a><br>
 chrono:<br><a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130916/089222.html" target="_blank"><font color="#ac193d">http://lists.cs.uiuc.edu/</font><font color="#ac193d">pipermail/cfe-commits/Week-of-</font><font color="#ac193d">Mon-20130916/089222.html</font></a><br>
rtti:<br><a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130923/089284.html" target="_blank"><font color="#ac193d">http://lists.cs.uiuc.edu/</font><font color="#ac193d">pipermail/cfe-commits/Week-of-</font><font color="#ac193d">Mon-20130923/089284.html</font></a><br>
 inline warnings<br><a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130923/089288.html" target="_blank"><font color="#ac193d">http://lists.cs.uiuc.edu/</font><font color="#ac193d">pipermail/cfe-commits/Week-of-</font><font color="#ac193d">Mon-20130923/089288.html</font></a><br>
pragma visibility<br><a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130923/089436.html" target="_blank"><font color="#ac193d">http://lists.cs.uiuc.edu/</font><font color="#ac193d">pipermail/cfe-commits/Week-of-</font><font color="#ac193d">Mon-20130923/089436.html</font></a><br>
 _LIBCPP_WEAK<br><a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130923/089456.html" target="_blank"><font color="#ac193d">http://lists.cs.uiuc.edu/</font><font color="#ac193d">pipermail/cfe-commits/Week-of-</font><font color="#ac193d">Mon-20130923/089456.html</font></a></div>
<div>cmake turn of all warnings for msvc</div><div><a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130923/089608.html">http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130923/089608.html</a></div>
<div> </div><div>FYI, if you have applied all the above patches and they have taken. libcxx should compile with Visual Studio 2013RC with the error count being reduced to about 135 errors and about 80 warnings. The bulk of not easily fixable errors are down to dynamic_cast, and missing headers for pthread and atomics for VC. The errors other than those, I hope to look at next if this lot is applied.</div>
<div> </div></div><div>Thanks!<br></div></div>