[patch] [libcxx] win32 support, h intrinsic fixes.for cl/visual studio

G M gmisocpp at gmail.com
Wed Oct 2 05:18:45 PDT 2013 

Hello Everyone, especially Windows developers.
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.

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

There are several elements to this patch (least to most important):

1. Just some simple tidy ups to move the include's to the start of the file
where people expect them.

2. Some other clarity changes:
On Windows, __builtin_ctzl(unsigned long) was implemented by calling
_builtin_ctz(unsinged int). same for clzl calling clz.
This doesn't quite make sense, conceptually at least, because
sizeof(unsigned int) <= sizeof(unsigned long).
It makes more sense to implement things the other way around and have ctz
call ctzl etc. if at all.
So this patch makes that change.
Practically, it makes no difference on Windows (which is all this post
relates to) because sizeof(long) == sizeof(int) but I find it clearer.
3. Fixes to make __builtin_ctzll and __builtin_clzll available for 32 bit
compiles.
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.
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.

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

4. Some bug fixes to the above routines.
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.


5. It also introduces a few comments regarding further possible changes in
the future, some of which I hope to do next.

6. Test case and Windows Reviewer info:

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.

— Built-in Function: int __builtin_clz (unsigned int x)
Returns the number of leading 0-bits in x, starting at the most significant
bit position. If x is 0, the result is undefined

— Built-in Function: int __builtin_ctz (unsigned int x)
Returns the number of trailing 0-bits in x, starting at the least
significant bit position. If x is 0, the result is undefined.

_BitScanForward, _BitScanForward64
Search the mask data from least significant bit (LSB) to the most
significant bit (MSB) for a set bit (1).

_BitScanReverse, _BitScanReverse64
Search the mask data from most significant bit (MSB) to least significant
bit (LSB) for a set bit (1).


Attached test case
-------------------------

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.

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

If you try, you will get this error:
----
/libcxx/include\type_traits(927) : error C2681:
'add_rvalue_reference<_Tp*>::type' : invalid expression type for dynamic_c
ast
/libcxx/include\type_traits(1404) : error C2516:
'std::common_type<_Tp>::type' : is not a legal base class

If you look at line 927 of <type_traits>
template<typename _Tp> char &__is_polymorphic_impl(
    typename enable_if<sizeof((_Tp*)dynamic_cast<const volatile
void*>(declval<_Tp*>())) != 0,
                       int>::type);
----

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.

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:

* 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).
* Edit bstestcase.cpp and uncomment the two includes on the first few lines
of the program.
* 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.
* 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.
* 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.
* If you tried all that without error, and no one has written in with and
LGTM already, please send an LGTM to cfe-commits!

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.

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.

http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-
Mon-20130916/089252.html
chrono:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-
Mon-20130916/089222.html
rtti:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-
Mon-20130923/089284.html
inline warnings
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-
Mon-20130923/089288.html
pragma visibility
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-
Mon-20130923/089436.html
_LIBCPP_WEAK
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-
Mon-20130923/089456.html
cmake turn of all warnings for msvc
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130923/089608.html

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.

Thanks!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131003/38433976/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: support.h.2.diff
Type: application/octet-stream
Size: 5756 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131003/38433976/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bstestcase.cpp
Type: text/x-c++src
Size: 2397 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131003/38433976/attachment.cpp>

More information about the cfe-commits mailing list