Hi, Committed the patch for support.h in revision 198481.
Thanks to G M ! Yaron 2014/1/3 Marshall Clow <[email protected]> > On Dec 26, 2013, at 5:03 AM, Yaron Keren <[email protected]> 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 <[email protected]> > >> 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 <[email protected]> wrote: > >> Hi Yaron >> >> >> On Tue, Dec 24, 2013 at 2:07 AM, Yaron Keren <[email protected]>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 <[email protected]> >>> >>>> 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 >>>> >>> >> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
