[patch] [libcxx] [win32] support.h

[Yaron Keren]

Hi,

Committed the patch for support.h in revision 198481.

Thanks to G M !

Yaron



2014/1/3 Marshall Clow <mclow.lists at gmail.com>

> On Dec 26, 2013, at 5:03 AM, Yaron Keren <yaron.keren at gmail.com> wrote:
>
> Hi,
>
> I tested this patch with gcc 4.8.2 32 bit, Visual C++ 2013 32 bit, Visual
> C++ 2013 64 bits.
>
> Without the patch (current svn), gcc passes, VC/32 fails to compile, VC/64
> compiles but fails the bit tests. With the patch, all compile and pass the
> tests.
>
> So the patch LGTM.  Can it be committed ?
>
>
> I do not currently have a setup that will let me test this.
> By eye, it looks fine to me.
>
> bittests.cpp tests several clang/gcc compiler intrinsics missing from
> Visual C++ which are required by libcxx. Can we put it into libcxx tests ?
>
>
> The test program needs a bit of work to fit into the libc++ tests.
> it should fail (aka call abort()) if any of the tests fail.
> An easy way to do that is to have check() call assert()
> Also, it needs to pass if it is build on a non-windows machine. (not fail
> to compile, etc).
>
> — Marshall
>
>
>
> Yaron
>
>
>
> 2013/12/23 G M <gmisocpp at gmail.com>
>
>> Here's a revised version of my recent patch that implements yaron's
>> suggestion to remove DWORD use. I've also made it more explicitly
>> Win32/msvc specific which I think it practically was anyway and being
>> explicit may be easier to manage.
>
>
>
> On Tue, Dec 24, 2013 at 8:45 AM, G M <gmisocpp at gmail.com> wrote:
>
>> Hi Yaron
>>
>>
>> On Tue, Dec 24, 2013 at 2:07 AM, Yaron Keren <yaron.keren at gmail.com>wrote:
>>
>>> Hi  G M,
>>>
>>> The DWORDs should not have been there at all as  _BitScanReverse64 and
>>> _BitScanForward formally take unsigned long* and not DWORD* (although they
>>> are the same thing).
>>>
>>> http://msdn.microsoft.com/en-us/library/wfd9z0bb(v=vs.90).aspx
>>>
>>>  Use unsigned long instead of DWORD and wtypes.h will not be needed.
>>>
>>> Yes I find this confusing because I'm pretty sure other documentation
>> (which I now conveniently can't find) had the types using DWORD etc. and I,
>> and I'm sure the original author of these functions that I'm fixing, based
>> their code after that and not the documentation you are linking too. That's
>> how I think the DWORD stuff got in to begin with.
>>
>> In an earlier version of this patch that I posted, but seemed to get lost
>> I removed the DWORD types etc. due to the documentation you linked to.
>>
>> What caused me to put it back was that some headers like Winnt.h still
>> refer to DWORD etc. But now looking at intrin.h which I can see is the one
>> to follow doesn't use DWORD anymore if it ever did.
>>
>> So I can see you are right, I'll re-submit another patch shortly that
>> again removes DWORD.
>>
>> Thanks.
>>
>>>
>>>
>>>
>>> 2013/12/23 G M <gmisocpp at gmail.com>
>>>
>>>> Hello Win32 libcxx watchers.
>>>> support.h implements the functions clz, clzl, clzll, ctz, ctzl, and
>>>> ctzll functions (I hate those names) for Win32 when the compiler does not
>>>> support them. So ms's cl.exe uses the implementations in support.h
>>>> but clang and gcc compilers provide these implementations themselves, not
>>>> support.h.
>>>>
>>>> Currently these support.h implementations have a few problems.
>>>>
>>>> With the removal of windows.h from support.h they no longer compile.
>>>> Even when they did compile, they were incorrect at runtime. In more detail:
>>>>
>>>> 1. support.h now fails to compile for msvc (cl.exe) because it depended
>>>> on things like DWORD from windows.h which was removed in a prior commit
>>>> and no lighter weight replacement for windows.h was provided. This patch
>>>> supplies that with wtypes.h
>>>>
>>>> 2. These functions aren't implemented for 32 bit targets of cl.exe but
>>>> they should be as they are available in the 32 bit and 64 bit gcc and clang
>>>> targets. This is because the current implementations use BitScanForward64
>>>> etc. which aren't available to 32 bit targets. This patch provides 32 bit
>>>> implementations of the clt/l/ll and clz/l/ll functions to fix that
>>>> .
>>>> 3. The above named functions current implementation in support.h is
>>>> incorrect. The return values of the support.h versions don't match the gcc
>>>> or clang return values when given the same inputs.
>>>>
>>>> The attached support.h.diff patch fixes these problems.
>>>>
>>>> Anyone wanting to verify the above statements: compile the attached
>>>> test case bittests.cpp with MS's 32 bit compiler cl.exe against the SVN
>>>> version of libcxx and it's support.h (i.e. before applying the
>>>> support.h.diff patch), you will see:
>>>> cl bittest.cpp -I/libcxx/include
>>>> c:\libcxx\include\support/win32/support.h(85) : error C2065: 'DWORD' :
>>>> undeclared identifier
>>>> <etc...> This is demonstrates problem 1 above.
>>>>
>>>> If you edit that svn version of support.h and add wtypes.h and compile
>>>> again, you will see the first errors vanish. but then you then you will see
>>>> these errors:
>>>> c:\libcxx\include\support\win32\support.h(97) : error C3861:
>>>> '_BitScanReverse64': identifier not found
>>>> c:\libcxx\include\support\win32\support.h(112) : error C3861:
>>>> '_BitScanForward64': identifier not found
>>>> This demonstrates problem 2 above.
>>>>
>>>> Switching to the 64 bit cl.exe compiler, if you compile with that, you
>>>> should be able to run it but get these runtime errors:
>>>> Test ctz failed. line 23, expected value: 32, actual value: 0
>>>> Test ctzl failed. line 33, expected value: 32, actual value: 0
>>>> Test ctzll failed. line 44, expected value: 64, actual value: 0
>>>> Test clz failed. line 57, expected value: 32, actual value: 0
>>>> <snip>
>>>> 134 tests failed
>>>> This demonstrates problem number 3.
>>>>
>>>> If you try compiling and runing this file with g++ or clang, you should
>>>> see no errors. This shows gcc and clang's implementations of the clt/clz
>>>> functions are ok yet support.h's version are not.
>>>> If you delete support.h and re get (assuming you modified it to add
>>>> wtypes.h) and then apply the support.h.diff patch and then re-compile with
>>>> g++, clang++ or cl.exe in 32 bit or 64 bit mode, the test case should then
>>>> run with no errors or output for all of those compilers, hopefully
>>>> indicating success with no regressions and proving that all the issues
>>>> stated were real and that this patch has fixed all the stated problems.
>>>>
>>>> I am going on holiday the day after tomorrow and won't have access to
>>>> email unfortunately so I won't be able to reply to any questions after
>>>> tomorrow for about a week, but I thought I'd make this patch available in
>>>> the mean time for review and anyone who wants to play with it. I'll try to
>>>> reply to any early responses that come in to this patch before I go though.
>>>>
>>>> Thanks
>>>>
>>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140104/e39e6519/attachment.html>